ZocaloResults: add parameter to use results from GPU #763

olliesilvester · 2024-08-29T13:56:45Z

Fixes #559

See related mx_bluesky PR: DiamondLightSource/mx-bluesky#445

Instructions to reviewer on how to test:

Check this PR adds and tests the following behavior:

If the GPU is toggled on, then:

Wait for results twice
If CPU arrives before GPU, then warn and continue
if CPU results differ from GPU within a tolerance, log the two results and continue
if GPU times out but CPU results arrive, warn and continue
if GPU results arrive but CPU results timeout, error
Always use CPU results after comparing the two
If it's off:
Ignore results from GPU

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

src/dodal/devices/zocalo/zocalo_results.py

Co-authored-by: Nicholas Devenish <[email protected]>

codecov · 2024-08-29T15:56:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.99%. Comparing base (ee5da2b) to head (45a3bc1).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   94.92%   94.99%   +0.07%     
==========================================
  Files         115      115              
  Lines        4687     4719      +32     
==========================================
+ Hits         4449     4483      +34     
+ Misses        238      236       -2

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

src/dodal/devices/zocalo/zocalo_results.py

ndevenish · 2024-08-29T19:04:58Z

Do you have pre-commits installed/activated? It looks like it hasn't been linted/auto-formatted (the unspaced comment #Only is a tell)

src/dodal/devices/zocalo/zocalo_results.py

olliesilvester · 2024-09-03T13:59:28Z

tests/devices/unit_tests/test_zocalo_results.py

+    await zocalo_results.stage()
+    zocalo_results._raw_results_received.get = MagicMock(side_effect=Empty)
+    with pytest.raises(NoResultsFromZocalo):
+        await zocalo_results.trigger()


There was a strange thing here where this test would fail if I used RE(bps.trigger(zocalo_results) instead, even though debugging the pytest showed the exception was being raised

The RE might swallow the exception or transform it?

DominicOram

Great, thank you. Couple of comments in the code.

src/dodal/devices/zocalo/zocalo_results.py

DominicOram · 2024-09-04T16:36:58Z

src/dodal/devices/zocalo/zocalo_results.py

@@ -79,6 +92,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


Nit: This device is already has zocalo in the name so having this variable with zocalo in is a bit redundant. It would also be good to have a docstring on it

src/dodal/devices/zocalo/zocalo_results.py

DominicOram · 2024-09-04T16:43:09Z

src/dodal/devices/zocalo/zocalo_results.py

+                )
+            else:
+                # Only add to queue if results are from CPU
+                if not recipe_parameters.get("gpu"):


Should: A unit test covering this would be good.

DominicOram · 2024-09-04T16:43:40Z

src/dodal/devices/zocalo/zocalo_results.py

+
+            if self.use_cpu_and_gpu_zocalo:
+                self._raw_results_received.put(
+                    {"results": results, "ispyb_ids": recipe_parameters}


Could: ispyb_ids isn't a great name any more as it contains other stuff.

DominicOram · 2024-09-04T16:49:48Z

tests/devices/unit_tests/test_zocalo_results.py

-        name="zocalo", zocalo_environment="dev_artemis", timeout_s=2
+        name="zocalo",
+        zocalo_environment="dev_artemis",
+        timeout_s=2,


Could: It's not great that we're waiting on this timeout for the test. Ideally we would mock it out so there's no wait on it but if not could we at least drop it down to as low as possible?

DominicOram · 2024-09-04T16:50:55Z

src/dodal/devices/zocalo/zocalo_results.py

+            # 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":
+                    LOGGER.warning("Recieved zocalo results from CPU before GPU")


Nit:

Suggested change

LOGGER.warning("Recieved zocalo results from CPU before GPU")

LOGGER.warning("Received zocalo results from CPU before GPU")

@dperl-dls and I combined have misspelt this word 10 times across mx_bluesky and dodal!

So have I tbf

DominicOram · 2024-09-04T16:55:51Z

src/dodal/devices/zocalo/zocalo_results.py

+
+                except Empty:
+                    LOGGER.warning(
+                        f"Zocalo results from {source_of_second_results} timed out. Using results from {source_of_first_results}"


@neil-i03 To confirm here, this will mean that if we have the GPU results turned on and we do not get a result from CPU zocalo within half the usual timeout time (180/2=90s) we will use the GPU result. Do we feel confident enough in trusting this or do we still want to only ever use the CPU result for now, even if it means we will timeout?

DominicOram · 2024-09-04T16:58:34Z

tests/devices/unit_tests/test_zocalo_results.py

+    await zocalo_results.stage()
+    zocalo_results._raw_results_received.get = MagicMock(side_effect=Empty)
+    with pytest.raises(NoResultsFromZocalo):
+        await zocalo_results.trigger()


The RE might swallow the exception or transform it?

DominicOram · 2024-09-04T16:59:01Z

tests/devices/unit_tests/test_zocalo_results.py

+    zocalo_results = ZocaloResults(
+        name="zocalo",
+        zocalo_environment="dev_artemis",
+        timeout_s=2,


As above, not ideal to be actually waiting on this timeout in tests

…ltiple_sources

DominicOram

Great, thank you!

ZocaloResults: add parameter to use results from GPU

22e7263

olliesilvester mentioned this pull request Aug 29, 2024

Add parameter to toggle zocalo using GPU results DiamondLightSource/hyperion#1548

Draft

Warn if CPU results arrived before GPU results

92af2e9

ndevenish reviewed Aug 29, 2024

View reviewed changes

src/dodal/devices/zocalo/zocalo_results.py Outdated Show resolved Hide resolved

olliesilvester and others added 2 commits August 29, 2024 16:25

Update src/dodal/devices/zocalo/zocalo_results.py

90e8385

Co-authored-by: Nicholas Devenish <[email protected]>

Correct typo

4871d73

ndevenish reviewed Aug 29, 2024

View reviewed changes

src/dodal/devices/zocalo/zocalo_results.py Outdated Show resolved Hide resolved

Update with new criteria and add tests

f181325

olliesilvester mentioned this pull request Sep 2, 2024

Zocalo results multiple sources DiamondLightSource/mx-bluesky#445

Merged

more tests for codecov

0632f5a

olliesilvester commented Sep 3, 2024

View reviewed changes

src/dodal/devices/zocalo/zocalo_results.py Show resolved Hide resolved

remove extra comments

9b1b4c9

olliesilvester marked this pull request as ready for review September 3, 2024 08:32

Change toggle name

99b8f49

olliesilvester requested a review from DominicOram September 3, 2024 08:44

olliesilvester commented Sep 3, 2024

View reviewed changes

DominicOram requested changes Sep 4, 2024

View reviewed changes

olliesilvester and others added 8 commits September 5, 2024 15:09

use deepdiff to get differences between gpu and cpu results

149c2bd

Review response and simplify deepdiff

984a134

Spell better

560c600

improve test

fc3bd5c

Merge branch 'main' into 559_zocalo_results_multiple_sources

7d6c7f8

better comment

bd35cab

Merge remote-tracking branch 'origin/main' into 559_zocalo_results_mu…

71f6e13

…ltiple_sources

fix linting

0c0b45d

olliesilvester requested a review from DominicOram September 6, 2024 16:58

olliesilvester added 2 commits September 6, 2024 18:04

Move deepdiff to regular dependancy

ad506a1

Always use GPU results

362a89e

olliesilvester and others added 3 commits September 19, 2024 14:38

Merge remote-tracking branch 'origin/main' into 559_zocalo_results_mu…

286b7bc

…ltiple_sources

Add new test

de5cc4b

Merge branch 'main' into 559_zocalo_results_multiple_sources

ff848a6

DominicOram approved these changes Sep 23, 2024

View reviewed changes

Merge branch 'main' into 559_zocalo_results_multiple_sources

45a3bc1

DominicOram merged commit eece064 into main Sep 23, 2024
18 checks passed

DominicOram deleted the 559_zocalo_results_multiple_sources branch September 23, 2024 09:00

DominicOram mentioned this pull request Sep 23, 2024

Zocalo tests are slow #798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZocaloResults: add parameter to use results from GPU #763

ZocaloResults: add parameter to use results from GPU #763

olliesilvester commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

ndevenish commented Aug 29, 2024

olliesilvester Sep 3, 2024

DominicOram Sep 4, 2024

DominicOram left a comment

DominicOram Sep 4, 2024

DominicOram Sep 4, 2024

DominicOram Sep 4, 2024

DominicOram Sep 4, 2024

DominicOram Sep 4, 2024

olliesilvester Sep 6, 2024

DominicOram Sep 6, 2024

DominicOram Sep 4, 2024

DominicOram Sep 4, 2024

DominicOram Sep 4, 2024

DominicOram left a comment

	LOGGER.warning("Recieved zocalo results from CPU before GPU")
	LOGGER.warning("Received zocalo results from CPU before GPU")

ZocaloResults: add parameter to use results from GPU #763

ZocaloResults: add parameter to use results from GPU #763

Conversation

olliesilvester commented Aug 29, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

ndevenish commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

olliesilvester commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading