Final tidy of the great ispyb refactor #1268

rtuck99 · 2024-03-18T10:31:57Z

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

To test:

do the tests pass

rtuck99 · 2024-03-19T11:39:08Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

@@ -56,6 +68,8 @@ def __init__(
        self.log = ISPYB_LOGGER

    def activity_gated_start(self, doc: RunStart):
+        self._event_driven_data_collection_info = DataCollectionInfo()
+        self._sample_barcode = None


This may be a bug because this will get called on every activity start while we are active not just the one where we are activated, thus we may end up erasing our data before we write it

DominicOram

Half a review, can further discuss in person

DominicOram · 2024-04-05T12:57:42Z

src/hyperion/experiment_plans/robot_load_then_centre_plan.py

@@ -178,6 +178,7 @@ def robot_load_then_centre(
        yield from read_energy(cast(SetEnergyComposite, composite))
    )

+    # XXX TODO 1173 remove this - left in to avoid breaking nexus files?


Created #1292 to remove this in a new PR

DominicOram · 2024-04-03T13:11:18Z

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

-        dcg_info = populate_data_collection_group(
-            self.ispyb.experiment_type,
-            params.hyperion_params.detector_params,
+    def populate_info_for_update(


Should: Why are we putting the info from the start doc back into this object? Happy to discuss in person

Discussed and fixed in #1279

DominicOram · 2024-04-05T12:57:42Z

src/hyperion/experiment_plans/robot_load_then_centre_plan.py

@@ -178,6 +178,7 @@ def robot_load_then_centre(
        yield from read_energy(cast(SetEnergyComposite, composite))
    )

+    # XXX TODO 1173 remove this - left in to avoid breaking nexus files?


Created #1292 to remove this in a new PR

DominicOram

Great, thank you! Made some comments but mostly quite minor in the end as I've pulled the bigger stuff into new issues

DominicOram · 2024-04-05T13:05:31Z

src/hyperion/experiment_plans/flyscan_xray_centre_plan.py

@@ -356,6 +356,7 @@ def flyscan_xray_centre(
    @transmission_and_xbpm_feedback_for_collection_decorator(
        composite.xbpm_feedback,
        composite.attenuator,
+        # TODO 1033 don't use ispyb_params for this value, move it to experiment_params


Should: 1033 is now merged, I think this will be removed when you merge main into the PR?

DominicOram · 2024-04-05T13:06:35Z

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

-        dcg_info = populate_data_collection_group(
-            self.ispyb.experiment_type,
-            params.hyperion_params.detector_params,
+    def populate_info_for_update(


Discussed and fixed in #1279

DominicOram · 2024-04-05T13:59:18Z

tests/system_tests/external_interaction/callbacks/test_external_callbacks.py

@@ -220,6 +220,7 @@ def test_remote_callbacks_write_to_dev_ispyb_for_rotation(
    test_rotation_params.hyperion_params.ispyb_params.beam_size_x = test_bs_x
    test_rotation_params.hyperion_params.ispyb_params.beam_size_y = test_bs_y
    test_rotation_params.hyperion_params.detector_params.exposure_time = test_exp_time
+    # TODO this should not be set here


Should: What does this comment mean? What's the path to fixing it?

I think this is part of the same set of changes as the other things which I have labelled #1173 but I think you've now labelled #1292 - basically the current_energy_ev shouldn't be in the ispyb_params but read out from events.

DominicOram · 2024-04-05T14:38:11Z

tests/system_tests/external_interaction/test_ispyb_dev_connection.py

+    # TODO Can't test barcode as need BLSample which needs Dewar, Shipping, Container entries for the
+    # upsert stored proc to use it.


Should: The barcode is on the data collection group, I think we can test that. But also that will probably be removed soon anyway with #1293

When you do an upsert_dc_group the sproc executes the following (or something similar):

IF p_sampleId IS NULL AND p_sampleBarcode IS NOT NULL THEN SELECT max(bls.blSampleId) INTO p_sampleId FROM BLSample bls INNER JOIN Container c on c.containerId = bls.containerId INNER JOIN Dewar d on d.dewarId = c.dewarId INNER JOIN Shipping s on s.shippingId = d.shippingId WHERE bls.code = p_sampleBarcode AND s.proposalId = row_proposal_id;

Therefore the insert will not do anything unless you have all those things
The DCG upsert puts a barcode into a column called actualSampleBarcode, there isn't a column which is just called sampleBarcode, sampleBarcode is just used to lookup the sampleId

This is good to know. So basically setting sampleBarcode is just another way of setting the sampleId, which we are doing anyway.

DominicOram · 2024-04-05T15:14:19Z

tests/unit_tests/experiment_plans/test_panda_flyscan_xray_centre_plan.py

@@ -805,3 +816,11 @@ def test_tidy_up_plans_disable_panda_and_zebra(
    RE(tidy_up_plans(MagicMock()))
    mock_panda_tidy.assert_called_once()
    mock_zebra_tidy.assert_called_once()
+
+
+def assert_event(mock_call, expected):


Should: Can we just use the one in the conftest?

Yes, it seems I created both of them at pretty much the same time, so suspect I got distracted whilst moving it and just forgot

DominicOram · 2024-04-05T15:20:54Z

tests/unit_tests/external_interaction/ispyb/test_gridscan_ispyb_store_2d.py

-            ),
-            dummy_params.hyperion_params.detector_params,
-            dummy_params.hyperion_params.ispyb_params,
+def scan_data_info_for_begin():


Could: I think it would be nicer if we could combine some of the common bits of these test helper functions that make ScanDataInfo and DataCollectionInfo as they're very verbose but I appreciate this makes it more explicit so up to you.

Thinking maybe I should remove this file if we don't support 2D gridscans!

DominicOram · 2024-04-05T15:25:14Z

src/hyperion/experiment_plans/panda_flyscan_xray_centre_plan.py

@@ -289,6 +289,7 @@ def panda_flyscan_xray_centre(
    @transmission_and_xbpm_feedback_for_collection_decorator(
        composite.xbpm_feedback,
        composite.attenuator,
+        # TODO 1033 don't use ispyb_params for this value, move it to experiment_params
        parameters.hyperion_params.ispyb_params.transmission_fraction,


DominicOram · 2024-04-05T15:28:08Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+                # TODO 1173 Remove this once nexus_utils no longer needs it
+                self.params.hyperion_params.ispyb_params.transmission_fraction = doc[
+                    "data"
+                ]["attenuator_actual_transmission"]


Should: I think this disappears on merge?

No it doesn't but I will remove them.
E: on reflection I will do that in a separate PR re new ticket #1295

DominicOram · 2024-04-05T15:31:08Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+                    convert_eV_to_angstrom(energy_ev)
+                )
+                # TODO 1173 Remove this once nexus_utils no longer needs wavelength_angstroms
+                self.params.hyperion_params.ispyb_params.current_energy_ev = energy_ev


Created #1292 to remove this in a new PR

DominicOram · 2024-04-05T15:35:52Z

src/hyperion/external_interaction/ispyb/ispyb_store.py

-                data_collection_group_id=data_collection_group_id,
-                data_collection_ids=(data_collection_id,),
+        ispyb_ids = IspybIds()
+        if scan_data_info.data_collection_info:


The creating/updating of groups is very complicated, I've made #1293 to simplify

* Remove dependencies on populate_ methods * Move some tests down to the callback mapping layer * Populate test fixtures by direct parameter construction instead of duplicating caller logic * Remove dependencies on dummy_params from unit tests

…emaining_data_collection_info() Additional unit tests for callbacks

…due to nexus callback * Correct references to current_energy_ev in various test.json files as it is now called expected_energy_ev

…rom read data

…ODOs as per PR feedback

DominicOram

Great, thank you!

DominicOram · 2024-04-08T17:58:47Z

Failing test is also failing on main so will merge this anyway and fix in a new PR

…Source/1218_final_tidy_of_the_great_ispyb_refactor Final tidy of the great ispyb refactor

rtuck99 commented Mar 19, 2024

View reviewed changes

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 1218_final_tidy_of_the_great_ispyb_refactor branch 2 times, most recently from 37e8849 to 266070d Compare April 2, 2024 13:48

rtuck99 changed the base branch from main to 1252_fix_logging_unit_test_failures April 2, 2024 13:50

Base automatically changed from 1252_fix_logging_unit_test_failures to main April 3, 2024 11:30

DominicOram reviewed Apr 3, 2024

View reviewed changes

DominicOram reviewed Apr 5, 2024

View reviewed changes

DominicOram mentioned this pull request Apr 5, 2024

Remove setting of parameters in ispyb_params #1292

Closed

DominicOram suggested changes Apr 5, 2024

View reviewed changes

rtuck99 added 8 commits April 8, 2024 11:26

(#1218) Refactor event-driven data collection logic out of populate_r…

5cfe788

…emaining_data_collection_info() Additional unit tests for callbacks

(#1218) ispyb_params fields no longer populated during hardware read.

442f28b

(#1218) partial removal of various ispyb params but have to leave in …

db23007

…due to nexus callback * Correct references to current_energy_ev in various test.json files as it is now called expected_energy_ev

(#1218) Removal of sample_barcode from ispyb_params

853ad7e

(#1218) Update to system tests to cover end-to-end ispyb population f…

0a4eaf5

…rom read data

(#1218) Make begin_deposition call the same code as update_deposition

0f08072

(#1218) Make pyright happy

5b83260

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

(#1218) Remove 2d gridscan test and dedup fixtures, remove obsolete T…

78a664d

…ODOs as per PR feedback

rtuck99 requested a review from DominicOram April 8, 2024 11:59

DominicOram mentioned this pull request Apr 8, 2024

Create data collection group once on start document #1293

Closed

DominicOram approved these changes Apr 8, 2024

View reviewed changes

Merge branch 'main' into 1218_final_tidy_of_the_great_ispyb_refactor

c7e35ef

DominicOram merged commit 7075487 into main Apr 8, 2024
3 of 4 checks passed

DominicOram deleted the 1218_final_tidy_of_the_great_ispyb_refactor branch April 8, 2024 17:58

olliesilvester pushed a commit to olliesilvester/mx-bluesky that referenced this pull request Aug 23, 2024

Merge pull request DiamondLightSource/hyperion#1268 from DiamondLight…

73a9658

…Source/1218_final_tidy_of_the_great_ispyb_refactor Final tidy of the great ispyb refactor

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final tidy of the great ispyb refactor #1268

Final tidy of the great ispyb refactor #1268

rtuck99 commented Mar 18, 2024

rtuck99 Mar 19, 2024

DominicOram left a comment

DominicOram Apr 5, 2024 •

edited

Loading

DominicOram Apr 3, 2024

DominicOram Apr 5, 2024

DominicOram Apr 5, 2024 •

edited

Loading

DominicOram left a comment

DominicOram Apr 5, 2024

DominicOram Apr 5, 2024

DominicOram Apr 5, 2024

rtuck99 Apr 8, 2024

DominicOram Apr 5, 2024

rtuck99 Apr 8, 2024

DominicOram Apr 8, 2024

DominicOram Apr 5, 2024

rtuck99 Apr 8, 2024

DominicOram Apr 5, 2024

rtuck99 Apr 8, 2024

DominicOram Apr 5, 2024

DominicOram Apr 5, 2024

rtuck99 Apr 8, 2024 •

edited

Loading

DominicOram Apr 5, 2024

DominicOram Apr 5, 2024

DominicOram left a comment

DominicOram commented Apr 8, 2024

		# TODO Can't test barcode as need BLSample which needs Dewar, Shipping, Container entries for the
		# upsert stored proc to use it.

Final tidy of the great ispyb refactor #1268

Final tidy of the great ispyb refactor #1268

Conversation

rtuck99 commented Mar 18, 2024

To test:

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

DominicOram Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram Apr 5, 2024 • edited Loading

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

rtuck99 Apr 8, 2024 • edited Loading

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

DominicOram commented Apr 8, 2024

DominicOram Apr 5, 2024 •

edited

Loading

DominicOram Apr 5, 2024 •

edited

Loading

rtuck99 Apr 8, 2024 •

edited

Loading