-
Notifications
You must be signed in to change notification settings - Fork 8
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
make device factory decorator #597
base: main
Are you sure you want to change the base?
make device factory decorator #597
Conversation
94124b4
to
27c1b79
Compare
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.
No idea if the rest of the code is right until some tests are written...
To clarify, the code in the original ticket is an idea, but has never been run or tests. It is expected that it is wrong in some way, and writing tests to exercise it will reveal how it is wrong... |
docs/developer/explanations/decisions/0003-added-make-devices-decorator.rst
Outdated
Show resolved
Hide resolved
f2ad915
to
fb5e142
Compare
26368ca
to
6cb1db6
Compare
do we want to add 'skip device' to the factory too? we could have just one decorator |
This is a question for @callumforrester and @DominicOram. I don't know what a |
|
it's an architectural question. We can have all the logic that belongs to 'making device class instances' inside a bunch of nested decorators that need to be in right order. OR we can have 1 factory class that accepts a dictionary of features (lazy, mock, skip, directory provicder, post-create script) in one place. |
I have a weak preference for a single decorator, but defer to @callumforrester and @DominicOram for a decision |
Single decorator works for me. |
ok I'll prepare a proof of concept for a single decorator |
6cb1db6
to
49e3bd2
Compare
beamline_utils.py 180 but that is inside ophyd, not sure how it works for ophyd-async |
0ecf386
to
656f918
Compare
e436ef6
to
f5a3fcf
Compare
todos:
|
@callumforrester from the blueapi perspective is the feature set enough (nonwithstanding the documentation and testing part, just the API exposed to blueapi)? |
Co-authored-by: Tom C (DLS) <[email protected]>
Co-authored-by: Tom C (DLS) <[email protected]>
Co-authored-by: Tom C (DLS) <[email protected]>
Co-authored-by: Tom C (DLS) <[email protected]>
16bfb79
to
3f9227f
Compare
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.
@stan-dot, apologies for not looking at this for a while. See DiamondLightSource/blueapi#440 for blueapi's requirements.
Does this API allow me to ask dodal for a list of all device factory functions for a particular beamline?
Having collected them all, how do I connect them in parallel? Or is that for a later PR?
Tagging @coretl in case he can help.
CONTROLLERS: dict[str, DeviceInitializationController] = {} | ||
|
||
|
||
def instantiation_behaviour( |
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.
Should: Apologies if there's already been a discussion around it, but can we change this name? I'm concerned because _behaviour
is a redundant suffix (see point 8 here) and because it didn't make sense to me until I read a function it was decorating. Some alternatives:
device_instantiation
device_factory
device_configuration
device_singleton
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.
device_instantiation
is bad for a function name here, as device_instantiation
is an event that happens whenever we call the waxs()
(where waxs = some_device), so a function with that name is confusing
factory
is technically true but it's an oop statement fitting for a class, but here it is a decorator
device_configuration
is underspecified - this is specifically device_iniitalization_configuration
singleton
is inaccurate as it does not prevent by itself more than 1 instance afaik
add_instantiation_behavior_to_device
would be my suggestion.
Zen of Python says:
- Explicit is better than implicit.
- Sparse is better than dense.
- Readability counts.
description="AVT Mako G-507B", | ||
distance=(-1.0, "m"), | ||
), | ||
metadata_holder=metadata_holder, | ||
directory_provider=get_directory_provider(), | ||
) | ||
|
||
|
||
@skip_device() |
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: Is @instantiation_behaviour
now rich enough that we can replace calls to @skip_device
with it?
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.
yes, that is the premise of the function signature, not 100% sure about the implementation details still
|
||
def __call__( | ||
self, | ||
connect=False, |
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.
connect=False, | |
connect: bool = False, |
CONTROLLERS[self.factory.__name__] = self | ||
self.device = None | ||
|
||
def see_if_device_is_in_cache(self) -> AnyDevice | None: |
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.
Should: I would prefer more conventional terminology here:
def see_if_device_is_in_cache(self) -> AnyDevice | None: | |
def get_device_if_cached(self) -> AnyDevice | None: |
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.
will implement suggestions tomorrow
beamline_prefix = "example:" | ||
|
||
|
||
@instantiation_behaviour( | ||
default_use_mock_at_connection=True, default_timeout_for_connect=10 | ||
) | ||
def new_detector_xyz(): | ||
"""Create an XYZ detector with specific settings.""" | ||
return XYZDetector(name="det1", prefix=f"{beamline_prefix}xyz:") | ||
|
||
|
||
@instantiation_behaviour( | ||
eager=True, default_use_mock_at_connection=True, default_timeout_for_connect=10 | ||
) | ||
def detector_xyz_variant(): | ||
"""Create an XYZ detector with specific settings.""" | ||
return XYZDetector(name="det2-variant", prefix=f"{beamline_prefix}xyz:") |
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: Could the example module be in its own file? I'd find that more readable.
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.
sure! any path and name suggestions @callumforrester ?
Fixes #483
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}