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

Let GDA write to ISPyB sample status #740

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

shihab-dls
Copy link
Collaborator

@shihab-dls shihab-dls commented Jan 10, 2025

Addresses #604
Dummy plans are provided to allow GDA to trigger error or loaded status in blsamplestatus.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.98%. Comparing base (eac39cb) to head (c7dbe82).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #740      +/-   ##
==========================================
+ Coverage   86.97%   86.98%   +0.01%     
==========================================
  Files         101      102       +1     
  Lines        6933     6963      +30     
==========================================
+ Hits         6030     6057      +27     
- Misses        903      906       +3     
Components Coverage Δ
i24 SSX 72.69% <ø> (ø)
hyperion 96.50% <90.90%> (-0.03%) ⬇️
other 96.51% <90.90%> (-0.08%) ⬇️

@olliesilvester
Copy link
Contributor

I think you meant to tag #604 instead in the description?

Copy link
Contributor

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

I think the deposit_sample_error plan you made does correctly do what we want, however, we've also slightly changed the behaviour of the sample_handling_callback which is used in other plans (load_centre_collect). This means it will update the sample status to 'loaded' at the end of the full load_centre_collect plan.

Perhaps we could remove the _record_loaded feature of SampleHandlingCallback, and instead subclass SampleHandlingCallback, then overload activity_gated_stop to include _record_loaded + call the super activity_gated_stop. Then we'd have a callback which does the right stuff and isn't used by our UDC plans



def create_devices() -> None:
"""Create the necessary devices for the plan."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Comment is a bit generic, could say something about this plan not requiring any devices

@olliesilvester
Copy link
Contributor

Also pls can you link the gerrit change you made here?

@shihab-dls
Copy link
Collaborator Author

Here's the Gerrit PR

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