DiamondLightSource / dodal

Ophyd devices and other utils that could be used across DLS beamlines
Apache License 2.0
2 stars 8 forks source link

Standardise Device Instantiation #153

Closed callumforrester closed 6 months ago

callumforrester commented 1 year ago

Currently not all beamlines have adopted device_instantiation. It would be neat if all devices were initialized in the same way, As I understand it the requirements are

An example of the current solution used on I03:

@skip_device(lambda: BL == "s03")
def eiger(
    wait_for_connection: bool = True,
    fake_with_ophyd_sim: bool = False,
    params: Optional[DetectorParams] = None,
) -> EigerDetector:
    """Get the i03 Eiger device, instantiate it if it hasn't already been.
    If this is called when already instantiated in i03, it will return the existing object.
    If called with params, will update those params to the Eiger object.
    """

    def set_params(eiger: EigerDetector):
        if params is not None:
            eiger.set_detector_parameters(params)

    return device_instantiation(
        device_factory=EigerDetector,
        name="eiger",
        prefix="-EA-EIGER-01:",
        wait=wait_for_connection,
        fake=fake_with_ophyd_sim,
        post_create=set_params,
    )

Which can then be invoked like so:

from dodal.beamlines.i03 import eiger

def my_plan():
    eiger = eiger()  # Only makes a new Eiger if none exists, otherwise uses cached version
    yield from do_stuff_with(eiger)

I think this is moving in the right direction and I'm somewhat happy to move the test rigs to this model, however I have a couple of concerns that would be good to address sooner rather than later. Firstly I'm worried about maintaining the boilerplate: The set of parameters to device_instantiation cannot really change now as it will be called by every single device factory. Second I feel this is a bit obfuscating, in the above example I would quite like to see just the logic of how to make an eiger, and all the logic of making a fake eiger, storing it in a backing store etc. being abstracted somewhere else (with an override in case any unusual logic is required). I'm wondering if a decorator would be a better fit, something like

@device(wait_for_connection=True)
def eiger(profile: str, params: Optional[DetectorParams] = None) -> EigerDetector:
    pv_prefix = i03_prefix(profile)  # either i03 or s03
    eiger = EigerDetector(name="eiger", prefix=f"{pv_prefix}-EA-EIGER-01:")
    if params is not None:
        eiger.set_detector_parameters(params)

The decorator handles all of the boilerplate seen earlier. It's signature is still not that flexible but is at least no longer tied to the factory signature. There would be default routines for making a fake version and storing the singleton somewhere that could be optionally overridden.

Now is probably also a good time to make sure the design works well with both Ophyd v1 and v2. For example, they both handle device names a bit differently. Will make a separate issue for that.

All thoughts on the above welcome. Paging @DominicOram and @dperl-dls. If there's appetite for the decorator concept I'm happy to have a go at it for the test rigs and see if it looks like something we can use.

callumforrester commented 1 year ago

Option discussed with @coretl in #154 is to make a new device context manager that works with both v1 and v2 devices, example usage would be something like:

from dodal import DeviceContextManager
from dodal.beamlines.i03 import eiger   # Ophyd v1 device
from dodal.beamlines.i03 import special_detector  # Ophyd v2 device 

def my_plan():
    with DeviceContextManager(ensure_connected=True, profile="s03"):
        eiger = eiger()
        sd = special_detector()
callumforrester commented 1 year ago

Decision here is to migrate all devices to ophyd-async and replace device_instantiation with decorators. Also add a "profile" mechanism to cover multiple groups of devices being instantiated. There should still be a make_all_devices function of some sort either in dodal or blueapi.

dperl-dls commented 6 months ago

Seems like most of the relevant discussion for this is actually in https://github.com/DiamondLightSource/dodal/issues/415#issuecomment-2071965438 now so I'm closing this but feel free to reopen if you think the separate issues are still useful