315 split fgs communicator #357

d-perl · 2022-11-16T14:53:11Z

Fixes #315

Separates the FGSCommunicator into three callback classes, adds a collection for easy subscription and handling the connection between the ispyb and zocalo components

To test:

Check the changes make sense
Check the new test makes sense
Confirm tests pass

and add changes from test_fgs_communicator to appropriate individual callback test files Squashed commit of the following: commit 59739c2 Merge: 0a0c534 67c49cc Author: David Perl <[email protected]> Date: Fri Nov 18 11:31:31 2022 +0000 Merge pull request #371 from DiamondLightSource/353_merge_smargons commit 67c49cc Author: Dominic Oram <[email protected]> Date: Fri Nov 18 11:23:07 2022 +0000 (#353) Merged smargons into one commit 0a0c534 Merge: 8b58b3d 0172183 Author: Dominic Oram <[email protected]> Date: Thu Nov 17 15:40:52 2022 +0000 Merge pull request #362 from DiamondLightSource/361_default_interpreter (#361) updating default python interpreter commit 8b58b3d Merge: b65f8f0 4f2de21 Author: Dominic Oram <[email protected]> Date: Thu Nov 17 15:17:35 2022 +0000 Merge pull request #347 from DiamondLightSource/231_adjust_zocalo_coords commit 0172183 Author: Neil.Smith <[email protected]> Date: Thu Nov 17 14:56:51 2022 +0000 (#361) updating default python interpreter commit 4f2de21 Author: David Perl <[email protected]> Date: Thu Nov 17 14:44:58 2022 +0000 commit b65f8f0 Merge: 2bb56e8 9c2d58d Author: Dominic Oram <[email protected]> Date: Thu Nov 17 13:54:07 2022 +0000 Merge pull request #360 from DiamondLightSource/344_update_nexgen 344 update nexgen commit 9c2d58d Author: David Perl <[email protected]> Date: Thu Nov 17 11:58:02 2022 +0000 commit 0baf4f4 Author: David Perl <[email protected]> Date: Thu Nov 17 11:53:33 2022 +0000 commit 0e27def Author: David Perl <[email protected]> Date: Thu Nov 17 11:51:59 2022 +0000 commit 2c8d12e Author: David Perl <[email protected]> Date: Thu Nov 17 09:59:01 2022 +0000 commit a7f94b7 Author: David Perl <[email protected]> Date: Fri Nov 11 14:44:37 2022 +0000 commit a5896ab Author: David Perl <[email protected]> Date: Fri Nov 11 14:40:37 2022 +0000 and update tests to match

DominicOram

This is a lot cleaner, thank you. Mostly minor comments:

Can you fix the merge conflicts?
I think it makes sense for the callbacks to be next to the thing they are handling e.g. the zocalo callback in zocalo_interaction
I really like the external_interaction folder, I think zocalo_interaction should live in it and it would be nice to have a description in the __init__ of what it contains
testdata.py looks like an empty file, is this required?

src/artemis/external_interaction/communicator_callbacks.py

DominicOram · 2022-11-18T13:06:43Z

src/artemis/external_interaction/communicator_callbacks.py

+        datacollection_ids = self.ispyb.ispyb_ids[0]
+        for id in datacollection_ids:
+            run_end(id)
+        self.processing_start_time


Should: remove line doing nothing

oops, was meant to be =time.time()

src/artemis/external_interaction/communicator_callbacks.py

DominicOram · 2022-11-18T13:14:49Z

src/artemis/external_interaction/communicator_callbacks.py

+    def __init__(self, parameters: FullParameters, ispyb_handler: ISPyBHandlerCallback):
+        self.params = parameters
+        self.processing_start_time = 0.0
+        self.processing_time = 0.0


Nit: This doesn't have to be an instance variable, we only use it in one method

src/artemis/external_interaction/communicator_callbacks.py

src/artemis/external_interaction/tests/conftest.py

DominicOram · 2022-11-18T14:20:38Z

src/artemis/external_interaction/tests/test_ispyb_handler.py

+    dc_ids = [1, 2]
+    dcg_id = 4
+
+    mock_ispyb_store_grid_scan.return_value = [dc_ids, None, dcg_id]
+    mock_ispyb_get_time.return_value = td.DUMMY_TIME_STRING
+    mock_ispyb_update_time_and_status.return_value = None
+
+    params = FullParameters()
+    ispyb_handler = ISPyBHandlerCallback(params)
+    ispyb_handler.start(td.test_start_document)
+    ispyb_handler.descriptor(td.test_descriptor_document)
+    ispyb_handler.event(td.test_event_document)


Nit: I would pull the common bit of these two tests out into a function/fixture

DominicOram · 2022-11-18T14:31:02Z

src/artemis/external_interaction/tests/test_ispyb_handler.py

+
+from artemis.external_interaction.communicator_callbacks import ISPyBHandlerCallback
+from artemis.parameters import FullParameters
+


Nit: I think we could test:

The exception on a stop w/o ids

That ispyb only gets triggered on the correct plan

DominicOram · 2022-11-18T14:39:08Z

src/artemis/external_interaction/tests/test_zocalo_handler.py

+from artemis.utils import Point3D
+
+
+def test_run_gridscan_zocalo_calls(


Nit: Can we change the name of this test to be a bit more descriptive?

sure, I didn't name it :p

DominicOram · 2022-11-18T14:41:14Z

src/artemis/external_interaction/tests/test_zocalo_handler.py

+from unittest.mock import MagicMock, call
+
+from artemis.external_interaction.communicator_callbacks import FGSCallbackCollection
+from artemis.parameters import FullParameters


Nit: Can we also test that the exceptions are thrown by the zocalo handler if ispyb has not been called correctly yet?

codecov · 2022-11-18T14:54:22Z

Codecov Report

Merging #357 (f3c1fdf) into main (59739c2) will increase coverage by 0.66%.
The diff coverage is 98.13%.

@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   85.94%   86.60%   +0.66%     
==========================================
  Files          43       44       +1     
  Lines        1601     1680      +79     
==========================================
+ Hits         1376     1455      +79     
  Misses        225      225

Impacted Files	Coverage Δ
...emis/external_interaction/ispyb/ispyb_dataclass.py	`100.00% <ø> (ø)`
src/artemis/fast_grid_scan_plan.py	`65.15% <77.77%> (-0.53%)`	⬇️
...mis/external_interaction/communicator_callbacks.py	`98.95% <98.95%> (ø)`
src/artemis/__main__.py	`92.12% <100.00%> (ø)`
...temis/external_interaction/ispyb/store_in_ispyb.py	`96.23% <100.00%> (ø)`
.../external_interaction/nexus_writing/write_nexus.py	`100.00% <100.00%> (ø)`
src/artemis/external_interaction/tests/conftest.py	`100.00% <100.00%> (ø)`
...artemis/external_interaction/zocalo_interaction.py	`97.72% <100.00%> (ø)`
src/artemis/parameters.py	`100.00% <100.00%> (ø)`

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

d-perl · 2022-11-21T08:45:36Z

I don't know why it's still saying there are conflicts - I merged main in dcb4316. I figure it's because I squashed 13 commits into one for that - but now when I try to merge again I can't commit it because there are no changes, and the diff shows me a bunch of things which are actually identical. All the conflicts can be safely accepted from this branch into main, all the changes are on this branch already.

DominicOram · 2022-11-21T12:32:56Z

I don't know why it's still saying there are conflicts - I merged main in dcb4316. I figure it's because I squashed 13 commits into one for that - but now when I try to merge again I can't commit it because there are no changes, and the diff shows me a bunch of things which are actually identical. All the conflicts can be safely accepted from this branch into main, all the changes are on this branch already.

Not sure what was going on here. I managed to merge fine doing:

git checkout main && git pull
git checkout 315_split_fgs_communicator && git pull
git merge main
... fixed a couple of merge conflicts
git commit && git push

DominicOram

Thanks, code looks good. I think we discussed refactoring to have the dc_id in the plan metadata or in the zocalo constructor. Is there an issue for this?

src/artemis/external_interaction/communicator_callbacks.py

src/artemis/external_interaction/tests/conftest.py

d-perl · 2022-11-21T13:34:42Z

Thanks, code looks good. I think we discussed refactoring to have the dc_id in the plan metadata or in the zocalo constructor. Is there an issue for this?

yes, #372

DominicOram · 2022-11-21T15:35:28Z

Thanks, code looks good. I think we discussed refactoring to have the dc_id in the plan metadata or in the zocalo constructor. Is there an issue for this?

yes, #372

Actually #375

…ource/315_split_fgs_communicator 315 split fgs communicator NOTE: Commit originally came from https://github.com/DiamondLightSource/python-artemis

d-perl added 23 commits November 15, 2022 14:49

#315 move external interactions to submodule

ee25e37

#315 rename fgs_communicator

a9273ea

#315 separate out nexus writing

545378d

#315 add collection for communication classes

ef874a0

#315 tidy and adjust tests

9254327

fix mypy error

3d06671

#315 split up ispyb and zocalo

3ad1289

#315 split up ispyb and zocalo

2a2d432

#315 connect ispyb and zocalo handlers

84ec38e

#315 rearrange tests and update names

69c90bd

#315 fix imports and tests

47d9a8b

fix test

3c94e00

#315 fix test data handling

88594ec

#fix test data handling

9d8c745

don't use dataclass

0a9d571

set zocalo env to dev

5eee253

tidy plan

a10f4dd

tidy tests

b579971

add ispyb exception

c842fe2

#315 add test for callback collection order

4bdd4cd

#315 add docstring and comment

ffafa8a

fix zocalo interaction test

13777ee

DominicOram suggested changes Nov 18, 2022

View reviewed changes

d-perl added 2 commits November 18, 2022 14:50

#315 fix test

c1bb0a9

#315 delete empy testdata

45e7fed

d-perl added 3 commits November 18, 2022 15:00

#315 move zocalo to external interaction folder

8fc2f8a

#315 improve docstrings

28cf1d0

dont store whole params in zocalo handler

eab9f2b

d-perl added 9 commits November 18, 2022 15:17

#315 check event name before triggering zocalo

308165c

#315 fix checking event name

318c96d

#315 add init docstring

0a350a3

#315 update comments

39da3a4

#315 fix comments

c50da32

small fixes

650e42d

fix tests

4c0f26b

#315 improve tests

868bb77

#315 improve tests

2b82e83

d-perl requested a review from DominicOram November 18, 2022 17:25

Merge branch 'main' into 315_split_fgs_communicator

f3c1fdf

DominicOram approved these changes Nov 21, 2022

View reviewed changes

src/artemis/external_interaction/communicator_callbacks.py Show resolved Hide resolved

src/artemis/external_interaction/tests/conftest.py Show resolved Hide resolved

d-perl merged commit d8819e7 into main Nov 21, 2022

d-perl deleted the 315_split_fgs_communicator branch November 21, 2022 13:35


		from artemis.external_interaction.communicator_callbacks import ISPyBHandlerCallback
		from artemis.parameters import FullParameters

		from artemis.utils import Point3D


		def test_run_gridscan_zocalo_calls(

315 split fgs communicator #357

315 split fgs communicator #357

Uh oh!

Conversation

d-perl commented Nov 16, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

d-perl commented Nov 21, 2022

Uh oh!

DominicOram commented Nov 21, 2022

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

d-perl commented Nov 21, 2022

Uh oh!

DominicOram commented Nov 21, 2022

Uh oh!

Uh oh!

codecov bot commented Nov 18, 2022 •

edited

Loading