Isolated device factory

[DiamondJoseph]

Fixes #483

This is a re-implementation of #597 isolated to just the changes necessary to implement the @device_factory() decorator and have functions decorated with that decorator picked up by make_all_devices: respecting any configuration passed into the decorator.

NB: this is also required for blueapi #647

Instructions to reviewer on how to test:

1. System test the i22 beamline to ensure that devices are instantiated, connected and named appropriately.

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.19%. Comparing base (9e12af0) to head (9030d80).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #841      +/-   ##
==========================================
+ Coverage   95.12%   95.19%   +0.07%     
==========================================
  Files         120      120              
  Lines        4899     4976      +77     
==========================================
+ Hits         4660     4737      +77     
  Misses        239      239              

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

[callumforrester]

I don't necessarily expect parallel connect to be implemented here but do we have tickets for achieving it?

[DiamondJoseph]

I don't necessarily expect parallel connect to be implemented here but do we have tickets for achieving it?

for blueapi there's DiamondLightSource/blueapi#440
There should probably be one for dodal connect (and async that by default) when more beamlines are able to make use of it.

[stan-dot]

nice functools use

[DominicOram]

Great, thank you!

Some comments in code, mostly minor. I think a few more tests would also be good. e.g. we can test (assuming these are all desired behavior?):

• A device gets renamed if called again with a different name
• If we call with connect=False, mock=True we can then call again with mock=False

Additionally I think there's a potential pitfall in:

dcm(mock=True)
.... # Lots of code
dcm() # This now fails because it tries to turn the cached mock one into a real one

I'm not 100% sure if this is expected or if you would rather give the cached mocked one, maybe with a warning. We should at least document it but can do so in #780.

[DominicOram]

Thanks. I'm still a little concerned about the not 100% intuitive behavior of calling the factory twice with different permutations of connect/mock but I thin we can document/fix that later.

[DiamondJoseph]

Assuming these are all desired behavior?

@coretl for comments?

* A device gets renamed if called again with a different name

A device can be renamed after having a name, so I think it should be allowed. Currently the device_factory name is only used if the device is currently unnamed.

* If we call with `connect=False, mock=True` we can then call again with `mock=False`

Only when connect is called is the device being mock or not considered. Calling with connect=False, mock=True then later connecting with mock=False would connect to the real signal backend.

Additionally I think there's a potential pitfall in:

dcm(mock=True)
.... # Lots of code
dcm() # This now fails because it tries to turn the cached mock one into a real one

Ophyd-async throws an exception if you try and connect a 2nd time with a different value of mock.
. I will amend to cache the value of mock after connect has been called at least one, and defer to it rather than the mock value set up with the decorator, so that we can still try and explicitly connect to [non-]mock, but will throw the exception only when we do so.

[DominicOram]

I will amend to cache the value of mock after connect has been called at least one, and defer to it rather than the mock value set up with the decorator, so that we can still try and explicitly connect to [non-]mock, but will throw the exception only when we do so.

I'm not even sure that is the best answer either. You then end up with the above code connecting you to a mock when you actually want a real device and potentially you fail to realise. I'm not sure there's a good solution here. I think the best thing to do is just document that calling the factory multiple times with different arguments can lead to odd behavior and to be careful.

[callumforrester]

I agree there is no good solution to be honest, caching singletons is hard to do without some hidden magic. Maybe add more logging (but in a separate change).