Skip to content
This repository was archived by the owner on Sep 2, 2024. It is now read-only.

951 activate callbacks based on plan#962

Merged
d-perl merged 12 commits into
mainfrom
951_activate_callbacks_based_on_plan
Nov 20, 2023
Merged

951 activate callbacks based on plan#962
d-perl merged 12 commits into
mainfrom
951_activate_callbacks_based_on_plan

Conversation

@d-perl

@d-perl d-perl commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

Fixes #951

Adds a class in between our callbacks and CallbackBase which adds functions gated by an activity flag which is set and unset on receiving appropriate documents from the RunEngine

To test:

  1. Run tests

@codecov

codecov Bot commented Nov 8, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3ad94ff) 93.48% compared to head (bdb89d4) 93.53%.

Files Patch % Lines
...al_interaction/callbacks/plan_reactive_callback.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #962      +/-   ##
==========================================
+ Coverage   93.48%   93.53%   +0.05%     
==========================================
  Files          54       55       +1     
  Lines        2623     2660      +37     
==========================================
+ Hits         2452     2488      +36     
- Misses        171      172       +1     

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

from event_model.documents.event import Event

if TYPE_CHECKING:
from event_model.documents.event import Event

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is imported twice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks

self.activity_uid = 0
return self.activity_gated_stop(doc) if do_stop else doc

def activity_gated_start(self, doc: RunStart):

@olliesilvester olliesilvester Nov 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to enforce implementation of these functions for subclasses of PlanReactiveCallback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, a lot of callbacks don't use all the methods

from event_model.documents.run_stop import RunStop


class PlanReactiveCallback(CallbackBase):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be useful to have a brief comment explaining that this should be used when we only want callbacks while a plan is running

@olliesilvester olliesilvester left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks neat! There's a double import I've mentioned on the comments, but it looks like everything here should work okay.

@d-perl
d-perl merged commit d17f9ea into main Nov 20, 2023
@d-perl
d-perl deleted the 951_activate_callbacks_based_on_plan branch November 20, 2023 15:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trigger callbacks based on the plan type

2 participants