Initialize callbacks using params emitted in Bluesky documents by d-perl · Pull Request #959 · DiamondLightSource/hyperion

d-perl · 2023-11-07T10:26:44Z

Fixes #950
Fixes #372

Callback classes now all get their params from the "hyperion_internal_params" emitted in the outer start document of each plan.

Adds an IspybIds to clean up some type checking issues

80% of line count is just tests being put into classes so I could mock stuff better

To test:

Run tests

and replace plan names with constants

DominicOram

Good start, thanks! I love pulling all the strings into constants. Some comments in code.

Maybe I'm being dumb but why can't we use a normal constructor instead of the setup method?

DominicOram · 2023-11-09T16:38:32Z

+        self.params: GridscanInternalParameters | RotationInternalParameters | None = (
+            None
+        )
+        self.ispyb: StoreRotationInIspyb | Store3DGridscanInIspyb | Store2DGridscanInIspyb


Should: Don't we want this to be anything that inherits from StoreInIspyb?

base StoreInIspyb doesn't have all the attributes, type checker will be mad

Which attributes? The only errors I see when I set it to StoreInIspyb are the arguments/return types on begin_deposition, which is a legitimate issue in the StoreInIspyb class

Ok, I fixed it properly :)

DominicOram · 2023-11-09T16:40:37Z

+        assert isinstance(self.params, GridscanInternalParameters) or isinstance(
+            self.params, RotationInternalParameters
+        ), "ISPyB handler params set with wrong type"


Should: I don't think this is very pythonic, we should just try and use it. Also, mypy should pick it up hopefully

DominicOram · 2023-11-09T16:42:52Z


    def start(self, doc: dict):
+        if doc.get("subplan_name") == ROTATION_OUTER_PLAN:
+            self.run_uid = doc.get("uid")


Should: run_uid is never used, remove it?

DominicOram · 2023-11-09T16:50:27Z

+                if self.params.experiment_params.is_3d_grid_scan
+                else Store2DGridscanInIspyb(self.ispyb_config, self.params)
+            )
+            self.run_start_uid = doc.get("uid")


Should: run_start_uid is never used, can we remove it?

DominicOram · 2023-11-09T16:57:26Z

+            self.ispyb: StoreRotationInIspyb = StoreRotationInIspyb(
+                self.ispyb_config, self.params
+            )
+        self.ispyb_ids: tuple[int, int] | tuple[None, None] = (None, None)


Should: Do we want to reset the ids for every start document we get? Why does the grid ispyb callback do this on init rather than on start? Feels like they should have the same behaviour

good point, but they will diverge soon, when the rotation scan one should check if it should be reset at all

DominicOram · 2023-11-09T17:31:02Z

+            assert (
+                self.ispyb.params is not None
+            ), "ISPyB handler attached to Zocalo handler did not recieve parameters"
+            zocalo_environment = self.ispyb.params.hyperion_params.zocalo_environment


Should: Can we just get this out of the metadata rather than the ispyb object?

DominicOram · 2023-11-09T17:32:23Z

+            self.zocalo_interactor = ZocaloInteractor(
+                self.ispyb.params.hyperion_params.zocalo_environment
+            )
+            self.grid_position_to_motor_position: Callable[
+                [ndarray], ndarray
+            ] = self.ispyb.params.experiment_params.grid_position_to_motor_position


Should: I don't like that we have to go through the ispyb object to get these, it seems really tightly coupled. Can we have a chat about a better way?

I didn't think it was a problem since we're pulling ispyb IDs from there anyway, but we can just get it from the parameters, that's fine

codecov · 2023-11-09T19:28:57Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (1f9bec6) 93.71% compared to head (2e50c4f) 93.79%.
Report is 13 commits behind head on main.

Files	Patch %	Lines
...erion/experiment_plans/flyscan_xray_centre_plan.py	83.33%	2 Missing ⚠️
...ernal_interaction/callbacks/ispyb_callback_base.py	92.30%	2 Missing ⚠️
...l_interaction/callbacks/rotation/ispyb_callback.py	88.23%	2 Missing ⚠️
...al_interaction/callbacks/plan_reactive_callback.py	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #959      +/-   ##
==========================================
+ Coverage   93.71%   93.79%   +0.07%     
==========================================
  Files          56       57       +1     
  Lines        2673     2755      +82     
==========================================
+ Hits         2505     2584      +79     
- Misses        168      171       +3

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

DominicOram

Some missed comments from before. Mostly there now.

Maybe I'm being dumb but why can't we use a normal constructor instead of the setup method?

Question still stands.

DominicOram · 2023-11-13T17:28:14Z

+            {
+                "subplan_name": GRIDSCAN_OUTER_PLAN,
+                "hyperion_internal_parameters": test_fgs_params.json(),
+            }


Nit: Pull this into a variable and use below.

DominicOram · 2023-11-13T17:38:03Z

        self.ispyb_ids: tuple = (None, None, None)

+    def start(self, doc: dict):
+        if doc.get("subplan_name") == "run_gridscan_move_and_tidy":


@dperl-dls still valid comment

DominicOram · 2023-11-13T17:38:13Z

+            LOGGER.info(
+                "Nexus writer recieved start document with experiment parameters."
+            )


@dperl-dls still valid comment

DominicOram · 2023-11-13T17:51:53Z

        ispyb_config: str,
        experiment_type: str,
-        parameters: GridscanInternalParameters = None,
+        parameters: GridscanInternalParameters | None = None,


Should: When do we ever pass None into here in production? Do we need to allow it?

good spot, I cleaned up a bunch of other ones in here too

d-perl · 2023-11-14T08:34:53Z

why can't we use a normal constructor instead of the setup method?

The constructor of a dataclass requires values for the fields as arguments, but the zocalo callback needs to be instantiated with the ispyb handler as an argument; you can't set the fields of self anywhere (even in init) for a frozen dataclass; we need frozen=True, order=True to guarantee that casting it to a list gives the list in the right order

We will also shortly remove these callback collections completely

DominicOram

Great, thank you! Sorry for such a long back and forth

d-perl added 6 commits November 3, 2023 14:48

# 950 remove params from callback init

ab13176

and replace plan names with constants

complete instantiation of callbacks on start doc

a27e9a0

fix tests

939e1b9

fix more tests

c631ae6

fix linting

23b586b

delete broken tests

c38cdf1

d-perl changed the title ~~950 Initialize callbacks using params emitted in BlueSky documents~~ 950 Initialize callbacks using params emitted in Bluesky documents Nov 7, 2023

DominicOram suggested changes Nov 9, 2023

View reviewed changes

zocalo callbacks get parameters themselves

42e7d88

d-perl added 4 commits November 10, 2023 10:01

add IspybIds class

86149a7

Merge branch 'main' into 950_init_callbacks_w_document

a2f5a7b

#372 fix dataclass and tests

7a594dd

#950 tidy up ispyb callback variables

5f8b6c6

d-perl requested a review from DominicOram November 10, 2023 11:49

DominicOram suggested changes Nov 13, 2023

View reviewed changes

d-perl and others added 4 commits November 14, 2023 09:36

#949 fix more typing in StoreInIspyb

4b16ad5

#950 merge in changes from #949

7e537e7

Merge branch 'main' into 950_init_callbacks_w_document

b153514

Merge branch 'main' into 950_init_callbacks_w_document

e84f88c

DominicOram approved these changes Nov 20, 2023

View reviewed changes

DominicOram changed the title ~~950 Initialize callbacks using params emitted in Bluesky documents~~ Initialize callbacks using params emitted in Bluesky documents Nov 20, 2023

d-perl added 4 commits November 21, 2023 09:44

Merge branch 'main' into 950_init_callbacks_w_document

2e8bf7e

#950 fix some errors from merging

0635428

#950 finish cleaning up merge artefacts

1ff94c6

#950 fix linting

2e50c4f

d-perl merged commit c856911 into main Nov 21, 2023

d-perl deleted the 950_init_callbacks_w_document branch November 21, 2023 10:46

d-perl mentioned this pull request Nov 21, 2023

fix tests for mjpg file handling change in dodal #990

Closed

Uh oh!

Conversation

d-perl commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test:

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-perl commented Nov 14, 2023

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

d-perl commented Nov 7, 2023 •

edited

Loading

codecov Bot commented Nov 9, 2023 •

edited

Loading