DiamondLightSource / DiamondLightSource/mx-bluesky
Tidy-up parameters for hyperion
- Dominant language
- Python
- Stars
- 4
- Forks
- 5
- Avg merge
- 1d 30m
- Merged PRs (30d)
- 2
Description
Problem Statement
---
The current parameters packages are quite confusing especially with the proliferation of different experiment plans and the split between `common` and `hyperion` packages. The current parameter model is very complex, see
https://diamondlightsource.github.io/mx-bluesky/main/developer/hyperion/reference/param-hierarchy.html
Currently we have found all together:
* The external parameters which constitute the REST interface to Hyperion
* Mixin classes such as `WithCentreSelection`, `WithSample` which are used in the inheritance hierarchy of the external parameter model, but which have no other use
* Internal parameter classes which are not exposed in the REST interface, such as `RobotLoadAndEnergyChange` and `DetectorParams` (which is not even defined in `mx-bluesky` but in `dodal`
* Intermediate classes in the parameter inheritance hierarchy which are never instantiated as concrete classes, such as `GridCommon`
* `FeatureFlags` which are partly part of the external request (in that the request may override the flags) but also internal (in that the internal state keeps track of both the server default and the override)
The parameter classes may be densely or sparsely populated in the request, depending on the context in which they are used, for example in `load_centre_collect_full()` many parameters for the rotation are determined at runtime from the output of the gridscan + grid detection so are not populated in the request.
This leads to complex validation problems, because the parameters are overspecified which means we have to implement additional validation logic such as:
* DiamondLightSource/mx-bluesky#563
Proper validation is also hard to achieve because there is no clearly adopted pattern for the point at which to do it. Simple parameter validation can be done in the pydantic model, but because this model is serialised and deserialised during callbacks validation is repeated multiple times. One-time validation that involves e.g. filesystem access ideally should be executed only once at the REST entry point. However the current pattern we follow for implementing bluesky plans leads to confusion with multiple entry-points for each plan depending on whether it is accessed directly via blueapi or internally as part of a subplan for an encompassing experiment plan.
In addition there are various related issues which impinge on the parameters to varying degrees, some of which have been addressed but not always cleanly:
* DiamondLightSource/mx-bluesky#527 (this is still an issue but now somewhat reduced in severity)
* DiamondLightSource/dodal#966
* DiamondLightSource/mx-bluesky#747
Proposal
---
Separate the concerns
* Clearly demarcated packages and/or modules for the following:
* The REST interface pydantic data model, which is the publicly supported API that users of Hyperion and blueapi work to, and represents an immutable model of the REST call parameters that is valid for the lifetime of the REST call. This should be normalised so that parameters in each call cannot be overspecified.
* The internal parameter pydantic data model that is not exposed via the REST model but is usable by sub-plans and callbacks. This model may change as additional information becomes available to populate it during plan execution and is part of the execution context. This would contain things such as the `DectectorParams` and the resolved state of the `FeatureFlags`.
* Mixins and abstract core parameter model archetypes that can be shared across beamlines, that may be used internally and externally as the basis for concrete parameter types but are not in themselves directly usable.
* The blueapi experiment plan entry points, which are responsible for the following:
* Any one-time validation of parameters that is non-trivial and not suitable to be performed by Pydantic validation
* Mapping of the external REST pydantic data model to the internal data model as necessary - this responsibility should be refactored out of the Pydantic classes.
* Delegating plan execution to the internal plan entry points.
* Error and exception handling to report plan execution status and errors.
* The internal top-level plans that are responsible for implementing the actual experiment plans in whole or in part.
What this does not
* Alter the external REST interface - these changes shouldn't require any clients to change their code
* Change how we talk to the config server
* Change how we serialize data - we still send pydantic models to the callbacks.
* A wholesale change to the parameter model - there may be some changes but hopefully most of the classes will still be present but their responsibilities clearer and they may be simpler and cleaner as some of the properties and implementation may be moved out of them.
Contributor guide
Assessment
This issue has not been assessed yet.