1217 Use event from grid detect to do ispyb deposition #1279

rtuck99 · 2024-03-22T15:57:24Z

Link to dodal PR (if required):N/A
(remember to update setup.cfg with the dodal commit tag if you need it for tests to pass!)

To test:

Unit + System tests pass
Grid scan details in ispyb and snapshots in syncweb should continue to populate

DominicOram

Thanks! Partly reviewed but ran out of time to go as deep as I wanted, sorry.
Nonetheless, some comments in code. Also, I think with the merge of #1297 some of the code reduces down.

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

src/hyperion/external_interaction/callbacks/xray_centre/ispyb_callback.py

+def ispyb_activation_wrapper(parameters, plan_generator):
+    @bpp.run_decorator(
+        md={
+            "activate_callbacks": ["GridscanISPyBCallback"],
+            "subplan_name": CONST.PLAN.GRID_DETECT_AND_DO_GRIDSCAN,
+            "hyperion_internal_parameters": parameters.json(),
+        }
+    )
+    @wraps(plan_generator)
+    def wrapped():
+        return plan_generator
+
+    yield from wrapped()


Should:

Suggested change

def ispyb_activation_wrapper(parameters, plan_generator):

@bpp.run_decorator(

md={

"activate_callbacks": ["GridscanISPyBCallback"],

"subplan_name": CONST.PLAN.GRID_DETECT_AND_DO_GRIDSCAN,

"hyperion_internal_parameters": parameters.json(),

}

)

@wraps(plan_generator)

def wrapped():

return plan_generator

yield from wrapped()

def ispyb_activation_wrapper(parameters, plan_generator):

return bpp.run_wrapper(

plan_generator,

md={

"activate_callbacks": ["GridscanISPyBCallback"],

"subplan_name": CONST.PLAN.GRID_DETECT_AND_DO_GRIDSCAN,

"hyperion_internal_parameters": parameters.json(),

},

)

Nit: I think you could then switch the parameters and plan_generator to more closely match the run_wrapper syntax

DominicOram · 2024-04-09T16:12:56Z

src/hyperion/external_interaction/callbacks/rotation/ispyb_callback.py

-        )
-        initial_collection_info = replace(
-            initial_collection_info,
+        data_collection_info = replace(


Should: Is this now taking an empty data collection and recreating it from the existing event_sourced_data_collection_info? Why can't we just use the event_sourced_data_collection_info itself?

Similar in the ispyb_callback for xray centring.

yes, I should have spotted this!

DominicOram · 2024-04-09T16:22:42Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+        grid_scan_info = GridScanInfo(
+            upper_left_px=numpy.array(
+                [data["oav_snapshot_top_left_x"], data["oav_snapshot_top_left_y"]]
+            ),
+            x_steps=data["oav_snapshot_num_boxes_x"],
+            y_steps=data["oav_snapshot_num_boxes_y"],
+            # convert pixels back to mm
+            x_step_size_mm=data["oav_snapshot_box_width"]
+            * self.params.hyperion_params.ispyb_params.microns_per_pixel_x
+            / 1000,
+            y_step_size_mm=data["oav_snapshot_box_width"]
+            * self.params.hyperion_params.ispyb_params.microns_per_pixel_y
+            / 1000,
+        )


Should: Is the only reason we need the grid scan info for constructing the comment? It seems messy to make this object here, use it once, then use the same parameters to make a DataCollectionGridInfo straight after. Could we change construct_comment_for_gridscan to take a DataCollectionGridInfo or to just take the parameters it needs then we can get rid of GridScanInfo?

I will get rid of it

DominicOram · 2024-04-09T16:28:15Z

src/hyperion/external_interaction/ispyb/ispyb_store.py

        assert (
            ispyb_ids.data_collection_group_id
        ), "Attempted to store scan data without a collection group"
        assert (
            ispyb_ids.data_collection_ids
        ), "Attempted to store scan data without a collection"
+        print(f"OBJECT PASSED IN IS {ispyb_ids}")


Should: I assume this is for debugging? Can you remove please? Same with other places in this file.

DominicOram · 2024-04-10T12:53:57Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+            self.ispyb_ids, None, [scan_data_info]
+        )
+        self._oav_snapshot_event_idx += 1
+        pass


Should: What is this pass doing?

DominicOram · 2024-04-10T13:11:50Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+            case CONST.DESCRIPTORS.ISPYB_HARDWARE_READ:
+                self._handle_ispyb_hardware_read(doc)
+            case CONST.DESCRIPTORS.OAV_SNAPSHOT_TRIGGERED:
+                self._handle_oav_snapshot_triggered(doc)
+            case CONST.DESCRIPTORS.ISPYB_TRANSMISSION_FLUX_READ:


Should: Why do each of these cases have different behavior w.r.t actually updating ispyb?

CONST.DESCRIPTORS.ISPYB_HARDWARE_READ - doesn't update ispyb at all

CONST.DESCRIPTORS.OAV_SNAPSHOT_TRIGGERED - Updates ispyb inside _handle_oav_snapshot_triggered

CONST.DESCRIPTORS.ISPYB_TRANSMISSION_FLUX_READ - Updates ispyb outside _handle_ispyb_transmission_flux_read

It seems to me that they should all update ispyb and they should do it in a consistent way

historical reasons really, that's the original underlying behaviour that has always been there.

DominicOram

Apologies, some more comments in code.

DominicOram · 2024-04-15T12:51:36Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

-                self._sample_barcode,
-            )
-            ISPYB_LOGGER.info(f"Recieved ISPYB IDs: {self.ispyb_ids}")
+        match event_descriptor.get("name"):


Could: Now that these all call the update_deposition maybe it would be better to have each of these cases return the update and then call the update_deposition here?

DominicOram · 2024-04-15T12:55:29Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+        data_collection_info = DataCollectionInfo()
+        data_collection_info.xtal_snapshot1 = data.get(
+            "oav_snapshot_last_path_full_overlay"
+        )
+        data_collection_info.xtal_snapshot2 = data.get("oav_snapshot_last_path_outer")
+        data_collection_info.xtal_snapshot3 = data.get("oav_snapshot_last_saved_path")
+        data_collection_info.n_images = (
+            data["oav_snapshot_num_boxes_x"] * data["oav_snapshot_num_boxes_y"]
+        )


Nit: We could just do this in the instantiation like _handle_ispyb_hardware_read:

data_collection_info = DataCollectionInfo( xtal_snapshot1 = data.get("oav_snapshot_last_path_full_overlay"), .... )

DominicOram · 2024-04-15T13:08:04Z

src/hyperion/external_interaction/callbacks/rotation/ispyb_mapping.py

-    return "Hyperion rotation scan"
+    return


Should: This function is no longer used, can we remove?

DominicOram · 2024-04-15T16:12:42Z

src/hyperion/external_interaction/callbacks/xray_centre/ispyb_mapping.py

 from hyperion.external_interaction.ispyb.data_model import (
    DataCollectionGridInfo,
    DataCollectionInfo,
 )
-from hyperion.external_interaction.ispyb.ispyb_dataclass import Orientation


 def populate_xz_data_collection_info(


Should: Why does this need full_params?

DominicOram · 2024-04-15T16:13:40Z

src/hyperion/external_interaction/callbacks/xray_centre/ispyb_mapping.py

    detector_params,
 ) -> DataCollectionInfo:
    assert (
        detector_params.omega_start is not None
        and detector_params.run_number is not None
-        and ispyb_params is not None
        and full_params is not None
    ), "StoreGridscanInIspyb failed to get parameters"
    omega_start = detector_params.omega_start + 90


Should: We should be able to read the omega from the smargon when we do the pin tip detection, can we make an issue for that?

Have made #1327 but is pin tip detection the right place or should it be done during read_hardware_for_ispyb_pre_collection?

It can't be done during read_hardware_for_ispyb_pre_collection because we want the omega for each grid. If we read omega at pre_collection we only get the first grid's omega. We would then have to add the 90 deg in the callback again, which I was trying to avoid. Basically I want the logic of what we're doing to be in the plan, the callback should just be blindly funneling it into ispyb

DominicOram · 2024-04-15T16:30:24Z

src/hyperion/external_interaction/callbacks/xray_centre/ispyb_callback.py

+                ScanDataInfo(
+                    data_collection_info=populate_remaining_data_collection_info(
+                        None,
+                        None,


Should: Shouldn't the dcgid here be the group made above?

we haven't made it yet, we've only populated the DataCollectionGroupInfo but it hasn't been posted to the database so there's no id, leaving it as none will cause it to be populate during the call to begin_deposition

Ok, that seems a bit convoluted but I guess we can tidy up in #1293

DominicOram · 2024-04-15T16:32:48Z

src/hyperion/external_interaction/callbacks/xray_centre/ispyb_callback.py

-                        grid_scan_info,
-                        self.params,
-                        self.params.hyperion_params.ispyb_params,
+            scan_data_infos = [


Should: Rather than do anything specific upfront could we get away with doing all the specific stuff in _handle_oav_snapshot_triggered? e.g. this could be something like:

number_of_collections = 2 if self.is_3d_gridscan() else 1 scan_data_infos = [ScanDataInfo(data_collection_info=populate_remaining_data_collection_info( None, None, DataCollectionInfo( axis_range=0, axis_end=omega_start, ), self.params.hyperion_params.detector_params, self.params.hyperion_params.ispyb_params, ) for range in (number_of_collections)]

Then handle run_number and omega_start in _handle_oav_snapshot_triggered?

I guess this is a wider question than for this one particular change but given that we are planning to move to ispyb-eye is it better to defer some of our internal changes until that point? At least until it's clearer how start / event documents might map to ispyb-eye calls? Unless you were proposing this with that already in mind.

Sure, happy to park this :)

* Move ispyb callback activation to before grid detect * Fixup all the broken unit tests * Separate descriptor constants from plan names * Move callback activation wrapper to callback module

* Remove upsert_dc_grid calls from hardware read ispyb updates * Add units to GridScanInfo fields * Move grid comment updates from hardware read to oav snapshot event

* Fix system tests * Change comment to be specified as a value instead of a factory

* Remove ispyb params upper_left

* Remove oav_snapshot_callback.py

…ivity_start() and eager create both xy and xz DataCollection, as per PR feedback

Remove GridScanInfo

…ore uniform in ispyb_callback_base.py as per PR review

* Remove unused methods and method parameters * Extract common call to update_deposition() from ispyb_callback_base handler methods * Tidy up initialisation of DataCollectionInfo

…deposition

DominicOram

Great, thank you! I think this all looks a lot cleaner now

…Source/1217_use_event_from_grid_detect_to_do_ispyb_deposition 1217 Use event from grid detect to do ispyb deposition

rtuck99 requested review from DominicOram and d-perl March 22, 2024 15:57

rtuck99 changed the base branch from main to 1218_final_tidy_of_the_great_ispyb_refactor March 22, 2024 15:58

rtuck99 force-pushed the 1218_final_tidy_of_the_great_ispyb_refactor branch from 20d7c37 to 6a2a0a9 Compare March 22, 2024 16:20

rtuck99 force-pushed the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch from af06d87 to de7235f Compare March 22, 2024 16:59

This was referenced Mar 26, 2024

Use pixels_per_micron from file not from GDA #1283

Closed

Modify the callbacks to use the new external param classes #1276

Closed

rtuck99 force-pushed the 1218_final_tidy_of_the_great_ispyb_refactor branch from 6a2a0a9 to 37e8849 Compare April 2, 2024 08:56

rtuck99 force-pushed the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch from de7235f to 7864007 Compare April 2, 2024 09:03

rtuck99 force-pushed the 1218_final_tidy_of_the_great_ispyb_refactor branch from 37e8849 to 266070d Compare April 2, 2024 13:48

rtuck99 force-pushed the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch from 7864007 to 831d8e8 Compare April 2, 2024 13:49

DominicOram mentioned this pull request Apr 5, 2024

Final tidy of the great ispyb refactor #1268

Merged

rtuck99 force-pushed the 1218_final_tidy_of_the_great_ispyb_refactor branch from 266070d to 5b83260 Compare April 8, 2024 11:16

rtuck99 force-pushed the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch from 928fd92 to 88ad151 Compare April 8, 2024 12:47

Base automatically changed from 1218_final_tidy_of_the_great_ispyb_refactor to main April 8, 2024 17:58

DominicOram suggested changes Apr 9, 2024

View reviewed changes

DominicOram reviewed Apr 10, 2024

View reviewed changes

rtuck99 force-pushed the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch from b5efe47 to 5dd5eb9 Compare April 11, 2024 13:19

rtuck99 requested a review from DominicOram April 12, 2024 09:25

DominicOram suggested changes Apr 15, 2024

View reviewed changes

rtuck99 added 9 commits April 23, 2024 09:15

(#1217) Use event from grid detect to do ispyb deposition

1192243

* Move ispyb callback activation to before grid detect * Fixup all the broken unit tests * Separate descriptor constants from plan names * Move callback activation wrapper to callback module

(#1217)

2e646da

* Remove upsert_dc_grid calls from hardware read ispyb updates * Add units to GridScanInfo fields * Move grid comment updates from hardware read to oav snapshot event

(#1217) Use event from grid detect to do ispyb deposition

396208b

* Fix system tests * Change comment to be specified as a value instead of a factory

(#1217) Use event from grid detect to do ispyb deposition

36e1458

* Remove ispyb params upper_left

(#1217) Use event from grid detect to do ispyb deposition

d236a1d

* Remove oav_snapshot_callback.py

(#1217) Make pyright happy

c6012bd

(#1217) Only populate common DataCollectionInfo properties during act…

010dced

…ivity_start() and eager create both xy and xz DataCollection, as per PR feedback

Fix system test breakage caused by changes to synchrotron mode

09418c4

(#1217) Fix broken ispyb system tests

13c7429

rtuck99 added 4 commits April 23, 2024 09:46

(#1283) Fix oav snapshot numpy type pollution

3c6b275

(#1217) Fix issues from PR review

6edb5a8

Remove GridScanInfo

(#1217) Remove _hwscan_data_collection_info and make event handling m…

7b19b8f

…ore uniform in ispyb_callback_base.py as per PR review

(#1217) make pyright happy

36436f3

rtuck99 force-pushed the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch from 662c1aa to 36436f3 Compare April 23, 2024 09:02

rtuck99 mentioned this pull request Apr 23, 2024

ispyb omega_start value should be read from hardware instead of supplied by params #1327

Closed

(#1217) Response to PR review comments

7779ab7

* Remove unused methods and method parameters * Extract common call to update_deposition() from ispyb_callback_base handler methods * Tidy up initialisation of DataCollectionInfo

rtuck99 requested a review from DominicOram April 23, 2024 11:17

rtuck99 and others added 2 commits April 23, 2024 15:18

(#1217) Make pyright happy

95959bc

Merge branch 'main' into 1217_use_event_from_grid_detect_to_do_ispyb_…

fbae9d8

…deposition

DominicOram approved these changes Apr 23, 2024

View reviewed changes

DominicOram merged commit d2c9c63 into main Apr 23, 2024
6 checks passed

DominicOram deleted the 1217_use_event_from_grid_detect_to_do_ispyb_deposition branch April 23, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1217 Use event from grid detect to do ispyb deposition #1279

1217 Use event from grid detect to do ispyb deposition #1279

rtuck99 commented Mar 22, 2024

DominicOram left a comment

DominicOram Apr 9, 2024

DominicOram Apr 9, 2024

DominicOram Apr 9, 2024

DominicOram Apr 9, 2024

rtuck99 Apr 10, 2024

DominicOram Apr 9, 2024

rtuck99 Apr 11, 2024

DominicOram Apr 9, 2024

rtuck99 Apr 11, 2024

DominicOram Apr 10, 2024

DominicOram Apr 10, 2024

rtuck99 Apr 11, 2024

DominicOram left a comment

DominicOram Apr 15, 2024

DominicOram Apr 15, 2024

DominicOram Apr 15, 2024

DominicOram Apr 15, 2024

DominicOram Apr 15, 2024

rtuck99 Apr 23, 2024

DominicOram Apr 23, 2024

DominicOram Apr 15, 2024

rtuck99 Apr 23, 2024

DominicOram Apr 23, 2024

DominicOram Apr 15, 2024

rtuck99 Apr 23, 2024

DominicOram Apr 23, 2024

DominicOram left a comment

1217 Use event from grid detect to do ispyb deposition #1279

1217 Use event from grid detect to do ispyb deposition #1279

Conversation

rtuck99 commented Mar 22, 2024

To test:

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

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

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