Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: build ordering #426

Open
hartytp opened this issue Dec 9, 2024 · 0 comments
Open

RFC: build ordering #426

hartytp opened this issue Dec 9, 2024 · 0 comments

Comments

@hartytp
Copy link
Contributor

hartytp commented Dec 9, 2024

Overview

At present the ordering of steps during build / execution - init_params, get_default_analyses, collect_result_channels broadcast_metadata - is not documented and depends on the scan runner. I would find it helpful to document this and see if we can establish a consistent ordering. In particular, I'd like to see if we can establish an ordering where get_default_analyses is always called after init_params but before collect_result_channels.

Motivation

I've found it important to be able to make analyses depend on parameter values - for example, supposing I want to seed/constrain a fit parameter based on the value of an experiment parameter. That relies on the parameters being initialized before get_default_analyses (bit of a misnomer at this point?) is called.

If fragments are run using a TopLevelRunner this works fine. However, for subscans there is a bit of a rub because the subscans need to forward analysis results to result channels. This is achieved by calling get_default_analyses during build_fragment (as part of setup_subscan). At this point the parameters have not yet been initialized.

This is somewhat similar to the issues ARTIQ has around calling build multiple times - once for discovery and once to actually build the code. Arguable, calling these functions to discover parameters is a bit of a hack and we'd probably be better off using a more declarative approach - parameters / results / analyses are declared as class-level attributes and then we use introspection for discovery. But that's a bigger change to ARTIQ/ndscan.

Proposed solution

  • Create and document an ordering of steps during build, which is consistent regardless of how the fragment is run.
  • Ensure that in all cases, parameter initialization happens first, then get_default_analysis then result channel collecting (this requires small modifications to both TLR and subscans): modify the subscan code to create the analysis schema / result channels inside get_default_analyses instead of inside build_fragment modify the TLR to call get_default_analyses before the results are configured

That way ndscan provides a clear initialisation order:

  • parameters are always initialised first
  • then get_default_analyses is called
  • then results are collected

Alternatives

A few options I can think of:

  • don't allow analyses to be configured based on parameter values (a real shame because I've found it adds a lot of value in certain circumstances)
  • move the code which configures the analyses based on parameter values to somewhere other than get_default_analyses (e.g. a finalise_analyses method) which is called before the schema are finalized for broadcast. I really don't like this approach because it ends up with "partially constructed objects". I find that dataflow is vastly simpler to understand when constructing objects (which includes configuring them) is all done in one place. Separating these steps just introduces footguns and makes code harder to read.

Background

Some notes to self:

  • TopLevelRunner never actually calls init_params there is an undocumented assumption that this will have been called previously, for example by FragmentScanExperiment which calls init_params and then constructs a TopLevelRunner in prepare
  • during build TopLevelRunner collects result channels and assigns sinks, then calls get_default_analyses
  • TopLevelRunner calls broadcast_metadata at the beginning of run (although the schema is frozen during build so changes between build and run result in the applet receiving an incorrect schema although this isn't flagged anywhere)

As a side note: one thing I really struggle with in this part of the code is how spread out schema creation is. There are bits of it all over the place. This makes it really hard to follow the serialization process and reason about things like "when is the last time I can mutate this object and know that changes will be received by the applet?". It would make a huge improvement in ease of following the code if there were a single serialization step where this was all done in one go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant