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

Replace Union handling for Plan parameters with more JSONSchema focused approach #560

Open
keithralphs opened this issue Jul 16, 2024 · 0 comments
Labels
enhancement New feature or request rest api Potential REST API changes

Comments

@keithralphs
Copy link
Contributor

keithralphs commented Jul 16, 2024

Currently in context.py._convert_type, there is special Type Hint handling to provide syntactic sugar for command line environments so that users don't have to type brackets to specify a List of length one. e.g if they are specifying delay in the count plan they can type either

delay=10

or

delay=[10, 30, 40]

depending on whether they are specifying a single or multiple delays, and the type checking will accept the value without brackets for the single value case. This is because _convert_type specifies the type of delay as Union[float | List[float]] which allows both cases to pass type checking. Whilst this is good for command line cases, it is not for all others e.g. JSONSchema as it introduces unnecessary complexity into the schema that Pydantic produces, as internally delay is really just a List[float], which is what the schema should contain.

This pattern of Union[<type>, List[<type>] applies to several base plans like count via wrapped.py in dls-bluesky-core.

After discussion with Callum we have agreed that it is better to change the signature of the wrapped plans that this affects to just use raw Lists for these parameters and move them to dodal, so that dls-bluesky core can be deleted. We have not however agreed on what to do next, so here are the options for discussiom:

Keith's suggestion:

  • For each affected Bluesky plan, retain a single instance of the wrapped plan that can be used for both command line and other use cases which has List parameters presented as just a List, not a Union[<type>, List[<type>]
  • Copy the code that currently 'Unionizes' Lists to the BlueAPI CLI parsing code where it can then apply the same change to List parameters, preserving the current command line experience of not having to type brackets for a list of 1 thing but only for the Command line case
  • Probably retain the 'Unionizing' code in _convert_type for the time being as a means of identifying plans with parameters that should just be a List not Union[<type>, List[<type>]

Advantages:

  • There is only one wrapped plan per source plan
  • The non-command line case (e.g. JSONSchema consumption) works transparently as the parameter really is a List
  • The command line case works transparently whether you type <val> or [<val>] as before and as it does when using raw bluesky without typechecking

Complications
In most cases the parameters that fall into this category are actually Optional, which means the are already actually Union[ | NoneType] so any handling would need to take acount of this too.

Motivation

  • Presrving existing usage experience (Bluesky and current BlueAPI CLI
  • To me, when the CLI presents/validates is just a VIew like in MVC which is tailored to the consumers of that view

Callum's suggestion:

  • Create an instance of a blueapi plan that does precisely what you want, and takes precisely the parameters you want with a particular interface in mind. E.g. if it's primarily for a GUI client probably don't include unnecessary syntactic sugar unions, if it's for a CLI client then do.
  • Follow the pattern of having a core plan with all the complex logic and many small wrapper plans which take parameters tailored to a specific client. E.g. have a core scan plan and then at least two separate wrapper plans depending on whether the user is expecting to use a GUI vs a CLI.
  • Do not touch any code in _convert_type or the CLI, pass the plan schema straight through from the python function to the client with as little fiddling/obfuscation as possible.

Advantages:

  • There is one "thing" to edit, plans, that will have consequences for the client, which makes things easier for a scientist to understand.
  • There is no code making extra, hidden changes to the plan schema that user may be unaware of.
  • A GUI is harder to change than a CLI, even an autogenerated one, if you only expect your plan to change often, just use it with a CLI client and rapidly iterate it. If it's more stable, it can be used with the GUI.

Complications

  • Likely to end up with many small, purpose-specific wrapper stubs that ultimately do the same thing.

Motivation

  • Simplifying plan developer experience on a beamline
  • While this will lead to more small, specific plans, bluesky is designed to make that cheap and easy via composability.
@callumforrester callumforrester added enhancement New feature or request rest api Potential REST API changes labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rest api Potential REST API changes
Projects
None yet
Development

No branches or pull requests

2 participants