595 write to ispyb sample status after collection #625

rtuck99 · 2024-11-07T15:03:12Z

Fixes #595
Requires #606

The blSampleStatus field in ispyb will now be updated on successful robot load to LOADED, when the load_centre_collect plan is used.

In the event of the pin not being found or no diffraction, blSampleStatus will be updated to ERROR - sample to indicate an issue with the sample
If any other error occurs, blSampleStatus will be updated to ERROR - beamline to indicate a general problem with the beamline.

On normal completion the blSampleStatus will be left at LOADED

codecov · 2024-11-07T15:58:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.50%. Comparing base (89e436f) to head (679f338).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #625      +/-   ##
==========================================
+ Coverage   85.31%   85.50%   +0.19%     
==========================================
  Files         101      101              
  Lines        6726     6816      +90     
==========================================
+ Hits         5738     5828      +90     
  Misses        988      988

Components	Coverage Δ
i24 SSX	`69.57% <ø> (ø)`
hyperion	`96.85% <100.00%> (+0.07%)`	⬆️
other	`95.16% <100.00%> (+0.03%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

DominicOram

Great, thanks. Really like the additional system tests! I think we could have a few more unit tests alongside them to test more of the code paths though

DominicOram · 2024-11-11T12:53:58Z

src/mx_bluesky/hyperion/experiment_plans/oav_grid_detection_plan.py

@@ -105,8 +108,13 @@ def grid_detection_plan(
        # See #673 for improvements
        yield from bps.sleep(CONST.HARDWARE.OAV_REFRESH_DELAY)

-        tip_x_px, tip_y_px = yield from catch_exception_and_warn(


Should: With this removed catch_exception_and_warn is never used other than here, can we just re purpose it to raise a SampleException rather than WarningException then keep the call here?

Alternatively we can remove catch_exception_and_warn as it's no longer needed but I like having this behind a function as it's a bit confusing at first glance.

DominicOram · 2024-11-11T12:56:03Z

src/mx_bluesky/hyperion/experiment_plans/load_centre_collect_full_plan.py

+    # Below causes close_run not received before open_run in _open_run
+    # @set_run_key_decorator(CONST.PLAN.LOAD_CENTRE_COLLECT)
+    # @run_decorator(md={"metadata": {"sample_id": params.sample_id},
+    #                    "activate_callbacks": ["SampleHandlingCallback"]})
+    # @sample_handling_callback_decorator()


Should: This commented out code is exactly what is written below? What is the comment trying to tell me?

I think this is something I added as a note whilst I was trying to figure out what the hell was going on wrt run key decorator, I will remove it. The problem was caused elsewhere in the code.

DominicOram · 2024-11-11T13:01:02Z

src/mx_bluesky/hyperion/experiment_plans/oav_grid_detection_plan.py

-            PinNotFoundException, wait_for_tip_to_be_found, pin_tip_detection
+        def wrap_exception(e: Exception) -> Generator[Msg, Any, Any]:
+            if isinstance(e, PinNotFoundException):
+                raise SampleException(str(e))


Should:

Suggested change

raise SampleException(str(e))

raise SampleException from e

will give better chaining information

DominicOram · 2024-11-11T13:09:28Z

..._bluesky/hyperion/external_interaction/callbacks/sample_handling/sample_handling_callback.py

+
+
+class SampleHandlingCallback(PlanReactiveCallback):
+    """Intercepts"""


Should: Can we have a nicer docstring please?

DominicOram · 2024-11-11T14:01:49Z

src/mx_bluesky/hyperion/external_interaction/callbacks/robot_load/ispyb_callback.py

+            if not exit_status:
+                reason = "Exit status not available in stop document!"
+            elif not self._metadata:
+                reason = "Metadata not received before stop document."
+            else:
+                reason = doc.get("reason") or "OK"


Should: I agree with codcov that some unit tests around this would be good.

I don't think these lines are reachable in practice so I'm replacing them with asserts

DominicOram · 2024-11-11T14:07:48Z

src/mx_bluesky/hyperion/external_interaction/ispyb/exp_eye_store.py

+    bl_sample_status: str | None
+
+
+class ExpeyeSampleHandlingInteraction:


Should: I think some unit tests on this would be good, even if they're just smoke tests really

DominicOram · 2024-11-11T14:10:55Z

..._bluesky/hyperion/external_interaction/callbacks/sample_handling/sample_handling_callback.py

+    def _record_exception(self, exception_type: str):
+        expeye = ExpeyeSampleHandlingInteraction()
+        assert self._sample_id, "Unable to record exception due to no sample ID"
+        sample_status = self._decode_sample_status(exception_type)
+        expeye.update_sample_status(self._sample_id, sample_status)
+
+    def _decode_sample_status(self, exception_type: str) -> BLSampleStatus:


Should: Would be good to have a couple of tests covering these functions

DominicOram · 2024-11-11T14:12:20Z

..._bluesky/hyperion/external_interaction/callbacks/sample_handling/sample_handling_callback.py

+sample_handling_callback_decorator = make_decorator(
+    partial(contingency_wrapper, except_plan=_exception_interceptor)
+)


👍 I like this, does a good job of hiding the shenanigans

DominicOram · 2024-11-11T14:19:32Z

tests/system_tests/hyperion/external_interaction/conftest.py

+        set_mock_value(
+            ophyd_pin_tip_detection.triggered_top_edge,
+            numpy.array([]),
+        )
+
+        set_mock_value(
+            ophyd_pin_tip_detection.triggered_bottom_edge,
+            numpy.array([]),
+        )


Nit: We don't nee to set the edges to [] here for the pin tip to not be found

DominicOram · 2024-11-11T14:28:26Z

tests/system_tests/hyperion/external_interaction/test_load_centre_collect_full_plan.py

+
+
+@pytest.fixture(autouse=True)
+def use_real_ispyb():


Nit:

Suggested change

def use_real_ispyb():

def use_dev_ispyb():

DominicOram · 2024-11-11T16:15:21Z

I have also just been told about #636. I think it probably makes sense to assume the new endpoints for this issue

…un()

…ack.py. Add SampleExceptionStatus

… of run-key decorators/wrappers

rtuck99 · 2024-11-25T12:57:40Z

I have also just been told about #636. I think it probably makes sense to assume the new endpoints for this issue

Code has already been updated for the new endpoints

rtuck99 requested review from DominicOram, dperl-dls and olliesilvester November 7, 2024 15:55

DominicOram mentioned this pull request Nov 11, 2024

Fix system tests which don't require S03 #632

Open

DominicOram requested changes Nov 11, 2024

View reviewed changes

rtuck99 force-pushed the lightweight-events branch from 7b43714 to 1ff6b78 Compare November 14, 2024 16:32

rtuck99 force-pushed the 595_write_to_ispyb_sample_status_after_collection branch from ccf3dcf to bd50284 Compare November 14, 2024 16:33

DominicOram mentioned this pull request Nov 19, 2024

Implementation of simple lightweight event base class #606

Merged

rtuck99 force-pushed the lightweight-events branch from 1ff6b78 to 3907833 Compare November 21, 2024 15:32

rtuck99 force-pushed the 595_write_to_ispyb_sample_status_after_collection branch from bd50284 to 8e4d49c Compare November 22, 2024 10:53

rtuck99 force-pushed the lightweight-events branch from dde3045 to 5cfe52c Compare November 22, 2024 15:04

Base automatically changed from lightweight-events to main November 22, 2024 15:34

rtuck99 added 16 commits November 22, 2024 16:07

Robot load exp-eye callback for sample status

8ed7376

Sample handling callbacks WIP

78d3e70

Ultimately fruitless attempt to intercept the exception using close_r…

8733634

…un()

Implement event-based exception interception in sample_handling_callb…

9853817

…ack.py. Add SampleExceptionStatus

Register the SampleHandlingCallback

bef734a

Fix broken end_load() in exp_ey_store.py

5131444

Make BLSampleStatus enums shorter

5c0850b

Add system tests for BLSampleStatus and also fixup inconsistent usage…

5510181

… of run-key decorators/wrappers

Dedup oa_patch_motor()

1812a21

Increase motor limits in system test

4f3521b

Fix system tests to work with new OAV snapshots

2e2d2e4

Additional system tests, implement sample status for crystal not found

64dff0f

Make type checking happy

4a51117

Make ruff happy

e9138ec

Revert motor limits fixture changes and fix system tests

d616325

Remove now-superfluous endpoint-suffix

12a95a5

rtuck99 added 5 commits November 22, 2024 16:12

Response to PR comments WIP

bfa61da

Fix exception chaining message for SampleException

d95a91c

Responses to PR review comments

9ed5950

Enhance coverage of sample_handling_callback unit tests

78eea63

Fix circular import dependencies

679f338

rtuck99 force-pushed the 595_write_to_ispyb_sample_status_after_collection branch from b5da656 to 679f338 Compare November 22, 2024 16:27

rtuck99 requested a review from DominicOram November 22, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

595 write to ispyb sample status after collection #625

595 write to ispyb sample status after collection #625

rtuck99 commented Nov 7, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

DominicOram left a comment

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

rtuck99 Nov 21, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

rtuck99 Nov 22, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram commented Nov 11, 2024

rtuck99 commented Nov 25, 2024



		class SampleHandlingCallback(PlanReactiveCallback):
		"""Intercepts"""

		bl_sample_status: str \| None


		class ExpeyeSampleHandlingInteraction:

595 write to ispyb sample status after collection #625

Are you sure you want to change the base?

595 write to ispyb sample status after collection #625

Conversation

rtuck99 commented Nov 7, 2024

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented Nov 11, 2024

rtuck99 commented Nov 25, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading