Reinstate `DeviceInitializationController` #881

callumforrester · 2024-11-01T19:51:11Z

Fixes #878

Instructions to reviewer on how to test:

Run unit tests
Run system tests for I22
Run system tests for at least 1 other beamline

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2024-11-06T14:40:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (fc296b5) to head (b2c9475).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #881      +/-   ##
==========================================
+ Coverage   95.63%   95.68%   +0.04%     
==========================================
  Files         128      128              
  Lines        5273     5331      +58     
==========================================
+ Hits         5043     5101      +58     
  Misses        230      230

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

callumforrester · 2024-11-06T15:16:56Z

src/dodal/utils.py

+        mock: bool,
+        skip: SkipType,
+    ):
+        self._factory: Callable[[], D] = functools.lru_cache(factory)


This should behave in a similar way to the previous cache implementation, but we now have a lot of layers of caching:

3 caches is not ideal, especially for 2 use cases. It may be that we have to leave it like this and remove it once device_instantiation is gone because each use case routes to difference caches in different ways:

blueapi python shell (manual call) python shell (calling make_all_devices)

via device_instantiation BlueskyContext.devices ACTIVE_DEVICES ACTIVE_DEVICES

via device_factory BlueskyContext.devices DeviceInitializationController.lru_cache ACTIVE_DEVICES

We should think about where to cache things long-term. A single place would be ideal, thoughts welcome.

I can't see where make_all_devices uses ACTIVE_DEVICES other than it being called by device_instantiation? So is it not:

blueapi python shell (manual call) python shell (calling make_all_devices)

via device_instantiation BlueskyContext.devices ACTIVE_DEVICES ACTIVE_DEVICES

via device_factory BlueskyContext.devices DeviceInitializationController.lru_cache DeviceInitializationController.lru_cache

In which case the two caches are just the old one and the new one and I think we can then just remove the need for the new device factory to put anything into ACTIVE_DEVICES?

Here is where it uses ACTIVE_DEVICES, albeit in a write-only capacity:

dodal/src/dodal/common/beamlines/beamline_utils.py

Line 160 in ed41f3b

controller.add_callback(lambda device: cache_device(device, factory.__name__))

So maybe you're right, can we get away without that and have a clean separation?

Yes, I think there's no need for the new instantiation method to write to it

Done, although I had to change this test so it no longer relies on ACTIVE_DEVICES, see what you think

DominicOram

Thanks, some comments in code. Additionally, I don;t think the system tests are working. When I run dodal connect i22 it passes but if I change a device to be the wrong PV it still passes, I would expect it to fail

DominicOram · 2024-11-11T11:17:13Z

src/dodal/beamlines/i22.py

+    crystal_1_metadata = CrystalMetadata(
+        usage="Bragg",
+        type="silicon",
+        reflection=(1, 1, 1),
+        d_spacing=(3.13475, "nm"),
+    )
+    crystal_2_metadata = CrystalMetadata(
+        usage="Bragg",
+        type="silicon",
+        reflection=(1, 1, 1),
+        d_spacing=(3.13475, "nm"),
+    )


Should: I think the make_crystal_metadata_from_material is cleaner

DominicOram · 2024-11-11T11:44:45Z

src/dodal/utils.py

+        mock: bool,
+        skip: SkipType,
+    ):
+        self._factory: Callable[[], D] = functools.lru_cache(factory)


I can't see where make_all_devices uses ACTIVE_DEVICES other than it being called by device_instantiation? So is it not:

blueapi python shell (manual call) python shell (calling make_all_devices)

via device_instantiation BlueskyContext.devices ACTIVE_DEVICES ACTIVE_DEVICES

via device_factory BlueskyContext.devices DeviceInitializationController.lru_cache DeviceInitializationController.lru_cache

In which case the two caches are just the old one and the new one and I think we can then just remove the need for the new device factory to put anything into ACTIVE_DEVICES?

DominicOram · 2024-11-11T11:58:39Z

tests/common/beamlines/test_beamline_utils.py

+    assert mirror.connect.call_count == 1  # type: ignore
+
+
+def test_multiple_layers_of_lru_caching_does_not_affect_device():


Nit: It's not clear why this testing the multiple layers of cache is needed, would suggest a docstring as to why it's important to test

DominicOram · 2024-11-11T12:00:49Z

tests/common/beamlines/test_beamline_utils.py

+    assert mirror.connect.call_count == 1  # type: ignore
+
+
+def test_multiple_layers_of_lru_caching_does_not_affect_device():


Should: I think it would be good to test that the returned devices are the same without having the extra cache here too

That's implicitly covered by test_device_factory_can_rename but I think you may be right that it deserves its own test

callumforrester · 2024-11-11T12:11:01Z

@DominicOram I'm aware the system tests are unhappy, should have commented here, sorry about that.

DominicOram · 2024-11-11T12:29:12Z

@DominicOram I'm aware the system tests are unhappy, should have commented here, sorry about that

No worries, the Instructions to test imply they are ok though. Will this be addressed in another issue or part of this one?

callumforrester · 2024-11-11T14:38:17Z

It will be addressed here, I don't want to merge this without addressing this problem, I just thought it had already been fixed in #858, but apparently not...

* Add DeviceFactory decorator * Convert i22 beamline to use DeviceFactory * Documenting potential pitfall with repeated connect/mock

…he implementation

DominicOram · 2024-11-11T16:21:38Z

tests/common/beamlines/test_device_instantiation.py

+    _, _, exceptions = module_and_devices_for_beamline
+    if len(exceptions) > 0:
+        raise NotConnected(exceptions)
+    # Pass test if there were no exceptions


Could: This change is fine by me. You could also still check follows_bluesky_protocols for all returned devices but I'm not sure it gives you loads. If we're not using follows_bluesky_protocols now though we should remove it.

callumforrester · 2024-11-12T11:11:54Z

It will be addressed here, I don't want to merge this without addressing this problem, I just thought it had already been fixed in #858, but apparently not...

@DominicOram I've changed my mind about this having realised that this bug also affects device_instantiation in main (albeit less), see #864.

I'm going to take over and finish #865 to address.

callumforrester force-pushed the 878-reinstate-device-initialization-controller branch 3 times, most recently from b492c0c to e47b9f7 Compare November 6, 2024 14:37

callumforrester commented Nov 6, 2024

View reviewed changes

callumforrester marked this pull request as ready for review November 6, 2024 15:17

callumforrester requested review from DominicOram, rtuck99 and DiamondJoseph November 6, 2024 15:17

DominicOram requested changes Nov 11, 2024

View reviewed changes

DiamondJoseph and others added 7 commits November 11, 2024 14:41

Isolated device factory (#841)

84b37bf

* Add DeviceFactory decorator * Convert i22 beamline to use DeviceFactory * Documenting potential pitfall with repeated connect/mock

Prevent simulated connect from trying to connect to real device (#849)

7889da7

Prevent devices connecting as side effect of use elsewhere (#860)

24469bf

Remove secondary layer of device caching

a275a94

Fix most of the tests

3343ab5

Reintroduce caching inside controller

92e1587

Add test proving that lru_cache does the same job as the previous cac…

412a4c7

…he implementation

callumforrester force-pushed the 878-reinstate-device-initialization-controller branch from ed41f3b to 412a4c7 Compare November 11, 2024 14:41

callumforrester added 3 commits November 11, 2024 15:10

Test devices identical with and without lru_caching

35193e9

No not use ACTIVE_DEVICES to cache devices created in the new way

0841f92

Reinstate use of crystal metadata helper functions on i22

b2c9475

DominicOram reviewed Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reinstate `DeviceInitializationController` #881

Reinstate `DeviceInitializationController` #881

callumforrester commented Nov 1, 2024

codecov bot commented Nov 6, 2024 •

edited

Loading

callumforrester Nov 6, 2024

DominicOram Nov 11, 2024

callumforrester Nov 11, 2024

DominicOram Nov 11, 2024

callumforrester Nov 11, 2024

DominicOram left a comment

DominicOram Nov 11, 2024

callumforrester Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

DominicOram Nov 11, 2024

callumforrester Nov 11, 2024

callumforrester commented Nov 11, 2024

DominicOram commented Nov 11, 2024 •

edited

Loading

callumforrester commented Nov 11, 2024

DominicOram Nov 11, 2024

callumforrester commented Nov 12, 2024

	blueapi	python shell (manual call)	python shell (calling `make_all_devices`)
via `device_instantiation`	`BlueskyContext.devices`	`ACTIVE_DEVICES`	`ACTIVE_DEVICES`
via `device_factory`	`BlueskyContext.devices`	`DeviceInitializationController.lru_cache`	`ACTIVE_DEVICES`

		assert mirror.connect.call_count == 1 # type: ignore


		def test_multiple_layers_of_lru_caching_does_not_affect_device():

Reinstate DeviceInitializationController #881

Are you sure you want to change the base?

Reinstate DeviceInitializationController #881

Conversation

callumforrester commented Nov 1, 2024

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

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

callumforrester commented Nov 11, 2024

DominicOram commented Nov 11, 2024 • edited Loading

callumforrester commented Nov 11, 2024

Choose a reason for hiding this comment

callumforrester commented Nov 12, 2024

Reinstate `DeviceInitializationController` #881

Reinstate `DeviceInitializationController` #881

codecov bot commented Nov 6, 2024 •

edited

Loading

DominicOram commented Nov 11, 2024 •

edited

Loading