Wrap plans and plan stubs with lists of tuples

[EmsArnold]

Closes #1498, and as an alternative to #1702. Scan syntax is more similar to both GDA and bluesky.

Wraps bluesky plans (scan, rel_scan, grid_scan, rel_grid_scan) and plan stubs (rd, stop) for use in blueapi. Intended as a replacement for gda-style start stop step scans until spec_scan is stable and supports start stop step rather than start stop num. Start stop step scans are achieved through generating a list for use with list_scan, rel_list_scan, ...

Concurrent trajectory scans (scan, etc.) and independent trajectory scans (grid_scan, etc.) are both accessed through the same plan and differentiated by number of parameters given for each movable - similar to current GDA-style scans.

Instructions to reviewer on how to test:

1. Do thing x
2. Confirm thing y happens

Checks for reviewer

• Would the PR title make sense to a scientist on a set of release notes

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.12%. Comparing base (a039d73) to head (531c2df).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1734      +/-   ##
==========================================
+ Coverage   99.11%   99.12%   +0.01%     
==========================================
  Files         319      319              
  Lines       12267    12439     +172     
==========================================
+ Hits        12158    12330     +172     
  Misses        109      109              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RJCD-Diamond]

whatt else does this need until it can not be a draft and be merged?

[EmsArnold]

@RJCD-Diamond, it should be good to go, but could likely use some input on plan names and and other changes.
Perhaps others (@olliesilvester, @noemifrisina, @DominicOram, @oliwenmandiamond, @fajinyuan) would have some opinions on this naming / scanning convention as opposed to the one laid out in #1702.

[RJCD-Diamond]

Why did you go with num_scan when both gda and bluesky refer to this as scan? Are the behaviours different?

[EmsArnold]

Why did you go with num_scan when both gda and bluesky refer to this as scan? Are the behaviours different?

scan in gda is a step scan, hence the distinction of both num_scan and step_scan, to make it more apparent for users coming from either bluesky or gda

[DominicOram]

until spec_scan is stable and supports start stop step rather than start stop num

Does this mean you intend for these to be removed when bluesky/scanspec#186 is done? If so I would put a comment in each plan you intend to retire saying that it will be replaced and pointing to that issue.

Concurrent trajectory scans (scan, etc.) and independent trajectory scans (grid_scan, etc.) are both accessed through the same plan and differentiated by number of parameters given for each movable - similar to current GDA-style scans.

Is this a hard requirement? I think I would rather we have them separate as:

• We can then just point people to https://blueskyproject.io/bluesky/main/plans.html as documentation
• They seem to actually be quite different things and lead to weirdness e.g. snake_axes meaning nothing in some contexts

On that note, having different behaviour based on whether we provide a num for each axis or not is also not intuitive. Again, I would just stick to the bluesky way of being more explicit about what you actually want to do

[RJCD-Diamond]

@EmsArnold Been using this for commissioning purposes on i22 - and strongly suggest we make sure that all of these plans stubs wait for the plan to finish by default

[EmsArnold]

Does this mean you intend for these to be removed when bluesky/scanspec#186 is done? If so I would put a comment in each plan you intend to retire saying that it will be replaced and pointing to that issue.

I think we should reduce the number of plans which result in the same functional scan (i.e. having two plans which do a scan based on start stop num, one bluesky and one scanspec), as this will likely cause confusion for scientists and users when determining which plan they want to use. As to when (or even if) these plans move from bluesky to scanspec is a question of added utility - i.e. if scanspec can easily add the option to flyscan with limited onus upon the user. If there will be no improvement moving from bluesky to scanspec, then there seems to be no reason to change.

Is this a hard requirement?

No, I only implemented the plans this way (implicit scan type, familiar for GDA users) as a comparison to #1702 (explicit scan type, familiar to bluesky users). If there is a consensus to shift from the implicit GDA style to an explicit bluesky style (and we believe this would benefit scientists and users in the future), I am happy to make the swap.

[fajinyuan]

Sorry for comments so late due to holiday leaves. I support @DominicOram suggestion that all scans should be explicitly named so its behaviour doesn't depend on the number of parameters in the input - I regard this feature in GDA scan is a bad design as users need to read doc or use help to discover what each scan syntax does. in the new system we should make these explicitly and then enforce the number of parameters checking for each command in command pre-processor or parser.

[EmsArnold]

@DominicOram , I have swapped the plans from implicit to explicit for ...scan and ...grid_scan, and added comments to note future intention to migrate to using scanspec for num_scan and step_scan type scans. As far as I can tell, scanspec doesn't have a spec similar to list_scan (short of concatenating a series of Static specs?). As such, there's currently no intention to migrate list_scan type plans to scanspec.

[olliesilvester]

Check about dicts being unordered when sending json

[EmsArnold]

All dictionaries changed to lists of tuples which will retain order when sending json.

[oliwenmandiamond]

Are we also able to address this issue here #1983 as I think the movable command syntax and docs needs addressing

[Relm-Arrowny]

I have two questions/comments and I'm in agreement with @oliwenmandiamond. First, can we avoid changing the function name? If it’s called abs_set in Bluesky, keeping it the same would be better. Second, can we keep the default values consistent with the original Bluesky plan? This would reduce a lot of confusion and meet the expectations of users who expect the wrapper function to behave exactly like the underlying one.

[oliwenmandiamond]

Yes, having the plan names be the same as the ones their wrapped and syntax be the same would help a lot with the confusion

[EmsArnold]

Are we also able to address this issue here #1983 as I think the movable command syntax and docs needs addressing

From my understanding, you should be able to accomplish multiple moves using:

>>> plans.move({device1: position1, device2: position2, ...})

I also believe the choice to use a Mapping[Movable[T], T] was in the name of serialising and typing for blueapi, though anyone is welcome to shed some more light on what blueapi requires. After some testing using *args, blueapi didn't seem to be reading the different arguments correctly. I've written this up into a ticket on blueapi.

Yes, having the plan names be the same as the ones their wrapped and syntax be the same would help a lot with the confusion

There was a discussion in a previous Athena Drop-in, where these differences were discussed. The overall feeling seemed to be that if you're asking for a set or a move, you'll likely want to wait for it (thus the change in default wait behaviour). The difference in name then acted as a mechanism to differentiate the wrapped plan from the default behaviour of the bluesky plan.

Perhaps this is a question to whether these should be called "wrapped" if the behaviour is not replicated?

[oliwenmandiamond]

I support making the default behaviour is to wait=True which differs from Bluesky. However, I think the name change and syntax differences is confusing when actually using these.

BlueAPI only needs type hints for plans. It fails when trying to use bluesky plan_stubs because kwargs has no type hint.

@plan
def mv(
    *args: Union[Movable, Any],
    group: Optional[Hashable] = None,
    timeout: Optional[float] = None,
    **kwargs,
) -> MsgGenerator[tuple[Status, ...]]: 

So the simplest solution is to remove the unhinted kwargs as I believe we don't support it

@plan
def mv(
    *args: Movable | Any,
    group: Hashable | None = None,
    timeout: float | None = None,
) -> MsgGenerator[tuple[Status, ...]]:

This way you can keep the same syntax as bluesky (BlueapiClient example below).

#Move single device
>>> plans.mv(devices.shutter1, "In")
# Move multiple devices
>>> plans.mv(devices.shutter1, "In", devices.shutter2, "In", ...)

The other issue I have with using the wrapped commands is that they much longer, I can already see when presenting this to PBS that they won't like it due to how long the commands are. No one wants to type out on command line fully set_relative vs rel_set.

[oliwenmandiamond]

Okay, I didn't realise BlueAPI doesn't support generic *args. I think this needs to be clarified

[Relm-Arrowny]

There was a discussion in a previous Athena Drop-in, where these differences were discussed. The overall feeling seemed to be that if you're asking for a set or a move, you'll likely want to wait for it (thus the change in default wait behaviour). The difference in name then acted as a mechanism to differentiate the wrapped plan from the default behaviour of the bluesky plan.

Perhaps this is a question to whether these should be called "wrapped" if the behaviour is not replicated?

I am 95% sure mv is default wait and _set is default not wait, are we removing this distintion in blueapi?

[EmsArnold]

I am 95% sure mv is default wait and _set is default not wait, are we removing this distintion in blueapi?

I believe this is correct - bps.mv should wait by default, and bps.abs_set should not block at all by default. There's nothing stopping these stubs being used as-is in other plans.

I had thought that these wrapped plans will likely be used heavily in a scripting environment (or through the CLI or somehow with the future DSL). In most of these cases, you'll be looking for sets/moves to wait until the set/move has been completed (i.e. you've got a script which moves a motor, takes a count, moves a motor, takes a count...). Or perhaps you're aligning a component - you'd only want to start your data collection once you reach the position/value you're meant to be at.

Perhaps @RJCD-Diamond and @DominicOram can expand as well, as they were also part of this conversation.