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

Add ability to pre-declare a stream #1542

Merged
merged 30 commits into from
Apr 18, 2023
Merged

Conversation

tacaswell
Copy link
Contributor

@tacaswell tacaswell commented Aug 23, 2022

In discussions with PSI (SLS2) at NSLS-II today it became clear that we need the ability to pre-declare what the streams will be before we create the first event. This will have at least three and a half major benefits

  1. it makes the first measurement less special as we can ensure that the describe / read_configuration / describe_configuration happen when we think they wil
  2. they will enable the bundler to live in a different process and still correctly accumulate all of the notifications it will need
  3. it is now possible to get the descriptor is a custom plan without subscribing to the output stream
  4. the RunBundler class is now pluggable by class level monkey patching or thin sub-classing (could be convinced to make it init time configurable...)

In local testing I added a "strict mode" flag to demand that streams are pre-declared and many tests still passed.

@danielballan danielballan changed the title Add ability do pre-declare a stream Add ability to pre-declare a stream Aug 24, 2022
@tacaswell
Copy link
Contributor Author

Open questions:

  • do we want this?
  • how to manage "strict mode" configuration (probably want 3 modes: YOLO, warn, strict)
  • how to thread the stream name through count and the scan_nd family of plans because they take an arbitrary user function than can use a different stream name that 'primary' ?
  • how configurable do we want to make the RunBundler class on the RE?

@danielballan
Copy link
Member

I think we do want it.

I think we should start with "YOLO" mode only and get some experience with this before we add either "warn" or "strict".

I think that we can defer doing anything helpful or clever with per_step. If anything but the default per_step is used, count and scan_nd can drop the declaration. Users who want both a declaration and a custom per_step have to write a custom plan. As we get experience with this we can consider adding another parameter to deal with this.

I think class attribute is a fine place to start.

extra={'doc_name': 'descriptor',
'run_uid': self._run_start_uid,
'data_keys': data_keys.keys()})
await self._cache_describe(obj)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The describe cache from read() was previously used here:

Suggested change
await self._cache_describe(obj)
if obj not in self._describe_cache:
await self._cache_describe(obj)

Similar for the read_configuration() and describe_configuration(). Is this refresh of the cache on monitor purposeful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this and centralized most of this into one place.

Could not fully inline the helpers because collect also uses some of them. Will leave that refactor for the future.

@coretl
Copy link
Collaborator

coretl commented Aug 30, 2022

Also, what does "YOLO" mean?

@tacaswell
Copy link
Contributor Author

YOLO: You Only Live Once

@tacaswell
Copy link
Contributor Author

It looks like changes to ophyd broke some of the tests?

@coretl
Copy link
Collaborator

coretl commented Aug 30, 2022

I'm guessing bluesky/ophyd#1036 might have something to do with it? @callumforrester do you recognise the error?

@cjtitus
Copy link
Contributor

cjtitus commented Jan 12, 2023

I'm interested in getting this merged. I can try to look at why tests were failing, but as the logs have disappeared it would be helpful to re-run the tests first.

I don't think I can tell GH to re-run the tests, but someone has the power to do that, right?

@danielballan
Copy link
Member

Power-cycled to re-run CI

@cjtitus
Copy link
Contributor

cjtitus commented Jan 12, 2023

Well it looks like turning it off and then back on has "fixed" the previously failing tests. Were the previously open questions ever answered satisfactorily?

Open questions:

* do we want this?

* how to manage "strict mode" configuration (probably want 3 modes: YOLO, warn, strict)

* how to thread the stream name through `count` and the `scan_nd` family of plans because they take an arbitrary user function than can use a different stream name that 'primary' ?

* how configurable do we want to make the RunBundler class on the RE?

I can at least answer the first question and say yes, I want this.
The rest of these were addressed by @danielballan

In order to manage count and scan_nd I think the existing code needs to be changed to avoid pre-declaring a stream if a non-default per_shot is passed in. There is already a check for if per_shot is None to assign the default. This could be when the stream pre-declaration is toggled.

@cjtitus
Copy link
Contributor

cjtitus commented Jan 13, 2023

Another important question is if strict checking option should be left in RunBundler, but False by default, or if it should be removed entirely for now. If it's False, there's really no way to ever turn it on, unless an option for strict checking were to be added to the RunEngine that could be passed to RunBundler instantiation during open_run

Maybe this is a plausible enough future addition that the code for strict checking ought to be left in?

@tacaswell
Copy link
Contributor Author

Thank you @cjtitus for pushing on this!

One change I would like to make is to change the strict_pre_declare to an enum rather than a bool (with values "strict", "warn", and "permissive" and change the parameter to "stream_pre_declare_policy" or something like that), but it is not a disaster if it goes in as a bool (and we can promote later it is just annoying to do so).

@tacaswell tacaswell marked this pull request as ready for review January 17, 2023 17:54
tacaswell and others added 18 commits April 6, 2023 00:12
When unifying the descriptor work with monitors we picked up adding the object
name to the data keys in the descriptors from monitor consistent with other
descriptors.
…eclaration. Added test that creating a stream (i.e, in a custom per_step) without pre-declaring fails in this case. Note that turning on strict checking functionally precludes passing in per_step
…es primary, causes strict checking of pre-declaration to fail
@tacaswell
Copy link
Contributor Author

@coretl I think this is ready to go modulo:

  • do we want a boolean or a tri-state of the strictness
  • public API to make the RE strict

@coretl coretl merged commit 5dc91e4 into bluesky:master Apr 18, 2023
8 checks passed
@tacaswell tacaswell deleted the enh_declare_stream branch April 18, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants