-
Notifications
You must be signed in to change notification settings - Fork 12
Example conversion to the new device manager #1684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1035326 to
984894d
Compare
11dcde0 to
6e29bb8
Compare
984894d to
7c7244e
Compare
DominicOram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is generally ok. I have made DiamondLightSource/mx-bluesky#1459 which fixes most of the tests on the mx-bluesky side I believe.
DominicOram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you take a look at the tests in test_device_instantiation.py?
6e29bb8 to
87ded47
Compare
79265a2 to
33ffd3a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1684 +/- ##
==========================================
- Coverage 99.13% 99.10% -0.03%
==========================================
Files 279 279
Lines 10504 10510 +6
==========================================
+ Hits 10413 10416 +3
- Misses 91 94 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rtuck99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the docs and made the test_device_instantiation.py cover i03 again, everything else looks fine, will need DiamondLightSource/mx-bluesky#1459 as well when merged
f94bf5e to
f626761
Compare
56a74f1 to
a6b6a3f
Compare
a6b6a3f to
aa3882c
Compare
9af2428 to
9ccf338
Compare
|
With the manager itself merged I will tidy this up now to make it ready for merging |
src/dodal/beamlines/i03.py
Outdated
|
|
||
| @devices.factory() | ||
| def aperture_scatterguard() -> ApertureScatterguard: | ||
| """Get the i03 aperture and scatterguard device, instantiate it if it hasn't already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove these docstrings as they're lies
Most devices are a straightforward swap to the new decorator. The main things to
be aware of:
a, b = synchrotron.build(), synchrotron.build()will result in two synchrotrons.Adding
@cachemight work if the function takes no other devices but givenmost devices aren't hashable it probably won't work otherwise.
devices.build_all()will only build one of each device, so two devicesdepending on the same third device will be passed the same instance.
parameter), it will create a separate instance.
Other things specific to i03's module
post_create method, that would be where it was. Given ophyd v1 support is not
meant to be long-lived, this will do for now.
the methods that require them are called. When using the device manager, this
is automatic, eg
panda.build()will automatically create the path providervia the fixture. BlueApi will pass its own path_provider which would be used
instead.
undulatorandundulator_dcmnow take their configuration path as arequired argument rather than using the global if not set. This will be taken
from the fixture unless overridden. When the two devices are built at the same
time (eg
undulator_dcm.build()which buildsundulatoras a dependency)both will use the same configuration path.
The CLI has already been updated so this can be tested via
with or without
-sto mock things as before. The-n devicesis currentlyneeded to determine what the device manager is called. If we standardise on a
convention for this, that can be removed.