From 22e72633bcfcd2a0384cef231ac3f360f52e352d Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Thu, 29 Aug 2024 14:48:26 +0100 Subject: [PATCH 01/17] ZocaloResults: add parameter to use results from GPU --- src/dodal/devices/zocalo/zocalo_results.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 1aa74c7432..73386c56a7 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -61,7 +61,7 @@ class ZocaloResults(StandardReadable, Triggerable): be triggered from a plan-subscribed callback using the run_start() and run_end() methods on dodal.devices.zocalo.ZocaloTrigger. - See https://github.com/DiamondLightSource/dodal/wiki/How-to-Interact-with-Zocalo""" + See https://diamondlightsource.github.io/dodal/main/how-to/zocalo.html""" def __init__( self, @@ -71,6 +71,7 @@ def __init__( sort_key: str = DEFAULT_SORT_KEY.value, timeout_s: float = DEFAULT_TIMEOUT, prefix: str = "", + use_fastest_zocalo_result: bool = False ) -> None: self.zocalo_environment = zocalo_environment self.sort_key = SortKeys[sort_key] @@ -79,6 +80,7 @@ def __init__( self._prefix = prefix self._raw_results_received: Queue = Queue() self.transport: CommonTransport | None = None + self.use_fastest_zocalo_result = use_fastest_zocalo_result self.results, self._results_setter = soft_signal_r_and_setter( list[XrcResult], name="results" @@ -237,9 +239,18 @@ def _receive_result( self.transport.ack(header) # type: ignore # we create transport here results = message.get("results", []) - self._raw_results_received.put( - {"results": results, "ispyb_ids": recipe_parameters} - ) + + if self.use_fastest_zocalo_result: + self._raw_results_received.put( + {"results": results, "ispyb_ids": recipe_parameters} + ) + else: + #Only add to queue if results are from CPU + if recipe_parameters.get('gpu') == None: + self._raw_results_received.put( + {"results": results, "ispyb_ids": recipe_parameters} + ) + subscription = workflows.recipe.wrap_subscribe( self.transport, From 92af2e9bb378f366f96c13227751fc1f3ef767df Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Thu, 29 Aug 2024 15:38:50 +0100 Subject: [PATCH 02/17] Warn if CPU results arrived before GPU results --- src/dodal/devices/zocalo/zocalo_results.py | 5 ++++- tests/devices/unit_tests/test_zocalo_results.py | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 73386c56a7..bde90271c1 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -167,7 +167,10 @@ async def trigger(self): ) raw_results = self._raw_results_received.get(timeout=self.timeout_s) - LOGGER.info(f"Zocalo: found {len(raw_results['results'])} crystals.") + source_of_results = "CPU" if raw_results["ispyb_ids"]["gpu"] == None else "GPU" + if source_of_results.equals == "CPU" and self.use_fastest_zocalo_result: + LOGGER.warn("Recieved zocalo results from CPU before GPU") + LOGGER.info(f"Zocalo results from {source_of_results} processing: found {len(raw_results['results'])} crystals.") # Sort from strongest to weakest in case of multiple crystals await self._put_results( sorted( diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 180b84d077..839d43c584 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -207,11 +207,14 @@ async def test_zocalo_results_trigger_log_message( name="zocalo", zocalo_environment="dev_artemis", timeout_s=2 ) + recipe_wrapper = MagicMock() + recipe_wrapper.recipe_step = {"parameters": {}} + def zocalo_plan(): yield from bps.stage(zocalo_results) receive_result = mock_wrap_subscribe.mock_calls[0].args[2] receive_result( - MagicMock(autospec=RecipeWrapper), + recipe_wrapper, {}, { "results": [ From 90e8385688a2393436634cdb462bf5911bc7441c Mon Sep 17 00:00:00 2001 From: olliesilvester <122091460+olliesilvester@users.noreply.github.com> Date: Thu, 29 Aug 2024 16:25:07 +0100 Subject: [PATCH 03/17] Update src/dodal/devices/zocalo/zocalo_results.py Co-authored-by: Nicholas Devenish --- src/dodal/devices/zocalo/zocalo_results.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index bde90271c1..1754419d74 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -167,7 +167,7 @@ async def trigger(self): ) raw_results = self._raw_results_received.get(timeout=self.timeout_s) - source_of_results = "CPU" if raw_results["ispyb_ids"]["gpu"] == None else "GPU" + source_of_results = "CPU" if not raw_results["ispyb_ids"].get("gpu") else "GPU" if source_of_results.equals == "CPU" and self.use_fastest_zocalo_result: LOGGER.warn("Recieved zocalo results from CPU before GPU") LOGGER.info(f"Zocalo results from {source_of_results} processing: found {len(raw_results['results'])} crystals.") From 4871d735c13294a80df22cfa5353cf861750764d Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Thu, 29 Aug 2024 16:53:33 +0100 Subject: [PATCH 04/17] Correct typo --- src/dodal/devices/zocalo/zocalo_results.py | 2 +- tests/devices/unit_tests/test_zocalo_results.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 1754419d74..2dbab217f9 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -168,7 +168,7 @@ async def trigger(self): raw_results = self._raw_results_received.get(timeout=self.timeout_s) source_of_results = "CPU" if not raw_results["ispyb_ids"].get("gpu") else "GPU" - if source_of_results.equals == "CPU" and self.use_fastest_zocalo_result: + if source_of_results == "CPU" and self.use_fastest_zocalo_result: LOGGER.warn("Recieved zocalo results from CPU before GPU") LOGGER.info(f"Zocalo results from {source_of_results} processing: found {len(raw_results['results'])} crystals.") # Sort from strongest to weakest in case of multiple crystals diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 839d43c584..5de2ff4426 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -238,7 +238,7 @@ def zocalo_plan(): yield from bps.trigger(zocalo_results) RE(zocalo_plan()) - mock_logger.info.assert_has_calls([call("Zocalo: found 1 crystals.")]) + mock_logger.info.assert_has_calls([call("Zocalo results from CPU processing: found 1 crystals.")]) @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) From f1813257fbe078fb27e2832185255ff7f60e1dc6 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Mon, 2 Sep 2024 18:34:48 +0100 Subject: [PATCH 05/17] Update with new criteria and add tests --- src/dodal/devices/zocalo/zocalo_results.py | 62 +++++++-- .../devices/unit_tests/test_zocalo_results.py | 129 +++++++++++++++++- 2 files changed, 179 insertions(+), 12 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 2dbab217f9..b46453b916 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -56,6 +56,19 @@ def bbox_size(result: XrcResult): ] +# TODO this may give uneccessary warnings if dicts are different within rounding errors +def get_dict_differences( + dict1: dict, dict1_source: str, dict2: dict, dict2_source: str +) -> str: + differences_str = "" + for key in dict1: + if not dict1[key] == dict2[key]: + differences_str += f"Results differed in {key}: {dict1_source} contains {dict1[key]} while {dict2_source} contains {dict2[key]} \n" + if differences_str: + LOGGER.warning(differences_str) + return differences_str + + class ZocaloResults(StandardReadable, Triggerable): """An ophyd device which can wait for results from a Zocalo job. These jobs should be triggered from a plan-subscribed callback using the run_start() and run_end() @@ -71,7 +84,7 @@ def __init__( sort_key: str = DEFAULT_SORT_KEY.value, timeout_s: float = DEFAULT_TIMEOUT, prefix: str = "", - use_fastest_zocalo_result: bool = False + use_fastest_zocalo_result: bool = False, ) -> None: self.zocalo_environment = zocalo_environment self.sort_key = SortKeys[sort_key] @@ -167,10 +180,40 @@ async def trigger(self): ) raw_results = self._raw_results_received.get(timeout=self.timeout_s) - source_of_results = "CPU" if not raw_results["ispyb_ids"].get("gpu") else "GPU" - if source_of_results == "CPU" and self.use_fastest_zocalo_result: - LOGGER.warn("Recieved zocalo results from CPU before GPU") - LOGGER.info(f"Zocalo results from {source_of_results} processing: found {len(raw_results['results'])} crystals.") + source_of_first_results, source_of_second_results = ( + ("CPU", "GPU") + if not raw_results["ispyb_ids"].get("gpu") + else ("GPU", "CPU") + ) + + # Wait for results from CPU and GPU, warn and continue if one timed out, error if both time out + if self.use_fastest_zocalo_result: + if source_of_first_results == "CPU": + LOGGER.warning("Recieved zocalo results from CPU before GPU") + raw_results_two_sources = [raw_results] + try: + raw_results_two_sources.append( + self._raw_results_received.get(timeout=self.timeout_s / 2) + ) + + # Compare results from both sources and warn if they aren't the same + differences_str = get_dict_differences( + raw_results_two_sources[0]["results"], + source_of_first_results, + raw_results_two_sources[1]["results"], + source_of_second_results, + ) + if differences_str: + LOGGER.warning(differences_str) + + except Empty: + LOGGER.warning( + f"Zocalo results from {source_of_second_results} timed out. Using results from {source_of_first_results}" + ) + + LOGGER.info( + f"Zocalo results from {source_of_first_results} processing: found {len(raw_results['results'])} crystals." + ) # Sort from strongest to weakest in case of multiple crystals await self._put_results( sorted( @@ -248,12 +291,11 @@ def _receive_result( {"results": results, "ispyb_ids": recipe_parameters} ) else: - #Only add to queue if results are from CPU - if recipe_parameters.get('gpu') == None: + # Only add to queue if results are from CPU + if not recipe_parameters.get("gpu"): self._raw_results_received.put( - {"results": results, "ispyb_ids": recipe_parameters} - ) - + {"results": results, "ispyb_ids": recipe_parameters} + ) subscription = workflows.recipe.wrap_subscribe( self.transport, diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 5de2ff4426..80931454c2 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -1,4 +1,5 @@ from functools import partial +from queue import Empty from unittest.mock import AsyncMock, MagicMock, call, patch import bluesky.plan_stubs as bps @@ -7,13 +8,14 @@ from bluesky.run_engine import RunEngine from bluesky.utils import FailedStatus from ophyd_async.core.async_status import AsyncStatus -from workflows.recipe import RecipeWrapper from dodal.devices.zocalo.zocalo_results import ( ZOCALO_READING_PLAN_NAME, + NoResultsFromZocalo, NoZocaloSubscription, XrcResult, ZocaloResults, + get_dict_differences, get_processing_result, ) @@ -238,7 +240,9 @@ def zocalo_plan(): yield from bps.trigger(zocalo_results) RE(zocalo_plan()) - mock_logger.info.assert_has_calls([call("Zocalo results from CPU processing: found 1 crystals.")]) + mock_logger.info.assert_has_calls( + [call("Zocalo results from CPU processing: found 1 crystals.")] + ) @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) @@ -253,3 +257,124 @@ async def test_when_exception_caused_by_zocalo_message_then_exception_propagated RE(bps.trigger(zocalo_results, wait=True)) assert isinstance(e.value.__cause__, NoZocaloSubscription) + + +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_if_use_fastest_zocalo_results_then_wait_twice_for_results( + mock_connection, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + ) + + await zocalo_results.connect() + await zocalo_results.stage() + zocalo_results._raw_results_received.put([]) + zocalo_results._raw_results_received.put([]) + zocalo_results._raw_results_received.get = MagicMock() + RE(bps.trigger(zocalo_results, wait=False)) + assert zocalo_results._raw_results_received.get.call_count == 2 + + +@patch("dodal.devices.zocalo.zocalo_results.LOGGER") +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_source_of_zocalo_results_correctly_identified( + mock_connection, mock_logger, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=False + ) + await zocalo_results.connect() + await zocalo_results.stage() + + zocalo_results._raw_results_received.get = MagicMock( + return_value={"ispyb_ids": {"test": 0}, "results": []} + ) + RE(bps.trigger(zocalo_results, wait=False)) + mock_logger.info.assert_has_calls( + [call("Zocalo results from CPU processing: found 0 crystals.")] + ) + + zocalo_results._raw_results_received.get = MagicMock( + return_value={"ispyb_ids": {"gpu": True}, "results": []} + ) + RE(bps.trigger(zocalo_results, wait=False)) + mock_logger.info.assert_has_calls( + [call("Zocalo results from GPU processing: found 0 crystals.")] + ) + + +# TODO figure out how to test the transport bit +# @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +# async def test_if_not_use_fastest_zocalo_results_then_only_wait_for_cpu_results(mock_connection, RE: RunEngine): +# zocalo_results = ZocaloResults( +# name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=False +# ) +# await zocalo_results.connect() +# await zocalo_results.stage() +# zocalo_results._raw_results_received.put([]) +# pass + + +def test_compare_cpu_and_gpu_results_warns_correctly(): + dict1 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 2]]} + dict2 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 2]]} + assert not get_dict_differences(dict1, "dict1", dict2, "dict2") + dict1 = {"key1": [2, 2, 3], "key2": "test", "key3": [[1, 2], [1, 4]]} + dict2 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 3]]} + assert ( + get_dict_differences(dict1, "dict1", dict2, "dict2") + == f"Results differed in key1: dict1 contains {dict1['key1']} while dict2 contains {dict2['key1']} \nResults differed in key3: dict1 contains {dict1['key3']} while dict2 contains {dict2['key3']} \n" + ) + + +@patch("dodal.devices.zocalo.zocalo_results.LOGGER") +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_if_zocalo_results_timeout_from_one_source_then_warn( + mock_connection, mock_logger, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + ) + await zocalo_results.connect() + await zocalo_results.stage() + zocalo_results._raw_results_received.get = MagicMock( + side_effect=[{"ispyb_ids": {"test": 0}, "results": []}, Empty] + ) + RE(bps.trigger(zocalo_results, wait=False)) + mock_logger.warning.assert_called_with( + "Zocalo results from GPU timed out. Using results from CPU" + ) + + +@patch("dodal.devices.zocalo.zocalo_results.LOGGER") +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_if_cpu_results_arrive_before_gpu_then_warn( + mock_connection, mock_logger, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + ) + await zocalo_results.connect() + await zocalo_results.stage() + zocalo_results._raw_results_received.get = MagicMock( + return_value={"ispyb_ids": {"test": 0}, "results": []} + ) + RE(bps.trigger(zocalo_results, wait=False)) + mock_logger.warning.assert_called_with( + "Recieved zocalo results from CPU before GPU" + ) + + +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_if_zocalo_results_timeout_before_any_results_then_error( + mock_connection, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + ) + await zocalo_results.connect() + await zocalo_results.stage() + zocalo_results._raw_results_received.get = MagicMock(side_effect=Empty) + with pytest.raises(NoResultsFromZocalo): + await zocalo_results.trigger() From 0632f5aba10143a96583960a0be478528df286eb Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Mon, 2 Sep 2024 19:29:06 +0100 Subject: [PATCH 06/17] more tests for codecov --- src/dodal/devices/zocalo/zocalo_results.py | 6 +- .../devices/unit_tests/test_zocalo_results.py | 76 ++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index b46453b916..857055405b 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -61,7 +61,7 @@ def get_dict_differences( dict1: dict, dict1_source: str, dict2: dict, dict2_source: str ) -> str: differences_str = "" - for key in dict1: + for key in dict1.keys(): if not dict1[key] == dict2[key]: differences_str += f"Results differed in {key}: {dict1_source} contains {dict1[key]} while {dict2_source} contains {dict2[key]} \n" if differences_str: @@ -198,9 +198,9 @@ async def trigger(self): # Compare results from both sources and warn if they aren't the same differences_str = get_dict_differences( - raw_results_two_sources[0]["results"], + raw_results_two_sources[0]["results"][0], source_of_first_results, - raw_results_two_sources[1]["results"], + raw_results_two_sources[1]["results"][0], source_of_second_results, ) if differences_str: diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 80931454c2..a0fc6a232a 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -206,7 +206,10 @@ async def test_zocalo_results_trigger_log_message( mock_wrap_subscribe, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", timeout_s=2 + name="zocalo", + zocalo_environment="dev_artemis", + timeout_s=2, + use_fastest_zocalo_result=True, ) recipe_wrapper = MagicMock() @@ -366,6 +369,28 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( ) +@patch("dodal.devices.zocalo.zocalo_results.LOGGER") +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_warning_if_results_are_different( + mock_connection, mock_logger, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + ) + await zocalo_results.connect() + await zocalo_results.stage() + zocalo_results._raw_results_received.get = MagicMock( + side_effect=[ + {"ispyb_ids": {}, "results": [{"test": 0}]}, + {"ispyb_ids": {}, "results": [{"test": 1}]}, + ] + ) + RE(bps.trigger(zocalo_results, wait=False)) + mock_logger.warning.assert_called_with( + "Results differed in test: CPU contains 0 while GPU contains 1 \n" + ) + + @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) async def test_if_zocalo_results_timeout_before_any_results_then_error( mock_connection, RE: RunEngine @@ -378,3 +403,52 @@ async def test_if_zocalo_results_timeout_before_any_results_then_error( zocalo_results._raw_results_received.get = MagicMock(side_effect=Empty) with pytest.raises(NoResultsFromZocalo): await zocalo_results.trigger() + + +@patch("dodal.devices.zocalo.zocalo_results.LOGGER") +@patch( + "dodal.devices.zocalo.zocalo_results.workflows.recipe.wrap_subscribe", autospec=True +) +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", new=MagicMock()) +async def test_gpu_results_ignored_if_toggle_disabled( + mock_wrap_subscribe, mock_logger, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", + zocalo_environment="dev_artemis", + timeout_s=2, + use_fastest_zocalo_result=False, + ) + + recipe_wrapper = MagicMock() + recipe_wrapper.recipe_step = {"parameters": {"gpu": True}} + + def zocalo_plan(): + yield from bps.stage(zocalo_results) + receive_result = mock_wrap_subscribe.mock_calls[0].args[2] + receive_result( + recipe_wrapper, + {}, + { + "results": [ + { + "centre_of_mass": [ + 2.207133058984911, + 1.4175240054869684, + 13.317215363511659, + ], + "max_voxel": [2, 1, 13], + "max_count": 702.0, + "n_voxels": 12, + "total_count": 5832.0, + "bounding_box": [[1, 0, 12], [4, 3, 15]], + } + ], + "status": "success", + "type": "3d", + }, + ) + yield from bps.trigger(zocalo_results) + mock_logger.warning.assert_called_with("Timed out waiting for zocalo results!") + + RE(zocalo_plan()) From 9b1b4c9c0be94d477f33a0f0c3e72a55890663b1 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Tue, 3 Sep 2024 09:26:31 +0100 Subject: [PATCH 07/17] remove extra comments --- src/dodal/devices/zocalo/zocalo_results.py | 1 - tests/devices/unit_tests/test_zocalo_results.py | 12 ------------ 2 files changed, 13 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 857055405b..01a9fb7e2f 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -56,7 +56,6 @@ def bbox_size(result: XrcResult): ] -# TODO this may give uneccessary warnings if dicts are different within rounding errors def get_dict_differences( dict1: dict, dict1_source: str, dict2: dict, dict2_source: str ) -> str: diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index a0fc6a232a..f3108540b5 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -307,18 +307,6 @@ async def test_source_of_zocalo_results_correctly_identified( ) -# TODO figure out how to test the transport bit -# @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) -# async def test_if_not_use_fastest_zocalo_results_then_only_wait_for_cpu_results(mock_connection, RE: RunEngine): -# zocalo_results = ZocaloResults( -# name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=False -# ) -# await zocalo_results.connect() -# await zocalo_results.stage() -# zocalo_results._raw_results_received.put([]) -# pass - - def test_compare_cpu_and_gpu_results_warns_correctly(): dict1 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 2]]} dict2 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 2]]} From 99b8f494718162a078138a536d0a2f60d020729b Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Tue, 3 Sep 2024 09:41:27 +0100 Subject: [PATCH 08/17] Change toggle name --- src/dodal/devices/zocalo/zocalo_results.py | 8 ++++---- .../devices/unit_tests/test_zocalo_results.py | 18 +++++++++--------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 01a9fb7e2f..7d64ad2edf 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -83,7 +83,7 @@ def __init__( sort_key: str = DEFAULT_SORT_KEY.value, timeout_s: float = DEFAULT_TIMEOUT, prefix: str = "", - use_fastest_zocalo_result: bool = False, + use_cpu_and_gpu_zocalo: bool = False, ) -> None: self.zocalo_environment = zocalo_environment self.sort_key = SortKeys[sort_key] @@ -92,7 +92,7 @@ def __init__( self._prefix = prefix self._raw_results_received: Queue = Queue() self.transport: CommonTransport | None = None - self.use_fastest_zocalo_result = use_fastest_zocalo_result + self.use_cpu_and_gpu_zocalo = use_cpu_and_gpu_zocalo self.results, self._results_setter = soft_signal_r_and_setter( list[XrcResult], name="results" @@ -186,7 +186,7 @@ async def trigger(self): ) # Wait for results from CPU and GPU, warn and continue if one timed out, error if both time out - if self.use_fastest_zocalo_result: + if self.use_cpu_and_gpu_zocalo: if source_of_first_results == "CPU": LOGGER.warning("Recieved zocalo results from CPU before GPU") raw_results_two_sources = [raw_results] @@ -285,7 +285,7 @@ def _receive_result( results = message.get("results", []) - if self.use_fastest_zocalo_result: + if self.use_cpu_and_gpu_zocalo: self._raw_results_received.put( {"results": results, "ispyb_ids": recipe_parameters} ) diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index f3108540b5..207b0ce4b9 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -209,7 +209,7 @@ async def test_zocalo_results_trigger_log_message( name="zocalo", zocalo_environment="dev_artemis", timeout_s=2, - use_fastest_zocalo_result=True, + use_cpu_and_gpu_zocalo=True, ) recipe_wrapper = MagicMock() @@ -263,11 +263,11 @@ async def test_when_exception_caused_by_zocalo_message_then_exception_propagated @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) -async def test_if_use_fastest_zocalo_results_then_wait_twice_for_results( +async def test_if_use_cpu_and_gpu_zocalos_then_wait_twice_for_results( mock_connection, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True ) await zocalo_results.connect() @@ -285,7 +285,7 @@ async def test_source_of_zocalo_results_correctly_identified( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=False + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=False ) await zocalo_results.connect() await zocalo_results.stage() @@ -325,7 +325,7 @@ async def test_if_zocalo_results_timeout_from_one_source_then_warn( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True ) await zocalo_results.connect() await zocalo_results.stage() @@ -344,7 +344,7 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True ) await zocalo_results.connect() await zocalo_results.stage() @@ -363,7 +363,7 @@ async def test_warning_if_results_are_different( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True ) await zocalo_results.connect() await zocalo_results.stage() @@ -384,7 +384,7 @@ async def test_if_zocalo_results_timeout_before_any_results_then_error( mock_connection, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_fastest_zocalo_result=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True ) await zocalo_results.connect() await zocalo_results.stage() @@ -405,7 +405,7 @@ async def test_gpu_results_ignored_if_toggle_disabled( name="zocalo", zocalo_environment="dev_artemis", timeout_s=2, - use_fastest_zocalo_result=False, + use_cpu_and_gpu_zocalo=False, ) recipe_wrapper = MagicMock() From 149c2bde493caacb94361c657a14de0c5da49c23 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Thu, 5 Sep 2024 15:09:50 +0100 Subject: [PATCH 09/17] use deepdiff to get differences between gpu and cpu results --- pyproject.toml | 1 + src/dodal/devices/zocalo/zocalo_results.py | 21 ++++++-- .../devices/unit_tests/test_zocalo_results.py | 48 +++++++++++++++++-- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c4d31959ce..702865ce30 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,7 @@ dev = [ "types-mock", "types-PyYAML", "types-aiofiles", + "deepdiff", ] [project.scripts] diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 7d64ad2edf..15cecbf931 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -10,6 +10,7 @@ import workflows.recipe import workflows.transport from bluesky.protocols import Descriptor, Triggerable +from deepdiff import DeepDiff from numpy.typing import NDArray from ophyd_async.core import HintedSignal, StandardReadable, soft_signal_r_and_setter from ophyd_async.core.async_status import AsyncStatus @@ -60,11 +61,21 @@ def get_dict_differences( dict1: dict, dict1_source: str, dict2: dict, dict2_source: str ) -> str: differences_str = "" - for key in dict1.keys(): - if not dict1[key] == dict2[key]: - differences_str += f"Results differed in {key}: {dict1_source} contains {dict1[key]} while {dict2_source} contains {dict2[key]} \n" - if differences_str: - LOGGER.warning(differences_str) + diff = DeepDiff(dict1, dict2, math_epsilon=1e-5, ignore_numeric_type_changes=True) + # diff is None if there are no differences, otherwise a dictionary with various keys depending + # on the type of difference detected. Eg 'values_changed', dictionary_item_added', 'iterable_item_added' + + if diff: + differences_str += ( + f"Differences found between {dict1_source} and {dict2_source} results:\n" + ) + + # Format the difference more nicely in a string + for key, value in diff.items(): + if key == "values_changed": + differences_str += f" {key}: {str(value).replace('old_value', dict1_source).replace('new_value', dict2_source)}\n" + else: + differences_str += f" {key}: {value}\n" return differences_str diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 207b0ce4b9..8df65f38f8 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -357,10 +357,48 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( ) +@pytest.mark.parametrize( + "dict1,dict2,output", + [ + ( + {"ispyb_ids": {"gpu": True}, "results": [{"test": 0}]}, + {"ispyb_ids": {}, "results": [{"test": 1}]}, + "Differences found between GPU and CPU results:\n values_changed: {\"root['test']\": {'CPU': 1, 'GPU': 0}}\n", + ), + ( + { + "ispyb_ids": {"gpu": True}, + "results": [{"test": [[1, 2 + 1e-6, 3], [1, 2, 3]]}], + }, + {"ispyb_ids": {}, "results": [{"test": [[1, 2, 3], [1, 2, 3]]}]}, + None, + ), + ( + { + "ispyb_ids": {"gpu": True}, + "results": [ + {"test": [[1, 2 + 1e-6, 3], [1, 2, 3]], "extra_key": "test"} + ], + }, + {"ispyb_ids": {}, "results": [{"test": [[1, 2, 3], [1, 2, 3]]}]}, + "Differences found between GPU and CPU results:\n dictionary_item_removed: SetOrdered([\"root['extra_key']\"])\n", + ), + ( + { + "ispyb_ids": {"gpu": False}, + "results": [ + {"test": [[1, 2 + 1e-6, 3], [1, 2, 3]], "extra_key": "test"} + ], + }, + {"ispyb_ids": {}, "results": [{"test": [[1, 3, 3], [1, 2, 3]]}]}, + "Differences found between CPU and GPU results:\n dictionary_item_removed: SetOrdered([\"root['extra_key']\"])\n values_changed: {\"root['test'][0][1]\": {'GPU': 3, 'CPU': 2.000001}}\n", + ), + ], +) @patch("dodal.devices.zocalo.zocalo_results.LOGGER") @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) async def test_warning_if_results_are_different( - mock_connection, mock_logger, RE: RunEngine + mock_connection, mock_logger, RE: RunEngine, dict1, dict2, output ): zocalo_results = ZocaloResults( name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True @@ -369,14 +407,14 @@ async def test_warning_if_results_are_different( await zocalo_results.stage() zocalo_results._raw_results_received.get = MagicMock( side_effect=[ - {"ispyb_ids": {}, "results": [{"test": 0}]}, - {"ispyb_ids": {}, "results": [{"test": 1}]}, + dict1, + dict2, ] ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.warning.assert_called_with( - "Results differed in test: CPU contains 0 while GPU contains 1 \n" - ) + output + ) if output else mock_logger.warning.assert_not_called() @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) From 984a134681604461a82f7666235c689c72be237e Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Fri, 6 Sep 2024 10:12:46 +0100 Subject: [PATCH 10/17] Review response and simplify deepdiff --- src/dodal/devices/zocalo/zocalo_results.py | 89 ++++++++++++------- .../devices/unit_tests/test_zocalo_results.py | 73 ++++++--------- 2 files changed, 86 insertions(+), 76 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 15cecbf931..2ef36dca87 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -34,6 +34,11 @@ class SortKeys(str, Enum): n_voxels = "n_voxels" +class ZocaloSource(str, Enum): + CPU = "CPU" + GPU = "GPU" + + DEFAULT_TIMEOUT = 180 DEFAULT_SORT_KEY = SortKeys.max_count ZOCALO_READING_PLAN_NAME = "zocalo reading" @@ -59,24 +64,22 @@ def bbox_size(result: XrcResult): def get_dict_differences( dict1: dict, dict1_source: str, dict2: dict, dict2_source: str -) -> str: - differences_str = "" +) -> str | None: + """Returns dict1 and dict2 as a string if differences between them are found greater than a + 1e-5 tolerance. If dictionaries are identical, return None""" + diff = DeepDiff(dict1, dict2, math_epsilon=1e-5, ignore_numeric_type_changes=True) - # diff is None if there are no differences, otherwise a dictionary with various keys depending - # on the type of difference detected. Eg 'values_changed', dictionary_item_added', 'iterable_item_added' if diff: - differences_str += ( - f"Differences found between {dict1_source} and {dict2_source} results:\n" - ) + return f"Zocalo results from {dict1_source} and {dict2_source} are not identical.\n Results from {dict1_source}: {dict1}\n Results from {dict2_source}: {dict2}" - # Format the difference more nicely in a string - for key, value in diff.items(): - if key == "values_changed": - differences_str += f" {key}: {str(value).replace('old_value', dict1_source).replace('new_value', dict2_source)}\n" - else: - differences_str += f" {key}: {value}\n" - return differences_str + +def source_from_results(results): + return ( + ZocaloSource.GPU.value + if results["recipe_parameters"].get("gpu") + else ZocaloSource.CPU.value + ) class ZocaloResults(StandardReadable, Triggerable): @@ -84,7 +87,25 @@ class ZocaloResults(StandardReadable, Triggerable): be triggered from a plan-subscribed callback using the run_start() and run_end() methods on dodal.devices.zocalo.ZocaloTrigger. - See https://diamondlightsource.github.io/dodal/main/how-to/zocalo.html""" + See https://diamondlightsource.github.io/dodal/main/how-to/zocalo.html + + Args: + name (str): Name of the device + + zocalo_environment (str): How zocalo is configured. Defaults to i03's development configuration + + channel (str): Name for the results Queue + + sort_key (str): How results are ranked. Defaults to sorting by highest counts + + timeout_s (float): Maximum time to wait for the Queue to be filled by an object, starting + from when the ZocaloResults device is triggered + + prefix (str): EPICS PV prefix for the device + + use_cpu_and_gpu (bool): When True, ZocaloResults will wait for results from the CPU and the GPU, compare them, and provide a warning if the results differ. When False, ZocaloResults will only use results from the CPU + + """ def __init__( self, @@ -94,7 +115,7 @@ def __init__( sort_key: str = DEFAULT_SORT_KEY.value, timeout_s: float = DEFAULT_TIMEOUT, prefix: str = "", - use_cpu_and_gpu_zocalo: bool = False, + use_cpu_and_gpu: bool = False, ) -> None: self.zocalo_environment = zocalo_environment self.sort_key = SortKeys[sort_key] @@ -103,7 +124,7 @@ def __init__( self._prefix = prefix self._raw_results_received: Queue = Queue() self.transport: CommonTransport | None = None - self.use_cpu_and_gpu_zocalo = use_cpu_and_gpu_zocalo + self.use_cpu_and_gpu = use_cpu_and_gpu self.results, self._results_setter = soft_signal_r_and_setter( list[XrcResult], name="results" @@ -132,14 +153,14 @@ def __init__( ) super().__init__(name) - async def _put_results(self, results: Sequence[XrcResult], ispyb_ids): + async def _put_results(self, results: Sequence[XrcResult], recipe_parameters): self._results_setter(list(results)) centres_of_mass = np.array([r["centre_of_mass"] for r in results]) bbox_sizes = np.array([bbox_size(r) for r in results]) self._com_setter(centres_of_mass) self._bbox_setter(bbox_sizes) - self._ispyb_dcid_setter(ispyb_ids["dcid"]) - self._ispyb_dcgid_setter(ispyb_ids["dcgid"]) + self._ispyb_dcid_setter(recipe_parameters["dcid"]) + self._ispyb_dcgid_setter(recipe_parameters["dcgid"]) def _clear_old_results(self): LOGGER.info("Clearing queue") @@ -190,21 +211,20 @@ async def trigger(self): ) raw_results = self._raw_results_received.get(timeout=self.timeout_s) - source_of_first_results, source_of_second_results = ( - ("CPU", "GPU") - if not raw_results["ispyb_ids"].get("gpu") - else ("GPU", "CPU") - ) + source_of_first_results = source_from_results(raw_results) # Wait for results from CPU and GPU, warn and continue if one timed out, error if both time out - if self.use_cpu_and_gpu_zocalo: - if source_of_first_results == "CPU": + if self.use_cpu_and_gpu: + if source_of_first_results == ZocaloSource.CPU: LOGGER.warning("Recieved zocalo results from CPU before GPU") raw_results_two_sources = [raw_results] try: raw_results_two_sources.append( self._raw_results_received.get(timeout=self.timeout_s / 2) ) + source_of_second_results = source_from_results( + raw_results_two_sources[1] + ) # Compare results from both sources and warn if they aren't the same differences_str = get_dict_differences( @@ -217,8 +237,13 @@ async def trigger(self): LOGGER.warning(differences_str) except Empty: + source_of_missing_results = ( + ZocaloSource.CPU.value + if source_of_first_results == ZocaloSource.GPU.value + else ZocaloSource.GPU.value + ) LOGGER.warning( - f"Zocalo results from {source_of_second_results} timed out. Using results from {source_of_first_results}" + f"Zocalo results from {source_of_missing_results} timed out. Using results from {source_of_first_results}" ) LOGGER.info( @@ -231,7 +256,7 @@ async def trigger(self): key=lambda d: d[self.sort_key.value], reverse=True, ), - raw_results["ispyb_ids"], + raw_results["recipe_parameters"], ) except Empty as timeout_exception: LOGGER.warning("Timed out waiting for zocalo results!") @@ -296,15 +321,15 @@ def _receive_result( results = message.get("results", []) - if self.use_cpu_and_gpu_zocalo: + if self.use_cpu_and_gpu: self._raw_results_received.put( - {"results": results, "ispyb_ids": recipe_parameters} + {"results": results, "recipe_parameters": recipe_parameters} ) else: # Only add to queue if results are from CPU if not recipe_parameters.get("gpu"): self._raw_results_received.put( - {"results": results, "ispyb_ids": recipe_parameters} + {"results": results, "recipe_parameters": recipe_parameters} ) subscription = workflows.recipe.wrap_subscribe( diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 8df65f38f8..78b2935408 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -15,7 +15,6 @@ NoZocaloSubscription, XrcResult, ZocaloResults, - get_dict_differences, get_processing_result, ) @@ -79,7 +78,7 @@ }, } -test_ispyb_ids = {"dcid": 0, "dcgid": 0} +test_recipe_parameters = {"dcid": 0, "dcgid": 0} @patch("dodal.devices.zocalo_results._get_zocalo_connection") @@ -90,7 +89,7 @@ async def device(results, run_setup=False): @AsyncStatus.wrap async def mock_trigger(results): - await zd._put_results(results, test_ispyb_ids) + await zd._put_results(results, test_recipe_parameters) zd.trigger = MagicMock(side_effect=partial(mock_trigger, results)) # type: ignore await zd.connect() @@ -113,7 +112,7 @@ async def test_put_result_read_results( RE, ) -> None: zocalo_device = await mocked_zocalo_device([], run_setup=True) - await zocalo_device._put_results(TEST_RESULTS, test_ispyb_ids) + await zocalo_device._put_results(TEST_RESULTS, test_recipe_parameters) reading = await zocalo_device.read() results: list[XrcResult] = reading["zocalo-results"]["value"] centres: list[XrcResult] = reading["zocalo-centres_of_mass"]["value"] @@ -128,7 +127,7 @@ async def test_rd_top_results( RE, ): zocalo_device = await mocked_zocalo_device([], run_setup=True) - await zocalo_device._put_results(TEST_RESULTS, test_ispyb_ids) + await zocalo_device._put_results(TEST_RESULTS, test_recipe_parameters) def test_plan(): bbox_size = yield from bps.rd(zocalo_device.bbox_sizes) @@ -182,7 +181,7 @@ async def test_subscribe_only_on_called_stage( mock_connection: MagicMock, mock_wrap_subscribe: MagicMock, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", timeout_s=2 + name="zocalo", zocalo_environment="dev_artemis", timeout_s=0 ) mock_wrap_subscribe.assert_not_called() await zocalo_results.stage() @@ -209,7 +208,7 @@ async def test_zocalo_results_trigger_log_message( name="zocalo", zocalo_environment="dev_artemis", timeout_s=2, - use_cpu_and_gpu_zocalo=True, + use_cpu_and_gpu=True, ) recipe_wrapper = MagicMock() @@ -267,7 +266,7 @@ async def test_if_use_cpu_and_gpu_zocalos_then_wait_twice_for_results( mock_connection, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=True ) await zocalo_results.connect() @@ -285,13 +284,13 @@ async def test_source_of_zocalo_results_correctly_identified( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=False + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=False ) await zocalo_results.connect() await zocalo_results.stage() zocalo_results._raw_results_received.get = MagicMock( - return_value={"ispyb_ids": {"test": 0}, "results": []} + return_value={"recipe_parameters": {"test": 0}, "results": []} ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.info.assert_has_calls( @@ -299,7 +298,7 @@ async def test_source_of_zocalo_results_correctly_identified( ) zocalo_results._raw_results_received.get = MagicMock( - return_value={"ispyb_ids": {"gpu": True}, "results": []} + return_value={"recipe_parameters": {"gpu": True}, "results": []} ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.info.assert_has_calls( @@ -307,30 +306,18 @@ async def test_source_of_zocalo_results_correctly_identified( ) -def test_compare_cpu_and_gpu_results_warns_correctly(): - dict1 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 2]]} - dict2 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 2]]} - assert not get_dict_differences(dict1, "dict1", dict2, "dict2") - dict1 = {"key1": [2, 2, 3], "key2": "test", "key3": [[1, 2], [1, 4]]} - dict2 = {"key1": [1, 2, 3], "key2": "test", "key3": [[1, 2], [1, 3]]} - assert ( - get_dict_differences(dict1, "dict1", dict2, "dict2") - == f"Results differed in key1: dict1 contains {dict1['key1']} while dict2 contains {dict2['key1']} \nResults differed in key3: dict1 contains {dict1['key3']} while dict2 contains {dict2['key3']} \n" - ) - - @patch("dodal.devices.zocalo.zocalo_results.LOGGER") @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) async def test_if_zocalo_results_timeout_from_one_source_then_warn( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=True ) await zocalo_results.connect() await zocalo_results.stage() zocalo_results._raw_results_received.get = MagicMock( - side_effect=[{"ispyb_ids": {"test": 0}, "results": []}, Empty] + side_effect=[{"recipe_parameters": {"test": 0}, "results": []}, Empty] ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.warning.assert_called_with( @@ -344,12 +331,12 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=True ) await zocalo_results.connect() await zocalo_results.stage() zocalo_results._raw_results_received.get = MagicMock( - return_value={"ispyb_ids": {"test": 0}, "results": []} + return_value={"recipe_parameters": {"test": 0}, "results": []} ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.warning.assert_called_with( @@ -361,37 +348,35 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( "dict1,dict2,output", [ ( - {"ispyb_ids": {"gpu": True}, "results": [{"test": 0}]}, - {"ispyb_ids": {}, "results": [{"test": 1}]}, - "Differences found between GPU and CPU results:\n values_changed: {\"root['test']\": {'CPU': 1, 'GPU': 0}}\n", + {"recipe_parameters": {"gpu": True}, "results": [{"test": 0}]}, + {"recipe_parameters": {}, "results": [{"test": 1}]}, + "Zocalo results from GPU and CPU are not identical.\n Results from GPU: {'test': 0}\n Results from CPU: {'test': 1}", ), ( { - "ispyb_ids": {"gpu": True}, + "recipe_parameters": {"gpu": True}, "results": [{"test": [[1, 2 + 1e-6, 3], [1, 2, 3]]}], }, - {"ispyb_ids": {}, "results": [{"test": [[1, 2, 3], [1, 2, 3]]}]}, + {"recipe_parameters": {}, "results": [{"test": [[1, 2, 3], [1, 2, 3]]}]}, None, ), ( { - "ispyb_ids": {"gpu": True}, + "recipe_parameters": {"gpu": True}, "results": [ {"test": [[1, 2 + 1e-6, 3], [1, 2, 3]], "extra_key": "test"} ], }, - {"ispyb_ids": {}, "results": [{"test": [[1, 2, 3], [1, 2, 3]]}]}, - "Differences found between GPU and CPU results:\n dictionary_item_removed: SetOrdered([\"root['extra_key']\"])\n", + {"recipe_parameters": {}, "results": [{"test": [[1, 2, 3], [1, 2, 3]]}]}, + "Zocalo results from GPU and CPU are not identical.\n Results from GPU: {'test': [[1, 2.000001, 3], [1, 2, 3]], 'extra_key': 'test'}\n Results from CPU: {'test': [[1, 2, 3], [1, 2, 3]]}", ), ( { - "ispyb_ids": {"gpu": False}, - "results": [ - {"test": [[1, 2 + 1e-6, 3], [1, 2, 3]], "extra_key": "test"} - ], + "recipe_parameters": {"gpu": False}, + "results": [{"test": [[1, 2 + 1e-6, 3], [1, 2, 3]]}], }, - {"ispyb_ids": {}, "results": [{"test": [[1, 3, 3], [1, 2, 3]]}]}, - "Differences found between CPU and GPU results:\n dictionary_item_removed: SetOrdered([\"root['extra_key']\"])\n values_changed: {\"root['test'][0][1]\": {'GPU': 3, 'CPU': 2.000001}}\n", + {"recipe_parameters": {}, "results": [{"test": [[1, 3, 3], [1, 2, 3]]}]}, + "Zocalo results from CPU and CPU are not identical.\n Results from CPU: {'test': [[1, 2.000001, 3], [1, 2, 3]]}\n Results from CPU: {'test': [[1, 3, 3], [1, 2, 3]]}", ), ], ) @@ -401,7 +386,7 @@ async def test_warning_if_results_are_different( mock_connection, mock_logger, RE: RunEngine, dict1, dict2, output ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=True ) await zocalo_results.connect() await zocalo_results.stage() @@ -422,7 +407,7 @@ async def test_if_zocalo_results_timeout_before_any_results_then_error( mock_connection, RE: RunEngine ): zocalo_results = ZocaloResults( - name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu_zocalo=True + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=True ) await zocalo_results.connect() await zocalo_results.stage() @@ -443,7 +428,7 @@ async def test_gpu_results_ignored_if_toggle_disabled( name="zocalo", zocalo_environment="dev_artemis", timeout_s=2, - use_cpu_and_gpu_zocalo=False, + use_cpu_and_gpu=False, ) recipe_wrapper = MagicMock() From 560c600e0a2d8c0a5f1b61d91c3e19d3a4119905 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Fri, 6 Sep 2024 10:13:44 +0100 Subject: [PATCH 11/17] Spell better --- src/dodal/devices/zocalo/zocalo_results.py | 4 ++-- tests/devices/unit_tests/test_zocalo_results.py | 2 +- tests/fake_zocalo/__main__.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 2ef36dca87..69c3b3ad18 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -169,7 +169,7 @@ def _clear_old_results(self): @AsyncStatus.wrap async def stage(self): """Stages the Zocalo device by: subscribing to the queue, doing a background - sleep for a few seconds to wait for any stale messages to be recieved, then + sleep for a few seconds to wait for any stale messages to be received, then clearing the queue. Plans using this device should wait on ZOCALO_STAGE_GROUP before triggering processing for the experiment""" @@ -216,7 +216,7 @@ async def trigger(self): # Wait for results from CPU and GPU, warn and continue if one timed out, error if both time out if self.use_cpu_and_gpu: if source_of_first_results == ZocaloSource.CPU: - LOGGER.warning("Recieved zocalo results from CPU before GPU") + LOGGER.warning("received zocalo results from CPU before GPU") raw_results_two_sources = [raw_results] try: raw_results_two_sources.append( diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 78b2935408..15fa07cbd6 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -340,7 +340,7 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.warning.assert_called_with( - "Recieved zocalo results from CPU before GPU" + "received zocalo results from CPU before GPU" ) diff --git a/tests/fake_zocalo/__main__.py b/tests/fake_zocalo/__main__.py index 65116e181d..e99e1a75e7 100644 --- a/tests/fake_zocalo/__main__.py +++ b/tests/fake_zocalo/__main__.py @@ -115,7 +115,7 @@ def main() -> None: def on_request(ch: BlockingChannel, method, props, body): print( - f"recieved message: \n properties: \n\n {method} \n\n {props} \n\n{body}\n" + f"received message: \n properties: \n\n {method} \n\n {props} \n\n{body}\n" ) try: message = json.loads(body) From fc3bd5c1697efbd55df196e2e8f6d24f6e73ee47 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Fri, 6 Sep 2024 11:37:51 +0100 Subject: [PATCH 12/17] improve test --- src/dodal/devices/util/lookup_tables.py | 2 +- src/dodal/devices/zocalo/zocalo_results.py | 2 +- .../devices/unit_tests/test_zocalo_results.py | 40 ++++++++++++++----- tests/fake_zocalo/__main__.py | 2 +- 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/dodal/devices/util/lookup_tables.py b/src/dodal/devices/util/lookup_tables.py index d920aa8eac..bc5cae4d99 100644 --- a/src/dodal/devices/util/lookup_tables.py +++ b/src/dodal/devices/util/lookup_tables.py @@ -27,7 +27,7 @@ async def energy_distance_table(lookup_table_path: str) -> np.ndarray: # Slight cheat to make the file IO async, numpy doesn't do any real IO now, just # decodes the text - async with aiofiles.open(lookup_table_path, "r") as stream: + async with aiofiles.open(lookup_table_path) as stream: raw_table = await stream.read() return loadtxt(StringIO(raw_table), comments=["#", "Units"]) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 69c3b3ad18..37c7e72d1b 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -216,7 +216,7 @@ async def trigger(self): # Wait for results from CPU and GPU, warn and continue if one timed out, error if both time out if self.use_cpu_and_gpu: if source_of_first_results == ZocaloSource.CPU: - LOGGER.warning("received zocalo results from CPU before GPU") + LOGGER.warning("Received zocalo results from CPU before GPU") raw_results_two_sources = [raw_results] try: raw_results_two_sources.append( diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index 15fa07cbd6..e1a1116100 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -15,6 +15,7 @@ NoZocaloSubscription, XrcResult, ZocaloResults, + ZocaloSource, get_processing_result, ) @@ -207,7 +208,7 @@ async def test_zocalo_results_trigger_log_message( zocalo_results = ZocaloResults( name="zocalo", zocalo_environment="dev_artemis", - timeout_s=2, + timeout_s=0, use_cpu_and_gpu=True, ) @@ -243,7 +244,11 @@ def zocalo_plan(): RE(zocalo_plan()) mock_logger.info.assert_has_calls( - [call("Zocalo results from CPU processing: found 1 crystals.")] + [ + call( + f"Zocalo results from {ZocaloSource.CPU.value} processing: found 1 crystals." + ) + ] ) @@ -294,7 +299,11 @@ async def test_source_of_zocalo_results_correctly_identified( ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.info.assert_has_calls( - [call("Zocalo results from CPU processing: found 0 crystals.")] + [ + call( + f"Zocalo results from {ZocaloSource.CPU.value} processing: found 0 crystals." + ) + ] ) zocalo_results._raw_results_received.get = MagicMock( @@ -321,7 +330,7 @@ async def test_if_zocalo_results_timeout_from_one_source_then_warn( ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.warning.assert_called_with( - "Zocalo results from GPU timed out. Using results from CPU" + f"Zocalo results from GPU timed out. Using results from {ZocaloSource.CPU.value}" ) @@ -340,7 +349,7 @@ async def test_if_cpu_results_arrive_before_gpu_then_warn( ) RE(bps.trigger(zocalo_results, wait=False)) mock_logger.warning.assert_called_with( - "received zocalo results from CPU before GPU" + f"Received zocalo results from {ZocaloSource.CPU.value} before {ZocaloSource.GPU.value}" ) @@ -416,23 +425,27 @@ async def test_if_zocalo_results_timeout_before_any_results_then_error( await zocalo_results.trigger() +@pytest.mark.parametrize( + "gpu,", + [(True), (False)], +) @patch("dodal.devices.zocalo.zocalo_results.LOGGER") @patch( "dodal.devices.zocalo.zocalo_results.workflows.recipe.wrap_subscribe", autospec=True ) @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", new=MagicMock()) -async def test_gpu_results_ignored_if_toggle_disabled( - mock_wrap_subscribe, mock_logger, RE: RunEngine +async def test_gpu_results_ignored_and_cpu_results_used_if_toggle_disabled( + mock_wrap_subscribe, mock_logger, RE: RunEngine, gpu: bool ): zocalo_results = ZocaloResults( name="zocalo", zocalo_environment="dev_artemis", - timeout_s=2, + timeout_s=0, use_cpu_and_gpu=False, ) recipe_wrapper = MagicMock() - recipe_wrapper.recipe_step = {"parameters": {"gpu": True}} + recipe_wrapper.recipe_step = {"parameters": {"gpu": gpu}} def zocalo_plan(): yield from bps.stage(zocalo_results) @@ -460,6 +473,13 @@ def zocalo_plan(): }, ) yield from bps.trigger(zocalo_results) - mock_logger.warning.assert_called_with("Timed out waiting for zocalo results!") + if gpu: + mock_logger.warning.assert_called_with( + "Timed out waiting for zocalo results!" + ) + else: + mock_logger.info.assert_called_with( + f"Zocalo results from {ZocaloSource.CPU.value} processing: found 1 crystals." + ) RE(zocalo_plan()) diff --git a/tests/fake_zocalo/__main__.py b/tests/fake_zocalo/__main__.py index e99e1a75e7..fb53596422 100644 --- a/tests/fake_zocalo/__main__.py +++ b/tests/fake_zocalo/__main__.py @@ -115,7 +115,7 @@ def main() -> None: def on_request(ch: BlockingChannel, method, props, body): print( - f"received message: \n properties: \n\n {method} \n\n {props} \n\n{body}\n" + f"Received message: \n properties: \n\n {method} \n\n {props} \n\n{body}\n" ) try: message = json.loads(body) From bd35cabef63ece3a7765b2f3d623510a783690ba Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Fri, 6 Sep 2024 15:31:32 +0100 Subject: [PATCH 13/17] better comment --- src/dodal/devices/zocalo/zocalo_results.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index f56186c9f4..40fcfb743e 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -69,7 +69,7 @@ def bbox_size(result: XrcResult): def get_dict_differences( dict1: dict, dict1_source: str, dict2: dict, dict2_source: str ) -> str | None: - """Returns dict1 and dict2 as a string if differences between them are found greater than a + """Returns a string containing dict1 and dict2 if there are differences between them, greater than a 1e-5 tolerance. If dictionaries are identical, return None""" diff = DeepDiff(dict1, dict2, math_epsilon=1e-5, ignore_numeric_type_changes=True) From 0c0b45d2f042a49ef458aee15ceb799d100826fa Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Fri, 6 Sep 2024 17:50:45 +0100 Subject: [PATCH 14/17] fix linting --- tests/devices/unit_tests/test_zocalo_results.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index af7d906ff2..c5cfc0f7e9 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -8,7 +8,6 @@ from bluesky.run_engine import RunEngine from bluesky.utils import FailedStatus from ophyd_async.core import AsyncStatus -from workflows.recipe import RecipeWrapper from dodal.devices.zocalo.zocalo_results import ( ZOCALO_READING_PLAN_NAME, From ad506a1e2fbe5d094cb6bb6f143b67160d19203a Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Fri, 6 Sep 2024 18:04:00 +0100 Subject: [PATCH 15/17] Move deepdiff to regular dependancy --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index f42fb2df3d..e81b532cb4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ dependencies = [ "aiofiles", "aiohttp", "redis", + "deepdiff", ] dynamic = ["version"] @@ -67,7 +68,6 @@ dev = [ "types-mock", "types-PyYAML", "types-aiofiles", - "deepdiff", ] [project.scripts] From 362a89ea8bc943b2d18e55b438b7e7ea3e0ffd87 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Thu, 19 Sep 2024 14:35:53 +0100 Subject: [PATCH 16/17] Always use GPU results --- src/dodal/devices/zocalo/zocalo_results.py | 25 ++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/dodal/devices/zocalo/zocalo_results.py b/src/dodal/devices/zocalo/zocalo_results.py index 40fcfb743e..e47ac73ff3 100644 --- a/src/dodal/devices/zocalo/zocalo_results.py +++ b/src/dodal/devices/zocalo/zocalo_results.py @@ -217,7 +217,7 @@ async def trigger(self): raw_results = self._raw_results_received.get(timeout=self.timeout_s) source_of_first_results = source_from_results(raw_results) - # Wait for results from CPU and GPU, warn and continue if one timed out, error if both time out + # Wait for results from CPU and GPU, warn and continue if only GPU times out. Error if CPU times out if self.use_cpu_and_gpu: if source_of_first_results == ZocaloSource.CPU: LOGGER.warning("Received zocalo results from CPU before GPU") @@ -240,18 +240,31 @@ async def trigger(self): if differences_str: LOGGER.warning(differences_str) - except Empty: + # Always use CPU results + raw_results = ( + raw_results_two_sources[0] + if source_of_first_results == ZocaloSource.CPU + else raw_results_two_sources[1] + ) + + except Empty as err: source_of_missing_results = ( ZocaloSource.CPU.value if source_of_first_results == ZocaloSource.GPU.value else ZocaloSource.GPU.value ) - LOGGER.warning( - f"Zocalo results from {source_of_missing_results} timed out. Using results from {source_of_first_results}" - ) + if source_of_missing_results == ZocaloSource.GPU.value: + LOGGER.warning( + f"Zocalo results from {source_of_missing_results} timed out. Using results from {source_of_first_results}" + ) + else: + LOGGER.error( + f"Zocalo results from {source_of_missing_results} timed out and GPU results not yet reliable" + ) + raise err LOGGER.info( - f"Zocalo results from {source_of_first_results} processing: found {len(raw_results['results'])} crystals." + f"Zocalo results from {ZocaloSource.CPU.value} processing: found {len(raw_results['results'])} crystals." ) # Sort from strongest to weakest in case of multiple crystals await self._put_results( From de5cc4b3b910c2a9985c64108df7694fa509758d Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Thu, 19 Sep 2024 15:21:13 +0100 Subject: [PATCH 17/17] Add new test --- .../devices/unit_tests/test_zocalo_results.py | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/tests/devices/unit_tests/test_zocalo_results.py b/tests/devices/unit_tests/test_zocalo_results.py index c5cfc0f7e9..73c5bc5b7f 100644 --- a/tests/devices/unit_tests/test_zocalo_results.py +++ b/tests/devices/unit_tests/test_zocalo_results.py @@ -306,18 +306,32 @@ async def test_source_of_zocalo_results_correctly_identified( ] ) + +@patch("dodal.devices.zocalo.zocalo_results.LOGGER") +@patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) +async def test_if_zocalo_results_timeout_from_gpu_then_warn( + mock_connection, mock_logger, RE: RunEngine +): + zocalo_results = ZocaloResults( + name="zocalo", zocalo_environment="dev_artemis", use_cpu_and_gpu=True + ) + await zocalo_results.connect() + await zocalo_results.stage() zocalo_results._raw_results_received.get = MagicMock( - return_value={"recipe_parameters": {"gpu": True}, "results": []} + side_effect=[ + {"recipe_parameters": {"test": 0}, "results": []}, + Empty, + ] ) RE(bps.trigger(zocalo_results, wait=False)) - mock_logger.info.assert_has_calls( - [call("Zocalo results from GPU processing: found 0 crystals.")] + mock_logger.warning.assert_called_with( + f"Zocalo results from GPU timed out. Using results from {ZocaloSource.CPU.value}" ) @patch("dodal.devices.zocalo.zocalo_results.LOGGER") @patch("dodal.devices.zocalo.zocalo_results._get_zocalo_connection", autospec=True) -async def test_if_zocalo_results_timeout_from_one_source_then_warn( +async def test_if_zocalo_results_from_gpu_but_not_cpu_then_error( mock_connection, mock_logger, RE: RunEngine ): zocalo_results = ZocaloResults( @@ -326,12 +340,13 @@ async def test_if_zocalo_results_timeout_from_one_source_then_warn( await zocalo_results.connect() await zocalo_results.stage() zocalo_results._raw_results_received.get = MagicMock( - side_effect=[{"recipe_parameters": {"test": 0}, "results": []}, Empty] - ) - RE(bps.trigger(zocalo_results, wait=False)) - mock_logger.warning.assert_called_with( - f"Zocalo results from GPU timed out. Using results from {ZocaloSource.CPU.value}" + side_effect=[ + {"recipe_parameters": {"test": 0, "gpu": True}, "results": []}, + Empty, + ] ) + with pytest.raises(NoResultsFromZocalo): + await zocalo_results.trigger() @patch("dodal.devices.zocalo.zocalo_results.LOGGER")