368 generalise ispyb handling to rotation scans by d-perl · Pull Request #711 · DiamondLightSource/hyperion

d-perl · 2023-06-07T10:16:07Z

Fixes #368

To test:

Run the new system test in test_rotation_scan_plan.py
verify that results appear in https://ispyb-test.diamond.ac.uk/dc/visit/cm31105-4

and instead pass references to active connection

codecov · 2023-06-07T10:17:44Z

Codecov Report

Merging #711 (2f3a95f) into main (3ea6e6f) will decrease coverage by 0.07%.
The diff coverage is 96.06%.

@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
- Coverage   94.45%   94.38%   -0.07%     
==========================================
  Files          37       40       +3     
  Lines        1875     2013     +138     
==========================================
+ Hits         1771     1900     +129     
- Misses        104      113       +9

Files Changed	Coverage Δ
...l_interaction/callbacks/rotation/ispyb_callback.py	`66.66% <66.66%> (ø)`
...ernal_interaction/callbacks/ispyb_callback_base.py	`95.74% <95.74%> (ø)`
...temis/external_interaction/ispyb/store_in_ispyb.py	`97.24% <97.32%> (+0.33%)`	⬆️
...emis/device_setup_plans/read_hardware_for_setup.py	`100.00% <100.00%> (ø)`
...rc/artemis/experiment_plans/fast_grid_scan_plan.py	`93.10% <100.00%> (-0.41%)`	⬇️
src/artemis/experiment_plans/rotation_scan_plan.py	`100.00% <100.00%> (ø)`
...ternal_interaction/callbacks/fgs/ispyb_callback.py	`95.45% <100.00%> (-2.59%)`	⬇️
...ernal_interaction/callbacks/fgs/zocalo_callback.py	`98.46% <100.00%> (ø)`
...callbacks/rotation/rotation_callback_collection.py	`100.00% <100.00%> (ø)`
...emis/external_interaction/ispyb/ispyb_dataclass.py	`100.00% <100.00%> (+1.81%)`	⬆️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DominicOram

A few initial comments, can pick the review up again later this week. Also, would be good to have some unit test coverage around the new stuff, even if it's just asserting happy path stuff

DominicOram · 2023-06-12T17:10:23Z

+
+@pytest.mark.s03()
+@patch("bluesky.plan_stubs.wait")
+def test_ispyb_deposition_in_plan(


Great test, thank you!
Should: Can we set the dev config here like we do in the test_ispyb_dev_connection so that it runs with the environment variable being set. We should move the path to the dev config into the constants area at the same time.

DominicOram · 2023-06-12T18:09:22Z

+        with ispyb.open(self.ISPYB_CONFIG_PATH) as conn:
+            return self._store_scan_data(conn)
+
+    def end_deposition(self, success: str, reason: str):


Should: Is the only difference between this and the grid one how many dcids we have? Feels like we can move some more logic into the parent here

DominicOram · 2023-06-12T18:17:35Z

+        data_collection_group_id = self._store_data_collection_group_table(conn)
+        self.data_collection_group_id = data_collection_group_id
+        data_collection_id = self._store_data_collection_table(
+            conn, data_collection_group_id
+        )
+        self.data_collection_id = data_collection_id
+        self._store_position_table(conn, data_collection_id)


Should: The logic of "store group, store dc, store position" is the same for all these. Can that go in the parent class?

I guess, but I think it would be more complicated given subtle differences - 3d grid scan does dc and position twice, and they have different return types. I personally think it's not worth changing three sets of three simple statements for one set of logic based on how many dcids we have.

DominicOram · 2023-06-12T18:20:50Z

-        self.datacollection_ids = None
-        self.datacollection_group_id = None
-        self.grid_ids = None
+    def __init__(self, ispyb_config, parameters=None) -> None:


Nit: The parent class doesn't need parameters to be passed in

Should: Every subclass needs an experiment_type, let's pass this in here

DominicOram

Mostly good, thanks. Some suggestions for improvements in the code. Also, when running the new system test I get src/artemis/experiment_plans/tests/test_rotation_scan_plan.py::test_ispyb_deposition_in_plan - AttributeError: armed

DominicOram · 2023-07-24T10:25:37Z

-        "zebra": zebra,
-        "detector_motion": detector_motion,
-        "backlight": backlight,
+        "eiger": i03.eiger(wait_for_connection=False),


Could: I should have advertised this more widely but I think this is no longer necessary. Feel free to modify here or we can wait for #825

DominicOram · 2023-07-24T10:27:45Z

+    zebra.pc.arm_demand.set = mock_arm_disarm
+
+    devices = {
+        "eiger": i03.eiger(wait_for_connection=False, fake_with_ophyd_sim=True),


Could: As above with waiting for connection

yes, this one wasn't necessary anyway, it's an ophyd sim device... haha...

DominicOram · 2023-07-24T10:29:14Z

            self.ispyb.append_to_comment(crystal_summary)

            raw_centre = np.array([*(raw_results[0]["centre_of_mass"])])
+            adjusted_centre = raw_centre - np.array([0.5, 0.5, 0.5])


DominicOram · 2023-07-24T10:29:57Z

+    E.g.:
+        nexus_file_handler_callback = NexusFileHandlerCallback(parameters)
+        RE.subscribe(nexus_file_handler_callback)
+    Or decorate a plan using bluesky.preprocessors.subs_decorator.
+
+    See: https://blueskyproject.io/bluesky/callbacks.html#ways-to-invoke-callbacks
+
+    Usually used as part of an FGSCallbackCollection.


Should: This has incorrect references to Nexus/FGS

DominicOram · 2023-07-24T10:32:31Z

+from artemis.parameters.plan_specific.fgs_internal_params import FGSInternalParameters
+
+
+class RotationISPyBHandlerCallback(CallbackBase):


Should: 90% of the code in here is the same as the FGS one, I feel like we could have a common inheritence for them?

DominicOram · 2023-07-24T11:02:04Z


 @patch("ispyb.open", new_callable=mock_open)
-def test_store_grid_scan(ispyb_conn, dummy_ispyb, dummy_params):
+def test_store_rotation_scan(


Should: I think we're mocking out a bit too much here, which is why we're getting such poor coverage on StoreRotationInIspyb. Can we either in this test or in a new test mock just the upserts?

I don't really understand. As fast as I can tell we are only mocking the connection (which is necessary at least for tests which run in CI) and nothing else? low coverage seems more down to not testing _mutate_data_collection_params_for_experiment

I mean that later in this test we have lots of when(x).y().thenReturn(z), which means we're never really testing any of the functionality of x.y(). I think it's fine to leave it for now though,given we have the additional tests on the mutate

DominicOram · 2023-07-24T11:02:57Z

+            LOGGER.warning("No xtal snapshot paths sent to ISPyB!")
+        super().__init__(ispyb_config, "SAD")
+
+    def _mutate_data_collection_params_for_experiment(


Should: I think a few additional simple tests just covering this function would be good

DominicOram · 2023-07-24T11:04:05Z

+    ispyb_params: GridscanIspybParams | None = None
+    data_collection_ids: tuple[int, ...] | None = None
+    upper_left: list[int] | None = None
+    y_steps: int | None = None
+    y_step_size: int | None = None
+    grid_ids: tuple[int, ...] | None = None


Should: As above, avoid class variables

DominicOram · 2023-07-24T11:08:57Z

+        # not an annotation but specified as field encoder in DetectorParams:
+        detector_field_keys.append("detector")
+        ispyb_field_keys = list(IspybParams.__annotations__.keys()) + list(
+            GridscanIspybParams.__annotations__.keys()


Should: This should be the RotationIspybParams right?

Can you write a test that would have caught this too

DominicOram · 2023-07-24T11:09:44Z

+        super().__init__(**args)
+
+    @staticmethod
+    def _artemis_param_key_definitions() -> tuple[list[str], list[str], list[str]]:


Should: I feel like we've moved too much of this function to be in each different class, when only a small part of it is different for each. Could we only move the different part please?

DominicOram

Minor issue in logging but once I fixed that this worked. Thank you very much, sorry for the long delay on it!

d-perl added 20 commits June 6, 2023 08:13

make gridscan artemisparam and ispybparam classes

73b0137

make calling of subclasses consistent

565dbc3

move ispybparams contents to base class

1697957

remove test file

1edec58

fix ispyb handler and tests

159333e

tidy up a bit

2950597

add correct typing to everything

19790af

move som emore functions to base class

d1d6c01

make most of store datacollection table generic

f00eeb6

add _store_scan_data for rotation

8b2f090

add begin and end depostion for rotation

b613bd4

create rotation ispyb callback

05a4a04

add ispyb reading stub to rotation scan plan

a509718

tidy rotation plan and add new devices to test

bd12a83

add system test for ispyb deposition in plan

2f04929

Merge branch 'main' into 368_generalise_ispyb_handling

2287d68

remove self.conn/acq/core

6a0fc58

and instead pass references to active connection

add test for making ispyb deposition in plan

20960df

test other deposition attributes

997d302

fix ispyb unit tests

8421bf3

add unit test

fc58c6d

DominicOram suggested changes Jun 12, 2023

View reviewed changes

d-perl added 7 commits June 15, 2023 18:55

make ispybconfig constant

cff2c16

Merge branch 'main' into 368_generalise_ispyb_handling

ed79fb4

fix stuff from merge

7450232

fix tests

7fce67e

pass experiment type to superclass

0402d03

move end deposition logic to parent class

e5cceb4

set dev config in test and fix typo

0fe777e

Merge branch 'main' into 368_generalise_ispyb_handling

cfaf9e3

d-perl requested a review from DominicOram June 22, 2023 15:30

d-perl added 6 commits July 17, 2023 15:52

Merge branch 'main' into 368_generalise_ispyb_handling

e725ac2

tidy

a723870

fix more merge issues

6c10d67

mock ispyb callback in nexus writing tests

5a7111d

Merge branch 'main' into 368_generalise_ispyb_handling

99e06a9

remove debugging line

5ea651c

DominicOram suggested changes Jul 24, 2023

View reviewed changes

d-perl added 12 commits July 25, 2023 13:56

tidy bits and comments

46704bf

make ispyb callback base class

31bc2e7

get rid of class variables in storeinispyb

2e4ca3c

more artemis param key definitions to superclass

2fa6ebc

Merge branch 'main' into 368_generalise_ispyb_handling

6415a2c

fixes and test fixes

e266a37

mock ispyb connection in zoc tests

7eb6a6b

fix mock

822ebd3

add some unhappy path tests

f6003d3

add couple more tests

530acdc

fix lint

8cb1c17

add test for matching params

da82a0c

d-perl requested a review from DominicOram July 26, 2023 13:21

(#368) Fix logging in BaseISPyBHandlerCallback

2f3a95f

DominicOram approved these changes Jul 27, 2023

View reviewed changes

d-perl merged commit 997ea2e into main Jul 27, 2023

d-perl deleted the 368_generalise_ispyb_handling branch July 27, 2023 12:11

		from artemis.parameters.plan_specific.fgs_internal_params import FGSInternalParameters


		class RotationISPyBHandlerCallback(CallbackBase):

Uh oh!

Conversation

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

Uh oh!

To test:

Uh oh!

codecov Bot commented Jun 7, 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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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 Jun 7, 2023 •

edited

Loading

codecov Bot commented Jun 7, 2023 •

edited

Loading

DominicOram left a comment •

edited

Loading

d-perl Jun 15, 2023 •

edited

Loading