DiamondLightSource / DiamondLightSource/blueapi
Redesign plan argument handling
- Dominant language
- Python
- Stars
- 13
- Forks
- 13
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 25
Description
We currently build a pydantic model from the plan args and use that model to convert from the JSON params sent as part of a TaskRequest. This leads to several issues
* All arguments must be addressable via keywords. [Positional only args](https://github.com/DiamondLightSource/blueapi/issues/780) and [varargs](https://github.com/DiamondLightSource/blueapi/issues/1450) are not supported
* Devices need special handling to convert from strings to devices #154
* Composite devices (built at runtime from existing devices) have their handling intertwined with the device name to device handling
* The `inject` method we currently use to provide default arguments to plans means plans can't be run outside blueapi but this is not restricted by python. Eg, calling a plan defined as `def foo(mov: Movable = inject("bar")): ...` fails with a run time error when trying to move the string instead of giving a type error. Type-checkers can't pick this up.
* Non-devices are not supported (eg [plans](https://github.com/DiamondLightSource/blueapi/issues/194) or [clients for external services.](https://github.com/DiamondLightSource/blueapi/issues/1462))
Not directly related to using a model for validation but plan args are validated twice, once on submission and again when the plan is actually started.
Redesigning the way args are handled has been discussed several times and the general consensus was for the following requirements
* We shouldn't have to change every existing plan. This would need a huge amount of churn and introduce new version dependencies
* No new dependencies between blueapi and dodal/plan repo in either direction. A third API only package is not out of the question. (This may also be useful for utilities like the `DeviceManager`)
* Devices built by a device manager should be injectable in a similar way to the current system
* Non-devices should be available. Eg a config service client should be usable by plans without relying on global state
* Composite devices should be built at runtime with overrides of individual components if possible
* Type-checking should work
* Vararg and positional only args should be supported (#1185)
* Plan definitions should remain as simple as possible. A bit subjective but the current `inject` method is fairly concise.
* If possible, less duplication than the current system would be good, eg `foo: Movable = inject("foo")` could be reduced to something like `foo: Injected(Movable)` and the name would not need to be duplicated.
Contributor guide
Research direction
No files or tests are named. Start by tracing the current pydantic model conversion and the inject method, then review the linked issues and existing discussion to compare proposed designs. Done means a decided approach that preserves existing plans, supports injection, non-devices, composite overrides, type-checking, and positional or vararg arguments without the listed dependency problems.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, backend, backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100