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

Use is_plan from bluesky utils #306

Merged
merged 6 commits into from
Sep 27, 2024
Merged

Use is_plan from bluesky utils #306

merged 6 commits into from
Sep 27, 2024

Conversation

jwlodek
Copy link
Member

@jwlodek jwlodek commented Sep 7, 2024

Description

Switch to use new is_plan function from bluesky utils.

Motivation and Context

Solves #305 . Requires bluesky/bluesky#1780 to be released, and pinned in the requirements file.

Summary of Changes for Release Notes

Remove is_plan function, use instead the equivalent from bluesky utils that can handle the decorated functions.

Fixed

Bluesky builtin plans that use the @plan decorator are now correctly identified by queueserver.

@dmgav
Copy link
Contributor

dmgav commented Sep 9, 2024

In my opinion, the internally defined function is_plan should not be deleted, but instead modified to call the function defined in bluesky. We also need to support older versions of Bluesky that don't have is_plan function by either checking the version of bluesky or using internal implementation if import fails. Also, bluesky is optional dependency, so is_plan should be imported inside the is_plan function. I would also be useful to add some tests to make sure the plans are actually recognized in the future.

@tacaswell
Copy link
Collaborator

I agree with @dmgav , this should be done as

try:
    from bluesky.utils import is_plan
except ImportError:
    def is_plan(...): ...

or we need to pin the version of bluesky qs depends on to be the first version that has is_plan (which is too aggressive in my view).

@dmgav dmgav merged commit 0db1a63 into bluesky:main Sep 27, 2024
35 checks passed
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.

3 participants