From b3b381e312164dd72410be18aa3b7bd73ed25a3b Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Thu, 20 Feb 2025 14:40:16 +0000 Subject: [PATCH 1/9] Add test to ensure that we unstage before we trigger zocalo --- .../test_multi_rotation_scan_plan.py | 64 +++++++++++++++++-- 1 file changed, 59 insertions(+), 5 deletions(-) diff --git a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py index 55db3657d..bc3c869de 100644 --- a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py +++ b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py @@ -6,7 +6,7 @@ from itertools import takewhile from math import ceil from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import h5py import numpy as np @@ -18,13 +18,22 @@ from ophyd.status import Status from ophyd_async.testing import set_mock_value -from mx_bluesky.common.external_interaction.ispyb.ispyb_store import StoreInIspyb +from mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback import ( + ZocaloCallback, +) +from mx_bluesky.common.external_interaction.ispyb.ispyb_store import ( + IspybIds, + StoreInIspyb, +) from mx_bluesky.common.external_interaction.nexus.nexus_utils import AxisDirection from mx_bluesky.hyperion.experiment_plans.rotation_scan_plan import ( RotationScanComposite, calculate_motion_profile, multi_rotation_scan, ) +from mx_bluesky.hyperion.external_interaction.callbacks.__main__ import ( + create_rotation_callbacks, +) from mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback import ( RotationISPyBCallback, ) @@ -249,14 +258,14 @@ def test_full_multi_rotation_plan_nexus_writer_called_correctly( ) nexus_writer_calls = mock_nexus_writer.call_args_list first_run_number = test_multi_rotation_params.detector_params.run_number - for call, rotation_params in zip( + for writer_call, rotation_params in zip( nexus_writer_calls, test_multi_rotation_params.single_rotation_scans, strict=False, ): - callback_params = call.args[0] + callback_params = writer_call.args[0] assert callback_params == rotation_params - assert call.kwargs == { + assert writer_call.kwargs == { "omega_start_deg": rotation_params.omega_start_deg, "chi_start_deg": rotation_params.chi_start_deg, "phi_start_deg": rotation_params.phi_start_deg, @@ -511,3 +520,48 @@ def test_full_multi_rotation_plan_arms_eiger_asynchronously_and_disarms( eiger.do_arm.set.assert_called_once() eiger.unstage.assert_called_once() + + +@patch( + "mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback.StoreInIspyb" +) +@patch( + "mx_bluesky.hyperion.experiment_plans.rotation_scan_plan.check_topup_and_wait_if_necessary", + autospec=True, +) +def test_zocalo_callback_end_only_gets_called_at_the_end_of_all_collections( + _, + mock_ispyb_store: MagicMock, + RE: RunEngine, + test_multi_rotation_params: MultiRotationScan, + fake_create_rotation_devices: RotationScanComposite, + oav_parameters_for_rotation: OAVParameters, +): + """We must unstage the detector before we trigger zocalo so that we're sure we've + finished writing data.""" + mock_ispyb_store.return_value = MagicMock(spec=StoreInIspyb) + mock_ispyb_store.return_value.begin_deposition.return_value = IspybIds( + data_collection_ids=(123,) + ) + eiger = fake_create_rotation_devices.eiger + parent_mock = MagicMock() + parent_mock.eiger = MagicMock(return_value=Status(done=True, success=True)) + eiger.unstage = parent_mock.eiger_unstage + callbacks = create_rotation_callbacks() + zocalo_callback = callbacks[1].emit_cb + assert isinstance(zocalo_callback, ZocaloCallback) + zocalo_callback.zocalo_interactor = MagicMock() + zocalo_callback.zocalo_interactor.run_end = parent_mock.run_end + + _run_multi_rotation_plan( + RE, + test_multi_rotation_params, + fake_create_rotation_devices, + callbacks, + oav_parameters_for_rotation, + ) + + assert parent_mock.method_calls.count(call.run_end(123)) == len( + test_multi_rotation_params.rotation_scans + ) + assert parent_mock.method_calls[0] == call.eiger_unstage From ed6a0bc758af97af8a42d10a45b47042254b86ed Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Thu, 20 Feb 2025 16:11:04 +0000 Subject: [PATCH 2/9] Add some docs around how callbacks are currently used --- .../explanations/callback_and_run_logic.rst | 23 +++++++++++++++++++ docs/developer/general/index.rst | 1 + 2 files changed, 24 insertions(+) create mode 100644 docs/developer/general/explanations/callback_and_run_logic.rst diff --git a/docs/developer/general/explanations/callback_and_run_logic.rst b/docs/developer/general/explanations/callback_and_run_logic.rst new file mode 100644 index 000000000..197031664 --- /dev/null +++ b/docs/developer/general/explanations/callback_and_run_logic.rst @@ -0,0 +1,23 @@ +Callbacks are used to trigger external services: +* Ispyb deposition +* Nexus writing +* Zocalo triggering + +These are linked in that to trigger zocalo you need to have made an ispyb deposition, written a nexus file and have finished writing raw data to disk. Nexus files and ispyb depositions can be made at anytime, we do not need to have necessarily finished writing raw data. + +Currently, the requirement of needing to have written to ispyb is explicit as the ispyb callback will emit to the zocalo callback. The nexus file is written when the hardware is read during a collection and so its ordering is implied. When instantiated the zocalo callback is told on which plan to trigger and it is up to the plan developer to make sure this plan finishes after data is written to the detector. + +In general, the ordering flow of when callbacks are triggered is controlled by emitting documents with the expected plan name and data. + +Rotation Scans +============== + +There are currently two ways of doing rotation scans. A single scan creates one hdf file, one ispyb deposition and then triggers zocalo once. Multi rotation scans create one hdf file for all rotations but then N nexus files, N ispyb depositions and triggers zocalo N times. + +For multi rotations this is does by starting 1+2*N different runs: + +1. `CONST.PLAN.ROTATION_MULTI`: This is emitted once for the whole multiple rotation. It is used by the nexus callback to get the full number of images and meta_data_run_number so that it knows which hdf file to use +2. `CONST.PLAN.ROTATION_OUTER`: Emitted N times, inside a `CONST.PLAN.ROTATION_MULTI` run. This is used to create the initial ispyb deposition and create the nexus writer (but not actually write the file) +3. `CONST.PLAN.ROTATION_MAIN`: Emitted N times, inside `CONST.PLAN.ROTATION_OUTER` run. Used to finish writing to ispyb (i.e. write success/failure) and to trigger zocalo (this is causing errors, see https://github.com/DiamondLightSource/mx-bluesky/issues/826) + +Single rotations only trigger 2 runs. One `CONST.PLAN.ROTATION_OUTER` and one `CONST.PLAN.ROTATION_MAIN`. Both of which do as above. diff --git a/docs/developer/general/index.rst b/docs/developer/general/index.rst index 221697518..689d2c365 100644 --- a/docs/developer/general/index.rst +++ b/docs/developer/general/index.rst @@ -41,6 +41,7 @@ Documentation is split into four categories, and each is also accessible from li :maxdepth: 1 explanations/decisions + explanations/callback_and_run_logic +++ From 747aa84397dee398d0dde38e297bc1d11e2aed57 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Fri, 21 Feb 2025 11:29:32 +0000 Subject: [PATCH 3/9] Fix test to stop writing nexus files --- .../hyperion/experiment_plans/test_multi_rotation_scan_plan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py index bc3c869de..c4a3334fd 100644 --- a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py +++ b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py @@ -557,7 +557,7 @@ def test_zocalo_callback_end_only_gets_called_at_the_end_of_all_collections( RE, test_multi_rotation_params, fake_create_rotation_devices, - callbacks, + [callbacks[1]], # Only add the ispyb callback otherwise we get nexus files oav_parameters_for_rotation, ) From 1baabe6807af008aea4ae4366706b2680df07f1d Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Fri, 21 Feb 2025 12:55:20 +0000 Subject: [PATCH 4/9] Zocalo callback will now trigger any plans that are started within the run --- .../explanations/callback_and_run_logic.rst | 8 +- .../callbacks/common/zocalo_callback.py | 33 ++--- .../callbacks/__main__.py | 2 +- .../callbacks/test_zocalo_handler.py | 81 ++++++----- .../test_multi_rotation_scan_plan.py | 127 +++++++++++++++++- .../callbacks/test_rotation_callbacks.py | 79 ----------- 6 files changed, 196 insertions(+), 134 deletions(-) diff --git a/docs/developer/general/explanations/callback_and_run_logic.rst b/docs/developer/general/explanations/callback_and_run_logic.rst index 197031664..12df7fe92 100644 --- a/docs/developer/general/explanations/callback_and_run_logic.rst +++ b/docs/developer/general/explanations/callback_and_run_logic.rst @@ -14,10 +14,10 @@ Rotation Scans There are currently two ways of doing rotation scans. A single scan creates one hdf file, one ispyb deposition and then triggers zocalo once. Multi rotation scans create one hdf file for all rotations but then N nexus files, N ispyb depositions and triggers zocalo N times. +Single scans will be removed in https://github.com/DiamondLightSource/mx-bluesky/issues/847 + For multi rotations this is does by starting 1+2*N different runs: -1. `CONST.PLAN.ROTATION_MULTI`: This is emitted once for the whole multiple rotation. It is used by the nexus callback to get the full number of images and meta_data_run_number so that it knows which hdf file to use +1. `CONST.PLAN.ROTATION_MULTI`: This is emitted once for the whole multiple rotation. It is used by the nexus callback to get the full number of images and meta_data_run_number so that it knows which hdf file to use. When this is finished zocalo end is triggered. 2. `CONST.PLAN.ROTATION_OUTER`: Emitted N times, inside a `CONST.PLAN.ROTATION_MULTI` run. This is used to create the initial ispyb deposition and create the nexus writer (but not actually write the file) -3. `CONST.PLAN.ROTATION_MAIN`: Emitted N times, inside `CONST.PLAN.ROTATION_OUTER` run. Used to finish writing to ispyb (i.e. write success/failure) and to trigger zocalo (this is causing errors, see https://github.com/DiamondLightSource/mx-bluesky/issues/826) - -Single rotations only trigger 2 runs. One `CONST.PLAN.ROTATION_OUTER` and one `CONST.PLAN.ROTATION_MAIN`. Both of which do as above. +3. `CONST.PLAN.ROTATION_MAIN`: Emitted N times, inside `CONST.PLAN.ROTATION_OUTER` run. Used to finish writing to ispyb (i.e. write success/failure) and to send collection information to zocalo. diff --git a/src/mx_bluesky/common/external_interaction/callbacks/common/zocalo_callback.py b/src/mx_bluesky/common/external_interaction/callbacks/common/zocalo_callback.py index bb2c15bf7..4f27704a5 100644 --- a/src/mx_bluesky/common/external_interaction/callbacks/common/zocalo_callback.py +++ b/src/mx_bluesky/common/external_interaction/callbacks/common/zocalo_callback.py @@ -18,10 +18,11 @@ class ZocaloCallback(CallbackBase): """Callback class to handle the triggering of Zocalo processing. - Sends zocalo a run_start signal on receiving a start document for the specified - sub-plan, and sends a run_end signal on receiving a stop document for the same plan. + Will start listening for collections when {triggering_plan} has been started. - The metadata of the sub-plan this starts on must include a zocalo_environment. + For every ispyb deposition that occurs inside this run the callback will send zocalo + a run_start signal. Once the {triggering_plan} has ended the callback will send a + run_end signal for all collections. Shouldn't be subscribed directly to the RunEngine, instead should be passed to the `emit` argument of an ISPyB callback which appends DCIDs to the relevant start doc. @@ -30,7 +31,9 @@ class ZocaloCallback(CallbackBase): def _reset_state(self): self.run_uid: str | None = None self.zocalo_info: list[ZocaloStartInfo] = [] + self._started_zocalo_collections: list[ZocaloStartInfo] = [] self.descriptors: dict[str, EventDescriptor] = {} + self.start_frame = 0 def __init__(self, triggering_plan: str, zocalo_environment: str): super().__init__() @@ -42,26 +45,21 @@ def start(self, doc: RunStart): ISPYB_ZOCALO_CALLBACK_LOGGER.info("Zocalo handler received start document.") if self.triggering_plan and doc.get("subplan_name") == self.triggering_plan: self.run_uid = doc.get("uid") - assert isinstance(scan_points := doc.get("scan_points"), list) + if self.run_uid: if ( - isinstance(ispyb_ids := doc.get("ispyb_dcids"), tuple) + isinstance(scan_points := doc.get("scan_points"), list) + and isinstance(ispyb_ids := doc.get("ispyb_dcids"), tuple) and len(ispyb_ids) > 0 ): ISPYB_ZOCALO_CALLBACK_LOGGER.info(f"Zocalo triggering for {ispyb_ids}") ids_and_shape = list(zip(ispyb_ids, scan_points, strict=False)) - start_frame = 0 - self.zocalo_info = [] for idx, id_and_shape in enumerate(ids_and_shape): id, shape = id_and_shape num_frames = number_of_frames_from_scan_spec(shape) self.zocalo_info.append( - ZocaloStartInfo(id, None, start_frame, num_frames, idx) + ZocaloStartInfo(id, None, self.start_frame, num_frames, idx) ) - start_frame += num_frames - else: - raise ISPyBDepositionNotMade( - f"No ISPyB IDs received by the start of {self.triggering_plan=}" - ) + self.start_frame += num_frames def descriptor(self, doc: EventDescriptor): self.descriptors[doc["uid"]] = doc @@ -73,6 +71,8 @@ def event(self, doc: Event) -> Event: for start_info in self.zocalo_info: start_info.filename = filename self.zocalo_interactor.run_start(start_info) + self._started_zocalo_collections.append(start_info) + self.zocalo_info = [] return doc def stop(self, doc: RunStop): @@ -80,7 +80,10 @@ def stop(self, doc: RunStop): ISPYB_ZOCALO_CALLBACK_LOGGER.info( f"Zocalo handler received stop document, for run {doc.get('run_start')}." ) - assert self.zocalo_interactor is not None - for info in self.zocalo_info: + if not self._started_zocalo_collections: + raise ISPyBDepositionNotMade( + f"No ISPyB IDs received by the end of {self.triggering_plan=}" + ) + for info in self._started_zocalo_collections: self.zocalo_interactor.run_end(info.ispyb_dcid) self._reset_state() diff --git a/src/mx_bluesky/hyperion/external_interaction/callbacks/__main__.py b/src/mx_bluesky/hyperion/external_interaction/callbacks/__main__.py index 5844d370e..08bb7be2a 100644 --- a/src/mx_bluesky/hyperion/external_interaction/callbacks/__main__.py +++ b/src/mx_bluesky/hyperion/external_interaction/callbacks/__main__.py @@ -67,7 +67,7 @@ def create_rotation_callbacks() -> tuple[ return ( RotationNexusFileCallback(), RotationISPyBCallback( - emit=ZocaloCallback(CONST.PLAN.ROTATION_MAIN, CONST.ZOCALO_ENV) + emit=ZocaloCallback(CONST.PLAN.ROTATION_MULTI, CONST.ZOCALO_ENV) ), ) diff --git a/tests/unit_tests/common/external_interaction/callbacks/test_zocalo_handler.py b/tests/unit_tests/common/external_interaction/callbacks/test_zocalo_handler.py index 27729a8fd..d42c956d9 100644 --- a/tests/unit_tests/common/external_interaction/callbacks/test_zocalo_handler.py +++ b/tests/unit_tests/common/external_interaction/callbacks/test_zocalo_handler.py @@ -5,6 +5,7 @@ from mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback import ( ZocaloCallback, + ZocaloTrigger, ) from mx_bluesky.common.external_interaction.ispyb.ispyb_store import ( IspybIds, @@ -14,25 +15,15 @@ from mx_bluesky.hyperion.external_interaction.callbacks.__main__ import ( create_gridscan_callbacks, ) -from mx_bluesky.hyperion.parameters.constants import CONST from .....conftest import TestData -EXPECTED_DCID = 100 -EXPECTED_RUN_START_MESSAGE = {"event": "start", "ispyb_dcid": EXPECTED_DCID} -EXPECTED_RUN_END_MESSAGE = { - "event": "end", - "ispyb_dcid": EXPECTED_DCID, - "ispyb_wait_for_runstatus": "1", -} +EXPECTED_RUN_START_MESSAGE = {"subplan_name": "test_plan_name", "uid": "my_uuid"} +EXPECTED_RUN_END_MESSAGE = {"event": "end", "run_start": "my_uuid"} td = TestData() -def start_dict(plan_name: str = "test_plan_name", env: str = "test_env"): - return {CONST.TRIGGER.ZOCALO: plan_name, "zocalo_environment": env} - - class TestZocaloHandler: def _setup_handler(self): zocalo_handler = ZocaloCallback("test_plan_name", "test_env") @@ -40,39 +31,53 @@ def _setup_handler(self): def test_handler_doesnt_trigger_on_wrong_plan(self): zocalo_handler = self._setup_handler() - zocalo_handler.start(start_dict("_not_test_plan_name")) # type: ignore - - def test_handler_raises_on_right_plan_with_wrong_metadata(self): - zocalo_handler = self._setup_handler() - with pytest.raises(AssertionError): - zocalo_handler.start({"subplan_name": "test_plan_name"}) # type: ignore + zocalo_handler.start({"sybplan_name": "_not_test_plan_name"}) # type: ignore - def test_handler_raises_on_right_plan_with_no_ispyb_ids(self): + @patch( + "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", + autospec=True, + ) + def test_handler_stores_collection_if_ispyb_ids_come_in_with_triggering_plan( + self, zocalo_trigger: ZocaloTrigger + ): zocalo_handler = self._setup_handler() - with pytest.raises(ISPyBDepositionNotMade): - zocalo_handler.start( - { - "subplan_name": "test_plan_name", - "zocalo_environment": "test_env", - "scan_points": [{"test": [1, 2, 3]}], - } # type: ignore - ) + assert not zocalo_handler.zocalo_info + zocalo_handler.start( + { + **EXPECTED_RUN_START_MESSAGE, + "ispyb_dcids": (135, 139), + "scan_points": [{"test": [1, 2, 3]}, {"test": [2, 3, 4]}], + } # type: ignore + ) + assert len(zocalo_handler.zocalo_info) == 2 @patch( "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", autospec=True, ) - def test_handler_inits_zocalo_trigger_on_right_plan(self, zocalo_trigger): + def test_handler_stores_collection_ispyb_ids_come_in_as_subplan( + self, zocalo_trigger: ZocaloTrigger + ): zocalo_handler = self._setup_handler() + assert not zocalo_handler.zocalo_info + zocalo_handler.start(EXPECTED_RUN_START_MESSAGE) # type: ignore + assert not zocalo_handler.zocalo_info zocalo_handler.start( { - "subplan_name": "test_plan_name", - "zocalo_environment": "test_env", - "ispyb_dcids": (135, 139), + "subplan_name": "other_plan", + "ispyb_dcids": (135,), "scan_points": [{"test": [1, 2, 3]}], } # type: ignore ) - assert zocalo_handler.zocalo_interactor is not None + zocalo_handler.start( + { + "subplan_name": "other_plan", + "ispyb_dcids": (563,), + "scan_points": [{"test": [2, 3, 4]}], + } # type: ignore + ) + + assert len(zocalo_handler.zocalo_info) == 2 @patch( "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", @@ -133,3 +138,15 @@ def test_execution_of_do_fgs_triggers_zocalo_calls( assert zocalo_handler.zocalo_interactor.run_end.call_count == len(dc_ids) # type: ignore zocalo_handler._reset_state.assert_called() + + @patch( + "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", + autospec=True, + ) + def test_handler_raises_on_the_end_of_a_plan_with_no_depositions( + self, zocalo_trigger: ZocaloTrigger + ): + zocalo_handler = self._setup_handler() + zocalo_handler.start(EXPECTED_RUN_START_MESSAGE) # type: ignore + with pytest.raises(ISPyBDepositionNotMade): + zocalo_handler.stop(EXPECTED_RUN_END_MESSAGE) # type: ignore diff --git a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py index c4a3334fd..5f0d994d8 100644 --- a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py +++ b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py @@ -26,6 +26,7 @@ StoreInIspyb, ) from mx_bluesky.common.external_interaction.nexus.nexus_utils import AxisDirection +from mx_bluesky.common.utils.exceptions import ISPyBDepositionNotMade from mx_bluesky.hyperion.experiment_plans.rotation_scan_plan import ( RotationScanComposite, calculate_motion_profile, @@ -547,8 +548,8 @@ def test_zocalo_callback_end_only_gets_called_at_the_end_of_all_collections( parent_mock = MagicMock() parent_mock.eiger = MagicMock(return_value=Status(done=True, success=True)) eiger.unstage = parent_mock.eiger_unstage - callbacks = create_rotation_callbacks() - zocalo_callback = callbacks[1].emit_cb + _, ispyb_callback = create_rotation_callbacks() + zocalo_callback = ispyb_callback.emit_cb assert isinstance(zocalo_callback, ZocaloCallback) zocalo_callback.zocalo_interactor = MagicMock() zocalo_callback.zocalo_interactor.run_end = parent_mock.run_end @@ -557,7 +558,7 @@ def test_zocalo_callback_end_only_gets_called_at_the_end_of_all_collections( RE, test_multi_rotation_params, fake_create_rotation_devices, - [callbacks[1]], # Only add the ispyb callback otherwise we get nexus files + [ispyb_callback], oav_parameters_for_rotation, ) @@ -565,3 +566,123 @@ def test_zocalo_callback_end_only_gets_called_at_the_end_of_all_collections( test_multi_rotation_params.rotation_scans ) assert parent_mock.method_calls[0] == call.eiger_unstage + + +@patch( + "mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback.StoreInIspyb" +) +@patch( + "mx_bluesky.hyperion.experiment_plans.rotation_scan_plan.check_topup_and_wait_if_necessary", + autospec=True, +) +def test_zocalo_start_and_end_not_triggered_if_ispyb_ids_not_present( + _, + mock_ispyb_store: MagicMock, + RE: RunEngine, + test_multi_rotation_params: MultiRotationScan, + fake_create_rotation_devices: RotationScanComposite, + oav_parameters_for_rotation: OAVParameters, +): + _, ispyb_callback = create_rotation_callbacks() + zocalo_callback = ispyb_callback.emit_cb + assert isinstance(zocalo_callback, ZocaloCallback) + zocalo_callback.zocalo_interactor = (zocalo_trigger := MagicMock()) + + ispyb_callback.ispyb = MagicMock(spec=StoreInIspyb) + with pytest.raises(ISPyBDepositionNotMade): + _run_multi_rotation_plan( + RE, + test_multi_rotation_params, + fake_create_rotation_devices, + [ispyb_callback], + oav_parameters_for_rotation, + ) + + zocalo_trigger.run_start.assert_not_called() # type: ignore + + +@patch( + "mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback.StoreInIspyb" +) +@patch( + "mx_bluesky.hyperion.experiment_plans.rotation_scan_plan.check_topup_and_wait_if_necessary", + autospec=True, +) +def test_ispyb_triggered_before_zocalo( + _, + mock_ispyb_store: MagicMock, + RE: RunEngine, + test_multi_rotation_params: MultiRotationScan, + fake_create_rotation_devices: RotationScanComposite, + oav_parameters_for_rotation: OAVParameters, +): + _, ispyb_callback = create_rotation_callbacks() + parent_mock = MagicMock() + + mock_ispyb_store.return_value = MagicMock(spec=StoreInIspyb) + mock_ispyb_store.return_value.begin_deposition = parent_mock.ispyb_begin + mock_ispyb_store.return_value.begin_deposition.return_value = IspybIds( + data_collection_ids=(123,) + ) + + zocalo_callback = ispyb_callback.emit_cb + assert isinstance(zocalo_callback, ZocaloCallback) + zocalo_callback.zocalo_interactor = MagicMock() + zocalo_callback.zocalo_interactor.run_start = parent_mock.zocalo_start + + _run_multi_rotation_plan( + RE, + test_multi_rotation_params, + fake_create_rotation_devices, + [ispyb_callback], + oav_parameters_for_rotation, + ) + + call_names = [call[0] for call in parent_mock.method_calls] + + assert "ispyb_begin" in call_names + assert "zocalo_start" in call_names + + assert call_names.index("ispyb_begin") < call_names.index("zocalo_start") + + +@patch( + "mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback.StoreInIspyb" +) +@patch( + "mx_bluesky.hyperion.experiment_plans.rotation_scan_plan.check_topup_and_wait_if_necessary", + autospec=True, +) +def test_zocalo_start_and_end_called_once_for_each_collection( + _, + mock_ispyb_store: MagicMock, + RE: RunEngine, + test_multi_rotation_params: MultiRotationScan, + fake_create_rotation_devices: RotationScanComposite, + oav_parameters_for_rotation: OAVParameters, +): + _, ispyb_callback = create_rotation_callbacks() + + mock_ispyb_store.return_value = MagicMock(spec=StoreInIspyb) + mock_ispyb_store.return_value.begin_deposition.return_value = IspybIds( + data_collection_ids=(123,) + ) + + zocalo_callback = ispyb_callback.emit_cb + assert isinstance(zocalo_callback, ZocaloCallback) + zocalo_callback.zocalo_interactor = MagicMock() + + _run_multi_rotation_plan( + RE, + test_multi_rotation_params, + fake_create_rotation_devices, + [ispyb_callback], + oav_parameters_for_rotation, + ) + + assert zocalo_callback.zocalo_interactor.run_start.call_count == len( + test_multi_rotation_params.rotation_scans + ) + assert zocalo_callback.zocalo_interactor.run_end.call_count == len( + test_multi_rotation_params.rotation_scans + ) diff --git a/tests/unit_tests/hyperion/external_interaction/callbacks/test_rotation_callbacks.py b/tests/unit_tests/hyperion/external_interaction/callbacks/test_rotation_callbacks.py index 0f05efe90..6e272ca01 100644 --- a/tests/unit_tests/hyperion/external_interaction/callbacks/test_rotation_callbacks.py +++ b/tests/unit_tests/hyperion/external_interaction/callbacks/test_rotation_callbacks.py @@ -9,10 +9,8 @@ ) from mx_bluesky.common.external_interaction.ispyb.ispyb_store import ( IspybIds, - StoreInIspyb, ) from mx_bluesky.common.parameters.components import IspybExperimentType -from mx_bluesky.common.utils.exceptions import ISPyBDepositionNotMade from mx_bluesky.hyperion.experiment_plans.rotation_scan_plan import rotation_scan from mx_bluesky.hyperion.external_interaction.callbacks.__main__ import ( create_rotation_callbacks, @@ -94,83 +92,6 @@ def test_nexus_handler_only_writes_once( cb.writer.create_nexus_file.assert_called_once() # type: ignore -@patch( - "mx_bluesky.hyperion.external_interaction.callbacks.rotation.nexus_callback.NexusWriter", - autospec=True, -) -@patch( - "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", - autospec=True, -) -@patch( - "mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback.StoreInIspyb", - autospec=True, -) -def test_zocalo_start_and_end_not_triggered_if_ispyb_ids_not_present( - ispyb_store, - zocalo_trigger_class, - nexus_writer, - RE: RunEngine, - params: RotationScan, - do_rotation_scan, -): - nexus_writer.return_value.data_filename = "test_full_filename" - nexus_callback, ispyb_callback = create_rotation_callbacks() - activate_callbacks((nexus_callback, ispyb_callback)) - zocalo_trigger = zocalo_trigger_class.return_value - - ispyb_callback.ispyb = MagicMock(spec=StoreInIspyb) - ispyb_callback.params = params - with pytest.raises(ISPyBDepositionNotMade): - RE.subscribe(nexus_callback) - RE.subscribe(ispyb_callback) - RE(do_rotation_scan) - zocalo_trigger.run_start.assert_not_called() # type: ignore - - -@patch( - "mx_bluesky.hyperion.external_interaction.callbacks.rotation.nexus_callback.NexusWriter", - autospec=True, -) -@patch( - "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", - autospec=True, -) -@patch( - "mx_bluesky.hyperion.external_interaction.callbacks.rotation.ispyb_callback.StoreInIspyb" -) -def test_ispyb_triggered_before_zocalo( - ispyb_store, zocalo_trigger_class, nexus_writer, RE: RunEngine, do_rotation_scan -): - mock_store_in_ispyb_instance = MagicMock(spec=StoreInIspyb) - returned_ids = IspybIds(data_collection_group_id=0, data_collection_ids=(0,)) - mock_store_in_ispyb_instance.begin_deposition.return_value = returned_ids - mock_store_in_ispyb_instance.update_deposition.return_value = returned_ids - - ispyb_store.return_value = mock_store_in_ispyb_instance - nexus_writer.return_value.data_filename = "test_full_filename" - nexus_callback, ispyb_callback = create_rotation_callbacks() - activate_callbacks((nexus_callback, ispyb_callback)) - ispyb_callback.emit_cb.stop = MagicMock() # type: ignore - - parent_mock = MagicMock() - parent_mock.attach_mock(zocalo_trigger_class.return_value, "zocalo") - parent_mock.attach_mock(mock_store_in_ispyb_instance, "ispyb") - - RE.subscribe(nexus_callback) - RE.subscribe(ispyb_callback) - RE(do_rotation_scan) - - call_names = [call[0] for call in parent_mock.method_calls] - - assert "ispyb.begin_deposition" in call_names - assert "zocalo.run_start" in call_names - - assert call_names.index("ispyb.begin_deposition") < call_names.index( - "zocalo.run_start" - ) - - @patch( "mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger", autospec=True, From a36f136768bb1bd99fce474c7a876f6441a08ed0 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Fri, 21 Feb 2025 12:58:55 +0000 Subject: [PATCH 5/9] Unstage before ending zocalo --- src/mx_bluesky/hyperion/experiment_plans/rotation_scan_plan.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mx_bluesky/hyperion/experiment_plans/rotation_scan_plan.py b/src/mx_bluesky/hyperion/experiment_plans/rotation_scan_plan.py index 899ff73ef..70ef569fb 100644 --- a/src/mx_bluesky/hyperion/experiment_plans/rotation_scan_plan.py +++ b/src/mx_bluesky/hyperion/experiment_plans/rotation_scan_plan.py @@ -461,6 +461,8 @@ def rotation_scan_core( yield from rotation_scan_core(single_scan) + yield from bps.unstage(eiger) + LOGGER.info("setting up and staging eiger...") yield from start_preparing_data_collection_then_do_plan( composite.beamstop, @@ -470,4 +472,3 @@ def rotation_scan_core( _multi_rotation_scan(), group=CONST.WAIT.ROTATION_READY_FOR_DC, ) - yield from bps.unstage(eiger) From 6b909af5fcc6a371d901d3c2be18fedf55ae31d8 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Fri, 21 Feb 2025 13:02:04 +0000 Subject: [PATCH 6/9] Add test to confirm transmission isn't changed until after collection --- .../test_multi_rotation_scan_plan.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py index 5f0d994d8..8f32c99da 100644 --- a/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py +++ b/tests/unit_tests/hyperion/experiment_plans/test_multi_rotation_scan_plan.py @@ -15,6 +15,7 @@ from bluesky.simulators import RunEngineSimulator, assert_message_and_return_remaining from dodal.devices.oav.oav_parameters import OAVParameters from dodal.devices.synchrotron import SynchrotronMode +from dodal.devices.xbpm_feedback import Pause from ophyd.status import Status from ophyd_async.testing import set_mock_value @@ -686,3 +687,34 @@ def test_zocalo_start_and_end_called_once_for_each_collection( assert zocalo_callback.zocalo_interactor.run_end.call_count == len( test_multi_rotation_params.rotation_scans ) + + +def test_multi_rotation_scan_does_not_change_transmission_back_until_after_data_collected( + fake_create_rotation_devices: RotationScanComposite, + test_multi_rotation_params: MultiRotationScan, + sim_run_engine_for_rotation: RunEngineSimulator, + oav_parameters_for_rotation: OAVParameters, +): + msgs = sim_run_engine_for_rotation.simulate_plan( + multi_rotation_scan( + fake_create_rotation_devices, + test_multi_rotation_params, + oav_parameters_for_rotation, + ) + ) + msgs = assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "unstage" and msg.obj.name == "eiger", + ) + msgs = assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "set" + and msg.obj.name == "xbpm_feedback-pause_feedback" + and msg.args[0] == Pause.RUN.value, + ) + msgs = assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "set" + and msg.obj.name == "attenuator" + and msg.args[0] == 1.0, + ) From 35f9a107ed9f4ba2a12a274ef406711eb34851d4 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Fri, 21 Feb 2025 13:06:31 +0000 Subject: [PATCH 7/9] Fix docs typo --- .../general/explanations/callback_and_run_logic.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/developer/general/explanations/callback_and_run_logic.rst b/docs/developer/general/explanations/callback_and_run_logic.rst index 12df7fe92..4d4368752 100644 --- a/docs/developer/general/explanations/callback_and_run_logic.rst +++ b/docs/developer/general/explanations/callback_and_run_logic.rst @@ -18,6 +18,6 @@ Single scans will be removed in https://github.com/DiamondLightSource/mx-bluesky For multi rotations this is does by starting 1+2*N different runs: -1. `CONST.PLAN.ROTATION_MULTI`: This is emitted once for the whole multiple rotation. It is used by the nexus callback to get the full number of images and meta_data_run_number so that it knows which hdf file to use. When this is finished zocalo end is triggered. -2. `CONST.PLAN.ROTATION_OUTER`: Emitted N times, inside a `CONST.PLAN.ROTATION_MULTI` run. This is used to create the initial ispyb deposition and create the nexus writer (but not actually write the file) -3. `CONST.PLAN.ROTATION_MAIN`: Emitted N times, inside `CONST.PLAN.ROTATION_OUTER` run. Used to finish writing to ispyb (i.e. write success/failure) and to send collection information to zocalo. +1. ``CONST.PLAN.ROTATION_MULTI``: This is emitted once for the whole multiple rotation. It is used by the nexus callback to get the full number of images and meta_data_run_number so that it knows which hdf file to use. When this is finished zocalo end is triggered. +2. ``CONST.PLAN.ROTATION_OUTER``: Emitted N times, inside a ``CONST.PLAN.ROTATION_MULTI`` run. This is used to create the initial ispyb deposition and create the nexus writer (but not actually write the file) +3. ``CONST.PLAN.ROTATION_MAIN``: Emitted N times, inside ``CONST.PLAN.ROTATION_OUTER`` run. Used to finish writing to ispyb (i.e. write success/failure) and to send collection information to zocalo. From 72cc4b6f2e8d0ec7d2da7a0ac336248a64b18ad7 Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Tue, 4 Mar 2025 16:56:59 +0000 Subject: [PATCH 8/9] Update docs/developer/general/explanations/callback_and_run_logic.rst Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com> --- .../general/explanations/callback_and_run_logic.rst | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/developer/general/explanations/callback_and_run_logic.rst b/docs/developer/general/explanations/callback_and_run_logic.rst index 4d4368752..622046cbe 100644 --- a/docs/developer/general/explanations/callback_and_run_logic.rst +++ b/docs/developer/general/explanations/callback_and_run_logic.rst @@ -1,7 +1,11 @@ +Callbacks and Run Logic +======================== + Callbacks are used to trigger external services: -* Ispyb deposition -* Nexus writing -* Zocalo triggering + +- Ispyb deposition +- Nexus writing +- Zocalo triggering These are linked in that to trigger zocalo you need to have made an ispyb deposition, written a nexus file and have finished writing raw data to disk. Nexus files and ispyb depositions can be made at anytime, we do not need to have necessarily finished writing raw data. From 76b0e294b9c152432691399e1efd336a5a36fcae Mon Sep 17 00:00:00 2001 From: Dominic Oram Date: Tue, 4 Mar 2025 16:57:12 +0000 Subject: [PATCH 9/9] Update docs/developer/general/explanations/callback_and_run_logic.rst Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com> --- docs/developer/general/explanations/callback_and_run_logic.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developer/general/explanations/callback_and_run_logic.rst b/docs/developer/general/explanations/callback_and_run_logic.rst index 622046cbe..e1e328fa7 100644 --- a/docs/developer/general/explanations/callback_and_run_logic.rst +++ b/docs/developer/general/explanations/callback_and_run_logic.rst @@ -14,7 +14,7 @@ Currently, the requirement of needing to have written to ispyb is explicit as th In general, the ordering flow of when callbacks are triggered is controlled by emitting documents with the expected plan name and data. Rotation Scans -============== +--------------------- There are currently two ways of doing rotation scans. A single scan creates one hdf file, one ispyb deposition and then triggers zocalo once. Multi rotation scans create one hdf file for all rotations but then N nexus files, N ispyb depositions and triggers zocalo N times.