You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
At present the ordering of steps during build / execution - init_params, get_default_analyses, collect_result_channelsbroadcast_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 buildTopLevelRunner 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.
The text was updated successfully, but these errors were encountered:
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 whereget_default_analyses
is always called afterinit_params
but beforecollect_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 callingget_default_analyses
duringbuild_fragment
(as part ofsetup_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
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 insideget_default_analyses
instead of insidebuild_fragment
modify the TLR to callget_default_analyses
before the results are configuredThat way ndscan provides a clear initialisation order:
get_default_analyses
is calledAlternatives
A few options I can think of:
get_default_analyses
(e.g. afinalise_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 callsinit_params
there is an undocumented assumption that this will have been called previously, for example byFragmentScanExperiment
which callsinit_params
and then constructs aTopLevelRunner
in preparebuild
TopLevelRunner
collects result channels and assigns sinks, then callsget_default_analyses
TopLevelRunner
callsbroadcast_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.
The text was updated successfully, but these errors were encountered: