Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make device_factory decorator to ease making and connecting ophyd-async Devices #483

Closed
coretl opened this issue Apr 29, 2024 · 7 comments · Fixed by #841
Closed

Make device_factory decorator to ease making and connecting ophyd-async Devices #483

coretl opened this issue Apr 29, 2024 · 7 comments · Fixed by #841
Assignees
@stan-dot

Comments

@coretl
Copy link
Collaborator

coretl commented Apr 29, 2024

#415 (comment) summarizes a discussion on Device connection.
bluesky/ophyd-async#265 will implement the ophyd-async supporting features of idempotent connect. The strategy we would like for dodal is:

  • Each beamline module decorates its device factories with a device_factory
  • This factory makes, names, caches and optionally connects a device
  • In script use can pass connect=True to the device to connect in the foreground, requires a RunEngine to have been made
  • In blueapi use can find all the eager devices and connect them at startup
  • Can also connect them in a plan with the ensure_connected plan stub

This ticket is for:

New ADR

Describing how Devices should be connected in dodal, from scripts and under blueapi

dodal.beamlines.beamline_utils

D = TypeVar(bound=Device)
class DeviceFactory(Protocol, Generic[D]):
    def __call__(self, connect: bool = False, timeout: float = DEFAULT_TIMEOUT) -> D:
        ...

_factory_made_devices: Dict[DeviceFactory, Device] = {}
_device_is_lazy: Dict[DeviceFactory, bool] = {}

def device_factory(lazy=False, set_name=True):
    def wrapper(f) -> DeviceFactory[D]:
        def factory(connect=False, timeout: float = DEFAULT_TIMEOUT):
            device = _factory_made_devices.get(f)
            if not device:
                device = f()
                if set_name:
                    device.set_name(f.__name__)
                _factory_made_devices[f] = device
            if connect: 
                call_in_bluesky_event_loop(device.connect(timeout=timeout))
            return device
        _device_is_lazy[factory] = lazy
        factory.__name__ = f.__name__
        return factory
    return wrapper

def get_device_factories() -> Dict[DeviceFactory, bool]:
    return _device_is_lazy.copy()

Need to add type hints to this so it knows the type of device it is making from the function it is decorating and check with @callumforrester that get_device_factories is what blueapi actually needs

Acceptance Criteria

  • ADR written so we know why this was done
  • device_factory available to use in beamline modules
  • i22.py updated to use it
This was referenced Apr 29, 2024
Open
Merged
Closed
@coretl coretl mentioned this issue Jun 3, 2024
Closed
@stan-dot stan-dot mentioned this issue Jun 5, 2024
Closed
4 tasks
@stan-dot stan-dot linked a pull request Jun 5, 2024 that will close this issue
make device factory decorator #597
Closed
4 tasks
@stan-dot
Copy link
Contributor

stan-dot commented Jun 6, 2024

so the idea is that all device()... line in i22.py or other beamline gets this decorator, right?

@coretl
Copy link
Collaborator Author

coretl commented Jun 7, 2024

so the idea is that all device()... line in i22.py or other beamline gets this decorator, right?

yes

@stan-dot
Copy link
Contributor

@coretl what is the role of keeping the lazy devices in a global dict? why is the key a complex object?

def get_device_factories() -> Dict[DeviceFactory, bool]: return _device_is_lazy.copy()
@callumforrester how would blueapi use this? Maybe this should be stateful in some class, not a function in beamline_utils?

@stan-dot
Copy link
Contributor 

ACTIVE_DEVICES: Dict[str, AnyDevice] = {}
BL = ""
DIRECTORY_PROVIDER: UpdatingDirectoryProvider | None = None

there is loads of global state in there! very surprising.

@stan-dot
Copy link
Contributor

graphviz

classes graph

@stan-dot
Copy link
Contributor

already_existing_device: AnyDevice | None = ACTIVE_DEVICES.get(name)
this line in device_instantiation makes most of the difference

@coretl
Copy link
Collaborator Author

coretl commented Jun 18, 2024 •
edited
Loading

@coretl what is the role of keeping the lazy devices in a global dict? why is the key a complex object?

As written in the original issue:

  • In blueapi use can find all the eager devices and connect them at startup
  • Can also connect them in a plan with the ensure_connected plan stub

So it looks like this:

  • At startup connect in parallel {device.connect() for device, lazy in get_device_factories().items() if not lazy}
  • When running a plan in parallel {device.connect() for device in whatever_function_you _use_to_inject_devices_in_plan_args}

But this is all specified in DiamondLightSource/blueapi#440

@stan-dot stan-dot mentioned this issue Jul 2, 2024
Closed
This was referenced Aug 9, 2024
Merged
Open
@stan-dot stan-dot mentioned this issue Aug 19, 2024
Closed
This was referenced Sep 25, 2024
Closed
Open
Closed
@DiamondJoseph DiamondJoseph mentioned this issue Oct 15, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stan-dot stan-dot

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@stan-dot @coretl