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

fqns/schema thoughts #272

Open
hartytp opened this issue Jan 20, 2022 · 11 comments
Open

fqns/schema thoughts #272

hartytp opened this issue Jan 20, 2022 · 11 comments

Comments

@hartytp
Copy link
Contributor

hartytp commented Jan 20, 2022

ndscan is an awesome addition to artiq. I've been trying to get my head around a few aspects of the design and can't yet tell if I'm hitting against rough edges of an MVP or careful design choices made for reasons I haven't understood. I'm afraid this post is a bit of a brain dump rather than a fully coherent set of questions, but hopefully it's useful for other people looking at the code (even if it's just pointing out that I've got completely the wrong end of the stick). @dnadlinger and others, any comments / corrections really welcome!


Glossary

schema: string representations of objects (fragment, scan parameter, result channel, etc). This ends up being roughly a json-ified dictionary.
fqn: fully-qualified name of an object as a string
schemata: dictionary of schema with fqns as keys

Why do we have schema?

There is a really nice discussion in the design retrospective which I found really helpful. My current understanding is:

  1. applets need to understand details about the scan. This is critical for allowing the ndscan applet to automatically figure out the best way of visualising a given scan, for enabling the applet to update datasets linked to scan parameters, etc. To achieve this, the entire scan needs to be sent to the applets, which is done by converting the objects into string representations which can be broadcast as datasets
  2. we want to be able to reconstruct scans after the fact from the hdf5 results file. e.g. ndscan.show allows us to easily reproduce the default plots for an experiment. This feature requires us to convert the scan to something that can fit inside the hd5 file
  3. (This one I'm particularly sketchy on...) exportability in the argument interface. The argument interface GUI allows users to override any scan parameter in a really nice way. I haven't quite got my head around how this is implemented, but e.g. AFAICT it uses the tree of fqns to allow the users to find parameters
  4. subscans?? (not sure, I haven't read that part of the codebase yet)

Bits I've found confusing...

I found (still find) the parts of ndscan which involve schema hard to get my head around. Trying to boil this down to something concrete, I think it's the following (c.f. the design retrospective):

Obscuring the ontology

As a newcomer to the codebase, I found myself thinking "what on earth is this object and where does it come from". With all the string representations of objects, often referred to by rather generic names ("schema" is used in the codebase to refer to schema for multiple different types of objects) and sparse doc strings, understanding a piece of code often turned into a rather non-local problem.

I found myself asking "what kind of object is this the schema for?" and not being able to answer that without digging through the code to find where it was called from, often having to search through a few layers to get an answer. This felt particularly bad on the applet side where everything is a dictionary and it took me quite a while to trace those through the dataset broadcast and back into the experiment code. It's also not easy to do this by searching the codebase because of the use of generic terminology and lack of concrete types.

To some degree ndscan gets away with this because the ontology is pretty simple, but it still felt like a major point of friction. Even some daft things like the figuring out the convention that that "channel" is synonymous with "result channel".

No clear finalization step

If I've followed the design correctly, the scan structure is communicated to the applet in the TopLevelRunner.run method (via _broadcast_metadata ). To avoid a messy synchronization problem, the scan needs to be "finalized" before then. When I first came to the codebase, I assumed this would be done in a single finalization step, which freezes the scan structure and creates the schema representation.

In fact, creating the schema is spread across at least a couple of places. Some is done at run time (either inside TopLevelRunner.[_broadcast_metadata](

def _broadcast_metadata(self):
) or by [scan_runner.describe_scan](
scan_schema = describe_scan(spec, self._fragment,
) depending on how the scan is run). Some bits are frozen at build time by the ArgumentInterface. Overall, I still find it hard to trace through where various bits of the schema are created and why they are done there (c.f. #269). To what extent would it be possible / desirable to have a single function that recursively does the entire thing?

Right now I find it really hard to follow how the information in the schema is used and what the impacts of changing something at various parts of the scan lifecycle are. (semi-related, why is the scan stored in a pyon argument rather than a dataset?).

No decode / encode methods

Parameters and result channels all have a describe method which produces a schema for the object. This is nice as it gives one a clear place to understand what's in the schema (although, arguably, it might be better to have a base class which provides this functionality since all parameters have the same structure of schema). However, on the applet side the schema are used instead of objects. I wonder if it would be worth having a decode (bikeshed name) method that reconstructs the objects from the schema?

But, other parts of the ontology are converted in a more ad hoc fashion (e.g. is there a reason that ScanAxis doesn't have a describe method?

axis_specs = [{
)

@hartytp
Copy link
Contributor Author

hartytp commented Jun 9, 2022

FWIW I still don't really get why build_fragment arguments are mangled into the fqn. e.g. this means one cannot pass an argument to build_fragment that is determined at runtime even if it does not change the parameter tree. There are also issues around passing classes into build_fragment.

@dnadlinger
Copy link
Member

dnadlinger commented Jun 11, 2022

Apologies for not getting to this earlier, but in general, your summary sounds about right. Hence, just a few points:

I'm using the term "schema" in vaguely the same sense as in "database schema" or "XML schema", that is, to unambiguously refer to the metadata that defines the shape and meaning of the parameters and results, the types, rather than the values taken by part of a scan/… (or to precise, as you surmise, the description detailing those types). As for "schemata", in actuality, I was just using it as the plural of "schema", but it might well be that it always ends up referring to a mapping of a name to the respective schema.

The summary of the "why" also sounds correct. As for "(This one I'm particularly sketchy on...) exportability in the argument interface", yes, the argument editor (i.e. the experiment submission UI) is the main client of the parameter schema information. The way ndscan parameters integrate with the default ARTIQ experiment scheduling infrastructure (which is entirely unchanged) is that the argument editor plugin receives all the schemata in a PYON argument named PARAMS_ARG_KEY, and writes back any overrides/scan axes/… to the same. If you launch the dashboard without the ndscan plugin enabled, you can actually see that it just shows up as that raw value; same of course goes for the expid arguments saved to the HDF5 results file. As for "semi-related, why is the scan stored in a pyon argument rather than a dataset?", this is why – the argument editor needs to read and write this data, and do so before the experiment is submitted. Since it needs to be stored there anyway, writing the information to a dataset in addition to that would be superfluous.

As for the fact that everything is a nested dictionary of mostly strings, you suspect correctly that the main reason here is that the data needs to be conveyed between both argument editor and experiment, and then experiment and applets (PYON)/ results files (HDF5). As you correctly point out, a way to reparse these dictionaries into a richly typed form (i.e. Python objects with docstrings, …) could be useful for code readability/explorability. Back when writing the bulk of this, I chose to have ndscan.plots directly operate on the dictionary representation as a matter of YAGNI/KISS, but especially now that many people are also wanting to write their own offline experiment result analysis routines (including ndscan.show and the results package), it would probably be a worth it to add such a "client library". Note, though, that this would necessarily remain separate from the ndscan.experiment objects, which are intimately tied to the particular object hierarchy in the user's ARTIQ code. At best, ndscan.experiment might use the "schema classes" to build up the exported information, but that could also turn out to be overkill. As for

Even some daft things like the figuring out the convention that that "channel" is synonymous with "result channel".

I agree that an explicit description would be good to have, whether in the form of docstring'd code, or just a textual description of the JSON contents. If it is any consolation, I was quite careful to stick to consistent terminology and not to use the same word in more than one meaning throughout, though.


Regarding

FWIW I still don't really get why build_fragment arguments are mangled into the fqn. e.g. this means one cannot pass an argument to build_fragment that is determined at runtime even if it does not change the parameter tree. There are also issues around passing classes into build_fragment.

the main reason here is that while the 1:1 mapping between FQNs and Python classes is straightforward and clear, sometimes being able to parametrise a type is helpful – for instance, to have multiple variants of a class for a number of different transition parameters. Like with C++ templates, those variants aren't actually related from the perspective of the type system (here: they just have unrelated FQNs). The concept of templates/generic types doesn't really exist in Python (at least not before the recent typing additions), hence by fiat, ndscan decides to treat build_fragment arguments as those "template parameters".

In this, the current, design, the way to pass extra arguments is simply to provide extra values later (through some other method, or just by setting a public property in the subfragment). It has to be that way round – rather than providing the parameters later –, as the FQN is used in setattr_param/…

However, I've been somewhat on the fence about this design ever since I added it in the first place. There are two alternatives: One is to explicitly require the user to actually create different Python types. In a sense, this would be the lowest-overhead option, as it doesn't introduce any new concepts. If there are enough of them that you don't want to write them all out by hand, making sure they end up with different names would requires some fairly esoteric code such as this, though (which could be addressed through documentation, I suppose):

class Foo(Fragment):
    def build_fragment(self):
        self.setattr_fragment("freq", FloatParam, "Frequency",
                              f"dataset('{self.name}.freq', 1e6)")

def make_foo(variant):
    class FooInstance(Foo):
        name = variant
    FooInstance.__qualname__ = f"Foo_{variant}"
    return FooInstance

class FooUser(ExpFragment):
    def build_fragment(self):
        self.setattr_fragment("foo_stretch", make_foo("stretch"))
        self.setattr_fragment("foo_clock", make_foo("clock"))

Another would be to add an explicit mechanism for specifying "template values" to Fragment, e.g. through a dict argument to build_fragment, which might be matched/validated against a set pre-defined in some class members by build().

@dnadlinger
Copy link
Member

dnadlinger commented Jun 11, 2022

@pmldrmota: Any thoughts on explicitly requiring users to make new classes for different variants like in the example (for the different transitions, …)? Just removing the build_fragment argument -> FQN mangling kind of seems the way to go for me, as it strictly removes complexity from ndscan – users just have to make sure every class (as defined by a unique __qualname__) has exactly one parameter layout. The overhead of generating a name doesn't seem too high a price to pay for simplicity in the simple case (being able to state the "one class == one FQN" rule), although it is a fairly common use case…

@dnadlinger
Copy link
Member

I suppose we could also have a specialise_fragment() funtion in ndscan that wraps the above in a generic way…

@dnadlinger
Copy link
Member

On the other hand, we need to make sure that there are good diagnostics for the schema mismatch case if we do remove the FQN mangling entirely, as one advantage of the current design is that it is easy to "accidentally" do the right thing without even thinking about the fact that there is only a single schema per FQN, as the most natural way to get values that would change e.g. some parameter defaults would precisely be from build_fragment arguments…

@hartytp
Copy link
Contributor Author

hartytp commented Jun 11, 2022

one advantage of the current design is that it is easy to "accidentally" do the right thing without even thinking about the fact that there is only a single schema per FQN, as the most natural way to get values that would change e.g. some parameter defaults would precisely be from build_fragment arguments…

I agree with this. A couple of disadvantages: fqns can get pretty messy and often have a trailing _None_False_None_... which makes them hard to read, particularly for the experiment name in the browser / applets. For this reason, we often end up creating a wrapper class for the top-level fragment which has a build_fragment that doesn't take any parameters so we end up with something more readable.

The case of having a parameter which does not affect the schema is not that rare. Currently if one includes it in build_fragment some things break in a pretty non-obvious way.

For generic code which can take a class as a parameter the FQNs get particularly bad and some things can easily break due to the way ARTIQ hacks the import mechanism during the first call to build.

@hartytp
Copy link
Contributor Author

hartytp commented Jun 11, 2022

If you launch the dashboard without the ndscan plugin enabled, you can actually see that it just shows up as that raw value; same of course goes for the expid arguments saved to the HDF5 results file

Aah, right, I had forgotten about the plugin. That makes more sense.

@hartytp
Copy link
Contributor Author

hartytp commented Jun 11, 2022

Shall we close this brain dump issue now and open new issues if there are any specific things we want to discuss further?

@hartytp
Copy link
Contributor Author

hartytp commented Aug 7, 2022

@dnadlinger one of the biggest challenges I still find with ndscan as a user is the lack of concrete types for things impeding discoverability. With all the dictionaries / tuples / strings I find myself constantly needing to refer to the documentation / do some searching to figure out what my options are.

Assuming we can get upstream pyon to (finally) accept a couple of basic data types like enum and namedtuple how would you feel about adding a bit of additional structure to ndscan?

Edit: the specific thing I was struggling with was OnlineFit's data parameter. What axes can we use here? Can it just be x y and y_err or are there other options (e.g. for multidimensional fits)? The docstring only mentions x and y and without reading the code I'm a little lost here.

@hartytp hartytp mentioned this issue Aug 8, 2022
8 tasks
@hartytp
Copy link
Contributor Author

hartytp commented Oct 31, 2022

Closing this issue as it doesn't feel actionable.

@hartytp hartytp closed this as completed Oct 31, 2022
@hartytp
Copy link
Contributor Author

hartytp commented Oct 31, 2022

Retrospective from having spent some time a while back thinking about the issue I highlighted above about confusion over types / interfaces. See also #308

I got into this because I wanted to add custom online fits (see #309). This meant being able to tell the applet what fitting code I wanted to run. I toyed around with extending the schema to do this (e.g. specifying a module and class that the applet should import the fit code from) but this ended up being complex and inflexible. The schema mechanism feels like a bit of a roll-your-own approach to a well-solved problem which is fine for simple things but quickly becomes messy. I do wonder if we'd be better off using a more standard approach (pickles, marshmallow, etc). I went for base64 encoded dill pickles, which worked well for what I wanted to do.

That bit worked out nicely. Where I ran into real trouble was trying to hook this into ndscan. The issue I hit was that there isn't a well-defined interface which maps between the experiment-side and applet-side scan representations. The kind of thing I was hoping to get to was an abstract scan model, which describes the entire scan in a way that is meaningful to both the experiment-side and applet-side code and a systematic way of constructing applet-side objects from it. I was hoping that this could then be serialised / deserialised in one go when the scan metadata is broadcast so there was a single point for that dataflow.

Right now though, things are very entangled in a way that makes abstracting (or understanding at all) the commonalities really difficult. To illustrate the point, here is how things currently stand with regards to plot annotations. As I understand it (but I may have got a few things wrong), things are as follows:

On the experiment side we have Annotation objects, whose schema consist of kind, coordinates, parameters, data

On the applet side things are a bit more complicated. The basic dataflow as far as I understand it (which is still pretty limited despite having spent a while looking at the code) is:

  • The applet is defined here and implements an ARTIQ SimpleApplet
  • The main widget for the applet is defined here and implements an ndscan RootWidget
  • the main widget has an ndscan SubscriberRoot to get scan metadata. This is fed from the SimpleApplet's subscriber
  • The SubscriberRoot watches data from the master until it gets an axes entry in the schema. At that point it creates a suitable scan model based on the metadata it receives from the master and pass further data to the model's data_changed method
  • Question: why put so much code inside ndscan.plots.model.__init__.py? I find this needlessly opaque!
  • the SubscriberScanModel 'data_changed' callback then deserializes the various elements of the scan one at a time.
  • In terms of annotation handling, the SubscriberScanModel passes JSON data it receives to the parent _set_annotation_schema method
  • anyway, set_annotation_schema creates an ndscan.plots.model.Annotation item for each annotation. These are simple data structures which just store the kind, coordinates, parameters and data where the coordinates and data have been converted into AnnodationDataSource objects
  • one thing that threw me here is that _set_annotation_schema is called both from SubscriberScanModel.get_data when we receive information about the annotations, and also from ScanModel._set_online_analyses.

The thing I struggled with here was how piecemeal the whole thing is. I had expected that the scan model would be something static that is known once the experiment is built and we could serialise / deserialise in one go. Instead there is a lot going on here and it's hard to follow how all the bits are intended to fit together / what's driving the complexity. Anyway...back to annotations...

AFAICT at present ndscan.plot.model.Annotations are only used inside the XY plots, although I guess the design aim was to keep this flexible for future? The XY plot applet converts the Annotation into an appropriate AnnotationItem subclass. These subclasses do the actual work.

The challenge here is that there isn't really an interface for how AnnotationItems should be created from an Annotation so one can't easily extend it to add a custom annotation. The XY plot creates the various annotation items while mixing in various bits of its internal state, with each annotation needing different information. I did have a go at defining a common interface here, but there were so many different steps along the way which were ad hoc and entangled in various ways that it felt like an uphill battel and I gate up and hacked something in...

@dnadlinger dnadlinger reopened this Nov 1, 2022
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

2 participants