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.

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

Standardise Device Instantiation #153

Closed
callumforrester opened this issue Aug 25, 2023 · 3 comments
Closed

Standardise Device Instantiation #153

callumforrester opened this issue Aug 25, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@callumforrester
Copy link
Contributor

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

  • Invoke a function that makes a specific device (for a specific beamline)
  • Can be configured to either perform or skip the step of ensuring all PVs are connected
  • Can be configured to make alternative devices based on profiles (e.g. dummy objects, S03)
  • Store the device as a singleton in a backing store somewhere
  • Should be possible to be lazy (make devices as-needed) or eager (make all devices at once for a particular beamline)
  • Should be introspectable so that something like blueapi can ask "what devices exist on beamline x?"
  • All above should be configurable globally and then individually
  • Should be maintainable, changes should not break all previous devices

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@d-perl
Copy link
Contributor

d-perl commented Apr 23, 2024

Seems like most of the relevant discussion for this is actually in #415 (comment) now so I'm closing this but feel free to reopen if you think the separate issues are still useful

@d-perl d-perl closed this as completed Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants