Move XRC callbacks to common #652

olliesilvester · 2024-11-15T15:55:56Z

Fixes #214 and #215

Moves lots of the external_interaction code into common. Some main bits:

A common ThreeDGridScan which HyperionThreeDGridScan now inherits
Functions for talking to Nexus and ISPyB moved to common
All XRC and common callbacks moved to mx_bluesky common
XRC nexus callbacks now needs to know what type of parameters are being used. It accepts ThreeDGridScan any any children of this type

Probably difficult to review, sorry

codecov · 2024-11-15T15:58:36Z

Codecov Report

Attention: Patch coverage is 97.83550% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.98%. Comparing base (9aa3698) to head (590b6c6).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #652      +/-   ##
==========================================
- Coverage   85.00%   84.98%   -0.02%     
==========================================
  Files          98       96       -2     
  Lines        6548     6546       -2     
==========================================
- Hits         5566     5563       -3     
- Misses        982      983       +1

Components	Coverage Δ
i24 SSX	`69.59% <ø> (ø)`
hyperion	`96.23% <100.00%> (-0.56%)`	⬇️
other	`96.97% <96.81%> (+1.99%)`	⬆️

olliesilvester · 2024-11-15T17:12:23Z

See #655 for sorting out the tests. Annoyingly, we might want to do this before merging, so we can be sure that this PR didn't break anything, but I don't want to hold up @noemifrisina 's work

noemifrisina · 2024-11-18T09:40:11Z

See #655 for sorting out the tests. Annoyingly, we might want to do this before merging, so we can be sure that this PR didn't break anything, but I don't want to hold up @noemifrisina 's work

If all the important bits have been moved and sorting out the tests is all that's left to this one, I think it would be worth it to sort out the tests before merging this. While that might take awhile, as you said at least we'd be reasonably sure things are still working after this PR. For my work, I can just make a new branch off this one in the meantime and make a start

olliesilvester · 2024-11-18T09:41:59Z

See #655 for sorting out the tests. Annoyingly, we might want to do this before merging, so we can be sure that this PR didn't break anything, but I don't want to hold up @noemifrisina 's work

If all the important bits have been moved and sorting out the tests is all that's left to this one, I think it would be worth it to sort out the tests before merging this. While that might take awhile, as you said at least we'd be reasonably sure things are still working after this PR. For my work, I can just make a new branch off this one in the meantime and make a start

Cool, as long as it's not holding you up

noemifrisina · 2024-11-18T10:33:45Z

src/mx_bluesky/common/external_interaction/callbacks/xray_centre/nexus_callback.py


 if TYPE_CHECKING:
    from event_model.documents import Event, EventDescriptor, RunStart

+T = TypeVar("T", bound="ThreeDGridScan")
+

 class GridscanNexusFileCallback(PlanReactiveCallback):


Is there a particular reason why we're moving this particular nexus callback and not the rotation one too? Would it be worth making a nore general one from which both this and the rotation one in hyperion - and any other ones we need in the future - can inherit?
This would be a separate PR in any case, only trying to understand the what's going on...

Yeah, there's no reason other than trying to not do everything at once. For now, I only need the bits in XRC, so I've only moved the parts needed for this

Thanks. Sounds good to me, maybe add an issue if it doesn't exist already - or a comment about this on an existing one?

It should be possible for the nexus callbacks to be much more generic. I think we could even get away with one for all usecases see #364

Yeah, as long as no one is waiting for this PR, I can include that here

I think that's going to be quite a big refactor, I would do it in a new PR

noemifrisina · 2024-11-18T10:34:05Z

src/mx_bluesky/common/parameters/gridscan.py

@@ -21,6 +28,7 @@
    HardwareConstants,
 )
 from mx_bluesky.common.parameters.robot_load import RobotLoadAndEnergyChange
+from mx_bluesky.hyperion.parameters.constants import CONST


Nit: maybe import CONST as hyperion_const or something?

On a first read this threw me a bit because I was assuming CONST would be common and not hyperion specific...

Ah, good catch, I shouldn't be importing anything from Hyperion to mx_bluesky/common. We're meant to have linting rules which catches this, I'll check why it didn't work

noemifrisina · 2024-11-18T10:38:27Z

On a first go through, looks okay, thank you. Will wait to finish review and approve till you're done with the tests. I'd say the fact that they pass right now is a good start though :)

DominicOram

Couple of comments between meetings, not a full review

DominicOram · 2024-11-18T11:38:28Z

src/mx_bluesky/hyperion/parameters/gridscan.py

-class HyperionGridCommon(GridCommon, WithHyperionFeatures):
-    # This class only exists so that we can properly select enable_dev_shm. Remove in
-    # https://github.com/DiamondLightSource/hyperion/issues/1395"""
+class HyperionThreeDGridScan(


Should: ThreeDGridScan already inherits from:

GridCommon, SpecifiedGrid, SplitScan, WithOptionalEnergyChange,

So we can drop the others here

DominicOram · 2024-11-18T11:40:25Z

src/mx_bluesky/common/external_interaction/callbacks/xray_centre/nexus_callback.py

            )
-            parameters = HyperionThreeDGridScan.model_validate_json(hyperion_params)
+            parameters = self.param_type.model_validate_json(mx_bluesky_parameters)


Could: Can we just always use do ThreeDGridScan.model_validate_json here? We don't need all the additional stuff from Hyperion anyway? Maybe we can get away with an even broader base class e.g. in ispyb_callback we just convert to a GridCommon. Basically the more broader the baseclass the more reusable this callback should be

olliesilvester requested a review from noemifrisina November 15, 2024 16:20

olliesilvester added 7 commits November 15, 2024 16:20

Move callbacks

83b9b5e

Update import locations

49e6857

Fix tests

b200438

Move some tests

c6cd4cf

remove accidentally added folder

b097eda

Create common 3d grid and move XRC callbacks to commo

958da82

Move test for nexus utils

972cb91

olliesilvester force-pushed the move_ispyb_callbacks_to_common branch from bde8d7a to 972cb91 Compare November 15, 2024 16:27

olliesilvester marked this pull request as draft November 15, 2024 16:27

olliesilvester added 2 commits November 15, 2024 16:38

Move things around more

e86a76b

Allow nexus callback to use different grid parameters

2176b60

olliesilvester mentioned this pull request Nov 15, 2024

Correctly organise tests in external interaction #655

Open

olliesilvester added 2 commits November 15, 2024 17:13

Rename some things from Hyperion to MX-Bluesky

fb48ae7

Merge branch 'main' into move_ispyb_callbacks_to_common

590b6c6

olliesilvester marked this pull request as ready for review November 15, 2024 17:33

olliesilvester changed the title ~~Begin moving callbacks to common~~ Move XRC callbacks to common Nov 15, 2024

olliesilvester requested review from rtuck99, DominicOram and dperl-dls November 15, 2024 18:52

noemifrisina reviewed Nov 18, 2024

View reviewed changes

DominicOram reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move XRC callbacks to common #652

Move XRC callbacks to common #652

olliesilvester commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

olliesilvester commented Nov 15, 2024

noemifrisina commented Nov 18, 2024

olliesilvester commented Nov 18, 2024

noemifrisina Nov 18, 2024

olliesilvester Nov 18, 2024

noemifrisina Nov 18, 2024

DominicOram Nov 18, 2024

olliesilvester Nov 18, 2024

DominicOram Nov 18, 2024

noemifrisina Nov 18, 2024

olliesilvester Nov 18, 2024

noemifrisina commented Nov 18, 2024

DominicOram left a comment

DominicOram Nov 18, 2024

DominicOram Nov 18, 2024

Move XRC callbacks to common #652

Are you sure you want to change the base?

Move XRC callbacks to common #652

Conversation

olliesilvester commented Nov 15, 2024 • edited Loading

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

olliesilvester commented Nov 15, 2024

noemifrisina commented Nov 18, 2024

olliesilvester commented Nov 18, 2024

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

noemifrisina commented Nov 18, 2024

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

olliesilvester commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading