Add type annotations to plans and plan stubs, update docstrings #1610
Conversation
|
There is an existing PR with similar changes: #1600 |
bluesky/plans.py
Outdated
|
|
||
|
|
||
| def count( | ||
| detectors: List[Readable], |
There was a problem hiding this comment.
This should probably be Union[List[Readable], Tuple[Readable]] for generality. Using Iterable[Readable] will cause issues with Queue Server parameter validation.
There was a problem hiding this comment.
Good point, I think Sequence works as a supertype of both. Would Sequence[Readable] work with Queue Server validation? It's much more concise.
There was a problem hiding this comment.
I made some changes to the Queue Server. Using Iterable is good now.
There was a problem hiding this comment.
I don't think iterable will work for these plans though. Some of them don't actually accept an iterable because they do random access. If you pass them an iterable they'll break. See example below: passing count a generator of detectors (a valid iterable) results in events that hold no information about those detectors:
>>> from ophyd.sim import det1, det2, motor1
>>> from bluesky import RunEngine
>>> import bluesky.plans as bp
>>> from pprint import pprint
>>>
>>> def detector_generator():
... yield det1
... yield det2
...
>>> RE = RunEngine()
>>> RE(bp.count([det1, det2]), lambda name, doc: pprint({"name": name, "doc": doc}))
{'doc': {'detectors': ['det1', 'det2'],
'hints': {'dimensions': [(('time',), 'primary')]},
'num_intervals': 0,
'num_points': 1,
'plan_args': {'detectors': ["SynGauss(prefix='', name='det1', "
"read_attrs=['val'], "
"configuration_attrs=['Imax', 'center', "
"'sigma', 'noise', 'noise_multiplier'])",
"SynGauss(prefix='', name='det2', "
"read_attrs=['val'], "
"configuration_attrs=['Imax', 'center', "
"'sigma', 'noise', 'noise_multiplier'])"],
'num': 1},
'plan_name': 'count',
'plan_type': 'generator',
'scan_id': 1,
'time': 1694075006.16437,
'uid': 'a76d7658-12b7-4296-b879-22513207a20a',
'versions': {'bluesky': '1.10.0+91.gbf8d9fd4', 'ophyd': '1.8.0'}},
'name': 'start'}
{'doc': {'configuration': {'det1': {'data': {'det1_Imax': 5,
'det1_center': 0,
'det1_noise': 'none',
'det1_noise_multiplier': 1,
'det1_sigma': 0.5},
'data_keys': OrderedDict([('det1_Imax',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det1_Imax'}),
('det1_center',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det1_center'}),
('det1_sigma',
{'dtype': 'number',
'shape': [],
'source': 'SIM:det1_sigma'}),
('det1_noise',
{'dtype': 'integer',
'enum_strs': ('none',
'poisson',
'uniform'),
'shape': [],
'source': 'SIM:det1_noise'}),
('det1_noise_multiplier',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det1_noise_multiplier'})]),
'timestamps': {'det1_Imax': 1694074921.2635274,
'det1_center': 1694074921.263523,
'det1_noise': 1694074921.2635052,
'det1_noise_multiplier': 1694074921.2635176,
'det1_sigma': 1694074921.2635317}},
'det2': {'data': {'det2_Imax': 2,
'det2_center': 1,
'det2_noise': 'none',
'det2_noise_multiplier': 1,
'det2_sigma': 2},
'data_keys': OrderedDict([('det2_Imax',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det2_Imax'}),
('det2_center',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det2_center'}),
('det2_sigma',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det2_sigma'}),
('det2_noise',
{'dtype': 'integer',
'enum_strs': ('none',
'poisson',
'uniform'),
'shape': [],
'source': 'SIM:det2_noise'}),
('det2_noise_multiplier',
{'dtype': 'integer',
'shape': [],
'source': 'SIM:det2_noise_multiplier'})]),
'timestamps': {'det2_Imax': 1694074921.2637875,
'det2_center': 1694074921.2637837,
'det2_noise': 1694074921.2637644,
'det2_noise_multiplier': 1694074921.2637773,
'det2_sigma': 1694074921.2637923}}},
'data_keys': {'det1': {'dtype': 'number',
'object_name': 'det1',
'precision': 3,
'shape': [],
'source': 'SIM:det1'},
'det2': {'dtype': 'number',
'object_name': 'det2',
'precision': 3,
'shape': [],
'source': 'SIM:det2'}},
'hints': {'det1': {'fields': ['det1']}, 'det2': {'fields': ['det2']}},
'name': 'primary',
'object_keys': {'det1': ['det1'], 'det2': ['det2']},
'run_start': 'a76d7658-12b7-4296-b879-22513207a20a',
'time': 1694075006.1660883,
'uid': 'c84977c2-c35a-4a5e-8e1a-d4f18570b885'},
'name': 'descriptor'}
{'doc': {'data': {'det1': 5.0, 'det2': 1.7649938051691907},
'descriptor': 'c84977c2-c35a-4a5e-8e1a-d4f18570b885',
'filled': {},
'seq_num': 1,
'time': 1694075006.1714554,
'timestamps': {'det1': 1694075006.1706445, 'det2': 1694075006.1710362},
'uid': 'f6507c44-3224-44f2-bfb2-6cf765dd20dd'},
'name': 'event'}
{'doc': {'exit_status': 'success',
'num_events': {'primary': 1},
'reason': '',
'run_start': 'a76d7658-12b7-4296-b879-22513207a20a',
'time': 1694075006.1717513,
'uid': '167a66b4-b58c-4cbf-9361-8f65c45fcf38'},
'name': 'stop'}
('a76d7658-12b7-4296-b879-22513207a20a',)
>>> RE(bp.count(detector_generator()), lambda name, doc: pprint({"name": name, "doc": doc}))
{'doc': {'detectors': ['det1', 'det2'],
'hints': {'dimensions': [(('time',), 'primary')]},
'num_intervals': 0,
'num_points': 1,
'plan_args': {'detectors': [], 'num': 1},
'plan_name': 'count',
'plan_type': 'generator',
'scan_id': 2,
'time': 1694075016.1725795,
'uid': 'cad1615b-f34f-4c17-bac7-c43eaaae8019',
'versions': {'bluesky': '1.10.0+91.gbf8d9fd4', 'ophyd': '1.8.0'}},
'name': 'start'}
{'doc': {'configuration': {},
'data_keys': {},
'hints': {},
'name': 'primary',
'object_keys': {},
'run_start': 'cad1615b-f34f-4c17-bac7-c43eaaae8019',
'time': 1694075016.1734006,
'uid': '7a91a155-0fd6-439e-a0f3-58a4d1deaa62'},
'name': 'descriptor'}
{'doc': {'exit_status': 'success',
'num_events': {},
'reason': '',
'run_start': 'cad1615b-f34f-4c17-bac7-c43eaaae8019',
'time': 1694075016.1739008,
'uid': '36c8009d-13d6-411a-88a8-5507b7680fcb'},
'name': 'stop'}
('cad1615b-f34f-4c17-bac7-c43eaaae8019',)
>>> There was a problem hiding this comment.
Good point. May be the right solution is to convert detectors (and motors) to a list in the plan wherever it is possible and annotate the respective parameters as Iterable for generality? @danielballan
There was a problem hiding this comment.
We can also leave it as List. The Queue Server always using lists.
There was a problem hiding this comment.
It doesn't support Sequence then?
There was a problem hiding this comment.
It does. We can use Sequence
There was a problem hiding this comment.
I'm generally in favor of "Accept lazy iterables (generators) everywhere!" but I think that it's pretty reasonable to expect a Sequence here.
There was a problem hiding this comment.
I think make it Sequence here to reflect what it will accept and make a separate issue for making all plans accept lazy iterables. #1613
|
|
||
|
|
||
| def grid_scan(detectors, *args, snake_axes=None, per_step=None, md=None): | ||
| def grid_scan( | ||
| detectors, |
There was a problem hiding this comment.
Good spot! Will replace based on outcome of https://github.com/bluesky/bluesky/pull/1610/files/7ebbfcf3408f15d11ece2f847a70b198103aef3d#r1310969275
bluesky/utils/__init__.py
Outdated
| CustomPlanMetadata = Dict[str, Any] | ||
|
|
||
| #: Scalar or iterable of values, one to be applied to each point in a scan | ||
| ScalarOrIterable = Union[float, Iterable[float]] |
There was a problem hiding this comment.
Should it be ScalarOrIterableFloat or ScalarOrIterableNumber for clarity?
bluesky/plans.py
Outdated
| def rel_list_scan(detectors, *args, per_step=None, md=None): | ||
| def rel_list_scan( | ||
| detectors: List[Readable], | ||
| *args: Any, |
There was a problem hiding this comment.
Here is another issue with the Queue Server, since it will not attempt to convert strings to objects (motors in this particular example) if parameter is annotated with a type not associated with Ophyd devices (such as Readable, Movable etc). So in this case *args should have no annotation (by default the queue server tries to convert all strings to objects) or the type should be something like Union[Movable, Any]. It should also work with mypy.
There was a problem hiding this comment.
Interesting, for blueapi we went the opposite way. Only attempts to convert strings if they are annotated with protocol types or things that follow protocol types.
There was a problem hiding this comment.
This is what the Queue Server does. But it also attempts to convert all strings it finds in values of parameters that are not annotated. This may lead to errors, since any string matching a plan or a device name will be converted to object, but this never happened in practice. Unfortunately, our local users do not want to learn about type annotations, which makes this feature very useful.
There was a problem hiding this comment.
So a parameter annotated with typing.Any is considered having type and the Queue Server does not try to convert strings to objects.
|
@dmgav as an aside in terms of types the queueserver supports, I've been using the types supported by pydantic as a guide: https://docs.pydantic.dev/1.10/usage/types/, it provides a useful rule of thumb. |
|
Unfortunately, Pydantic functionality is insufficient for proper type checking. The problem with |
|
Indeed, pydantic is a bit lenient, it's more using the list of types as a guide for what to support than actually using the pydantic library itself |
|
CI issues fixed in: #1612 |
callumforrester
force-pushed
the
plan-type-annotations
branch
from
6f9c3a1
to
40a3e02
Compare
|
@danielballan Could make a custom type but I wouldn't like to hide away the As an aside, I note that |
Description
Add type annotations to plans and plan stubs. IDEs such as vscode should now provide hints when using them. See screenshot below. Fix and update out-of-date docstrings on plans and plan stubs.
Motivation and Context
Makes development with plans easier with auto-fill etc. Also allows downstream projects to make use of static type checking utilities such as mypy.
How Has This Been Tested?
Docstrings and type annotations should not semantically change the code. All tests passing supports this.
Screenshots (if appropriate):