Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZocaloResults: add parameter to use results from GPU #763

Merged
merged 22 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
22e7263
ZocaloResults: add parameter to use results from GPU
olliesilvester Aug 29, 2024
92af2e9
Warn if CPU results arrived before GPU results
olliesilvester Aug 29, 2024
90e8385
Update src/dodal/devices/zocalo/zocalo_results.py
olliesilvester Aug 29, 2024
4871d73
Correct typo
olliesilvester Aug 29, 2024
f181325
Update with new criteria and add tests
olliesilvester Sep 2, 2024
0632f5a
more tests for codecov
olliesilvester Sep 2, 2024
9b1b4c9
remove extra comments
olliesilvester Sep 3, 2024
99b8f49
Change toggle name
olliesilvester Sep 3, 2024
149c2bd
use deepdiff to get differences between gpu and cpu results
olliesilvester Sep 5, 2024
984a134
Review response and simplify deepdiff
olliesilvester Sep 6, 2024
560c600
Spell better
olliesilvester Sep 6, 2024
fc3bd5c
improve test
olliesilvester Sep 6, 2024
7d6c7f8
Merge branch 'main' into 559_zocalo_results_multiple_sources
olliesilvester Sep 6, 2024
bd35cab
better comment
olliesilvester Sep 6, 2024
71f6e13
Merge remote-tracking branch 'origin/main' into 559_zocalo_results_mu…
olliesilvester Sep 6, 2024
0c0b45d
fix linting
olliesilvester Sep 6, 2024
ad506a1
Move deepdiff to regular dependancy
olliesilvester Sep 6, 2024
362a89e
Always use GPU results
olliesilvester Sep 19, 2024
286b7bc
Merge remote-tracking branch 'origin/main' into 559_zocalo_results_mu…
olliesilvester Sep 19, 2024
de5cc4b
Add new test
olliesilvester Sep 19, 2024
ff848a6
Merge branch 'main' into 559_zocalo_results_multiple_sources
olliesilvester Sep 20, 2024
45a3bc1
Merge branch 'main' into 559_zocalo_results_multiple_sources
DominicOram Sep 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/dodal/devices/zocalo/zocalo_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
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,
Expand All @@ -71,6 +71,7 @@
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]
Expand All @@ -79,6 +80,7 @@
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"
Expand Down Expand Up @@ -165,7 +167,10 @@
)

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 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")

Check warning on line 172 in src/dodal/devices/zocalo/zocalo_results.py

View check run for this annotation

Codecov / codecov/patch

src/dodal/devices/zocalo/zocalo_results.py#L172

Added line #L172 was not covered by tests
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(
Expand Down Expand Up @@ -237,9 +242,18 @@
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(

Check warning on line 247 in src/dodal/devices/zocalo/zocalo_results.py

View check run for this annotation

Codecov / codecov/patch

src/dodal/devices/zocalo/zocalo_results.py#L247

Added line #L247 was not covered by tests
{"results": results, "ispyb_ids": recipe_parameters}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)
else:
#Only add to queue if results are from CPU
if recipe_parameters.get('gpu') == None:
olliesilvester marked this conversation as resolved.
Show resolved Hide resolved
self._raw_results_received.put(
{"results": results, "ispyb_ids": recipe_parameters}
)


subscription = workflows.recipe.wrap_subscribe(
self.transport,
Expand Down
7 changes: 5 additions & 2 deletions tests/devices/unit_tests/test_zocalo_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand All @@ -235,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)
Expand Down
Loading