Add prepare command to RunEngine and bundler, as well as protocol

[rosesyrett]

resolves #1637

[rosesyrett]

I've got some questions after trying to implement this... I looked at existing protocols like kickoff/complete and used those as templates to try doing prepare.

1. Why does the RunBundler have an empty complete() method? I added an empty prepare() method just to keep to this convention.
2. Should it be enforced that prepare is only called when a run is open? I've assumed so, because presumably the function of prepare() is to prepare for a flyscan or something similar.
3. To me the protocols are a bit confusing in regards to if something is sync or async. e.g. a Flyable has two methods, kickoff and complete which are marked as regular sync functions. But in the run engine, they are called differently; one is called using maybe_await and the other is just called normally. Why do we do this? Would it be better to call maybe_await on every protocol method and allow any of them to be async or sync, or should we define in the protocols themselves if they ought to be sync or async? Is this a limitation of python's async logic, protocols, or something else I haven't considered? Either way, for the moment I've assumed that Preparable has a prepare method that should be async. But maybe this is not the way to go, especially as set is actually sync...

I've also noticed a few things I've noticed in the codebase which I think, either I don't understand fully, or perhaps could be 'cleaned up':

1. done_callback is an inner function defined in 4 other places in the RunEngine. Seems like unnecessary code duplication.
2. It seems like we never use the RunBundler before open_run is called on it. In this case, why don't we just make the open_run method part of the constructor logic, and the close_run method part of the destructor? This would make things a lot simpler in terms of checking if a run is open. For example at the moment, if you send a collect message to the RunEngine it first checks if self._run_bundlers contains a run_key entry. But then, it calls the .collect method in the associated run bundler, which checks if the run is open. In both cases, failing these checks raises the same error. This seems unnecessary and a bit "smelly" to me.
3. Not sure I like exceptions being used as logic blocks. I know python lets you do this but I think it's bad practise. A small example is RunEngine._read, which checks if self._run_bundlers has the message run_key. If it does, it awaits the current_run.read method, otherwise it does nothing. This is written in a try/except block for some reason instead of a simple if/else, which would use up less vertical space and be more readable in general. Another example is, for backwards compatibility, a try/except block is present in lots of methods in the RunEngine to change how callbacks are added to statuses. Surely there is a way to ensure this behaviour without the code duplication this method uses, and without using try/except.
4. I've just noticed that RunEngine._configure tries to call .configure on the object passed in the message, but there is no 'Configurable' protocol which checks that it will have this method. Seems like there ought to be (although there is a Configurable protocol in protocols.py, but it seems to do something else).

I realise this is probably a massive tangent, but this is one of the few times I've touched anything in the bluesky repo so I have lots of questions. Perhaps I should make some separate issues that can be addressed as part of a bluesky hackathon or something similar, but to me as someone who doesn't often touch the codebase there seems to be scope for clean up tasks of the repo, which also includes formatting it properly maybe with tools like ruff. I'd be interested in people's thoughts on this.

Edit: perhaps we should have a 'devsug' label for some github issues, to indicate a developer suggestion for this type of thing?

[untzag]

One idea, as I recall it, was that methods that return Status were to remain strictly synchronous on the protocol level. The Status pattern can be implemented using threads or the event loop, either way is easy to write to when making interfaces. Just my memory!

[untzag]

@runtime_checkable
class Preparable(Protocol):
    @abstractmethod
    def prepare(self, value) -> Status:
        """Prepare a device for fly scanning, i.e. configure the correct triggers."""
        ...

Could you say more about how the returned Status object is supposed to work? I was imagining that prepare would be instantaneous from an order-of-operations point of view. Do you imagine touching the hardware during this method call?

[coretl]

Could you say more about how the returned Status object is supposed to work? I was imagining that prepare would be instantaneous from an order-of-operations point of view. Do you imagine touching the hardware during this method call?

After the conversation with @prjemian and @danielballan we settled on this scheme for flyers:

• We need to prepare(flyscan_params) so the motors can move to the start
• Then we can kickoff() which returns when the motors have started moving
• Then we can complete() which returns when the motors have stopped moving

In this case, we are moving motors which could take a reasonable amount of time

The same could be said for a detector:

• We prepare(frame_params) to setup N frames in hardware triggered mode
• Then we can kickoff() which returns when the detector is armed
• Then we can complete() which returns when the detector has taken the correct number of frames
• We collect_asset_docs() repeatedly while complete is not done to publish frames

We also thought that this could also be useful for step scanning the same detectors:

• We prepare(frame_params) to setup N frames in software triggered mode
• We trigger() to take N frames
• We collect_asset_docs() to publish those frames

[untzag]

Thank you @coretl. Could you also comment on how prepare is similar or different from stage?

[coretl]

1. Why does the RunBundler have an empty complete() method? I added an empty prepare() method just to keep to this convention.

Question for @tacaswell

2. Should it be enforced that prepare is only called when a run is open? I've assumed so, because presumably the function of prepare() is to prepare for a flyscan or something similar.

Question for @tacaswell

3. To me the protocols are a bit confusing in regards to if something is sync or async. e.g. a `Flyable` has two methods, `kickoff` and `complete` which are marked as regular sync functions. But in the run engine, they are called differently; one is called using `maybe_await` and the other is just called normally. Why do we do this? Would it be better to call `maybe_await` on every protocol method and allow any of them to be async or sync, or should we define in the protocols themselves if they ought to be sync or async? Is this a limitation of python's async logic, protocols, or something else I haven't considered? Either way, for the moment I've assumed that `Preparable` has a `prepare` method that should be async. But maybe this is not the way to go, especially as `set` is actually sync...

This is for historic reasons, but is actually defendable:

• SyncOrAsync methods should return quickly, they may do I/O but never need to have progress tracking, and can return values
  • e.g. read(), describe(), pause(), resume(), stop(), collect()
• Sync methods returning a Status can start a long running operation, and should provide a Status object so the RE can do progress tracking, and cannot return values
  • e.g. kickoff(), complete(), trigger(), set()

I would argue that prepare() is in the latter category

I've also noticed a few things I've noticed in the codebase which I think, either I don't understand fully, or perhaps could be 'cleaned up':

1. `done_callback` is an inner function defined in 4 other places in the `RunEngine`. Seems like unnecessary code duplication.

That seems like a good target to clean up

2. It seems like we never use the `RunBundler` before `open_run` is called on it. In this case, why don't we just make the `open_run` method part of the constructor logic, and the `close_run` method part of the destructor? This would make things a lot simpler in terms of checking if a run is open. For example at the moment, if you send a `collect` message to the `RunEngine` it first checks if `self._run_bundlers` contains a `run_key` entry. But then, it calls the `.collect` method in the associated run bundler, which checks if the run is open. In both cases, failing these checks raises the same error. This seems unnecessary and a bit "smelly" to me.

Question for @tacaswell

3. Not sure I like exceptions being used as logic blocks. I know python lets you do this but I think it's bad practise. A small example is `RunEngine._read`, which checks if `self._run_bundlers` has the message `run_key`. If it does, it awaits the `current_run.read` method, otherwise it does nothing. This is written in  a try/except block for some reason instead of a simple if/else, which would use up less vertical space and be more readable in general. Another example is, for backwards compatibility, a try/except block is present in lots of methods in the RunEngine to change how callbacks are added to statuses. Surely there is a way to ensure this behaviour without the code duplication this method uses, and without using try/except.

Question for @tacaswell

4. I've just noticed that `RunEngine._configure` tries to call `.configure` on the object passed in the message, but there is no 'Configurable' protocol which checks that it will have this method. Seems like there ought to be (although there is a `Configurable` protocol in `protocols.py`, but it seems to do something else).

This is because the configure() method meant "reconfigure your device, invalidating the read_configuration cache", but this wasn't enforceable at the plan or Device level as they can be reconfigured by anything. Instead of solidifying into a protocol, it is there for back-compat only. Configurable now means "you can read its configuration"

I realise this is probably a massive tangent, but this is one of the few times I've touched anything in the bluesky repo so I have lots of questions. Perhaps I should make some separate issues that can be addressed as part of a bluesky hackathon or something similar, but to me as someone who doesn't often touch the codebase there seems to be scope for clean up tasks of the repo, which also includes formatting it properly maybe with tools like ruff. I'd be interested in people's thoughts on this.

This will be done, but not yet. We want to finish the copier project internally first and check that is the right tool for applying to the rest of the repos.

[coretl]

Thank you @coretl. Could you also comment on how prepare is similar or different from stage?

• stage() means "I'm going to be used in a scan, not sure what type yet". I generally use this to get it into a known idle state, then open files for taking data
• prepare(params) means "I'm about to do a step or fly scan with these parameters". I use it for moving motors or setting parameters on detectors

[rosesyrett]

just realised I haven't added a test to this; I'll have a look at doing this tomorrow morning so this can be merged relatively soonish

[rosesyrett]

I've now written a test for this, for the flyscan case. Since the call yesterday I've removed prepare from the RunBundler which means I no longer need to get the current_run anymore within RunEngine.prepare. Does that mean that we want prepare to be like set, i.e. it can be called outside of a run? Or do we want to enforce it's always called within a run? I've assumed the latter.

Personally I think it makes sense to enforce that prepare should be called inside of a run. But I found this interesting..

I'm going to create a GH issue about complete as well, which (as discussed) has an empty method in the RunBundler. Presumably for this use case, we would no longer need the complete method in the RunEngine to get the current_run anymore either. Yet presumably we want to enforce that complete only gets called when a run is open, and after kickoff.

[danielballan]

A set is something you might do in preparation for a series of Runs, so it can make sense to call outside of any given Run. If it had to be inside a Run, then the first Run in a batch might have to fold in special "setup" logic, which feels wrong.

But prepare is, in my understanding, a setup for a single use---e.g. a single flyscan. It applies settings that are not meant to be reused across multiple acquisitions, so it makes sense to enforce it being inside a Run.

[danielballan]

I'll leave it to @coretl to merge if ready.

[tacaswell]

Why does the RunBundler have an empty complete() method?

See discussion in #1644

Should it be enforced that prepare is only called when a run is open

I can see arguments both ways, but lean to leaving it allowed outside runs as it is about moving stuff around (e.g. we don't require a run to be open to stage) not about collecting and collating data. I suspect in practice it will mostly be used inside of an open_run but I don't see why not doing that is wrong.

It seems like we never use the RunBundler before open_run...

See discussion in #1642

Not sure I like exceptions being used as logic blocks.

See discussion in #1646

done_callback is an inner function defined in 4 other places in the `RunEngine

I think they are mostly relying on closures of values from the local scope to work. Could probably be factored into a factory function or functools.partial .

[tacaswell]

Also looks good to me, but if Dan is deferring to @coretl to merge so will I.

[coretl]

Please could we include this in hardware.rst, maybe somewhere around

bluesky/docs/source/hardware.rst

Line 258 in 9b0d729

? Then I'm happy with this