RFC Replacement for @device_factory

[tpoliaw]

Current system

The current @device_factory decorator takes a function and wraps it in a DeviceInitializationController. This controller is callable, taking a handful of its own arguments and passing the rest to the underlying function so that a device function such as def stage() -> Stage: ... can end up being called as stage(mock=True, connection_timeout=42) which isn't obviously possible from the signature. If the function itself takes a parameter that clashes with one of the wrapper's parameters, it will not be passed through.

To build these devices, the factories are found by checking all attributes of a module to see if they are instances of the wrapper (including special handling to avoid imports counting as functions again), then the dependencies between them are found and they are built in the correct order - and then connected.

The problems

• This mechanism has to be repeated/reused in blueapi when it imports a module to load devices. This is the main motivation for this change and is mentioned in Move util functions out of dodal repo #1189 and Remove dependency on dodal blueapi#1203
• Device functions take arguments that don't appear in the signature and aren't used in the function body
• There is no means of providing additional context when creating devices so that things like the path_provider need to be set as globals

Proposed new approach

Introduce a new device manager class that can keep track of all registered device factory functions. The idea would take a similar approach to fastAPI when an app is created and then endpoint functions are decorated using a method on the app. In our case, that would look something like

devices = DeviceManager()

@devices.factory
def stage_x() -> Stage:
    ...

@devices.factory
def detector() -> Detector:
    ...

The factory decorator would take the same arguments as the current device_factory (or none). The devices instance would track the factories it is aware of so a single build_all method would be enough to build all devices in a module without having to do any kind of runtime inspection of types. It would also be able to build subsets of devices and provide dependencies as required in a similar way to the current system (eg def stage(stage_x, stage_y): ... would automatically have stage_x and stage_y built and passed in).

In addition to dependencies on other devices, external dependencies could be required, eg if a detector requires a path_provider it could expect one via a parameter and the device manager could forward a user provided instance via an approach similar to fixtures in pytest. eg

@devices.factory
def detector(path_provider) -> Detector: ...

# in CLI/script/blueapi
devices.build_all(fixtures={"path_provider": PathProviderInstance()})

The functions returned by the decorator would maintain a reference to the manager allowing it to build its own dependencies if required allowing users to do something like

@devices.factory
def stage(base_x, base_y) -> Stage: ...

# in CLI/script
stage = stage.build() # returns stage with stage_x and stage_y provided by manager
# or with overrides for testing
stage = stage.build(stage_y=SpecialTestingMotor())

The factories would cache the devices in a similar way to the existing decorator so that calling stage.build() twice would return the same device.

Questions and/or remaining decisions to be made

Most of the ambiguity is around handling connecting the devices after creation.

The current factory decorator can optionally connect the device as well as build it when called (via its new connect_immediately magic parameter).

With the new approach where there are no new methods to pass directly to the factory but we could do something similar via arguments to build so that stage.build(connect_immediately=True) would connect the newly created device.

• In this case, should the dependencies also be connected? Presumably yes?

• If mock=True is passed to stage.build, should the dependencies also be mocked?

• If a dependency is mocked as part of building a parent device, should building it directly via its own build method, return the same (mocked) instance even if mock=True is not set? eg

  parent = stage.build(connect=True, mock=True)
  
  child = stage_x.build()

  in this scenario is it more surprising for child to be mocked or not connected?

• Should passing mock=True to a dependency after building the parent device
  without mock

  1. update the device
  2. leave it un-mocked
  3. fail

• Should we support calling factories multiple times?

  The current version raises an exception if a device factory is called twice.

• Should it handle connecting things at all or just concern itself with building things?

Related reading

https://github.com/DiamondLightSource/dodal/blob/main/docs/explanations/decisions/0004-make-devices-factory.md
#878 and #881

[fajinyuan]

Thanks @tpoliaw for this RFC. I am new comer in this system. I wonder if there is a way to set up a separate dedicated RFC channel or tracker in our current used system so RFC requests will no longer being buried inside normal developement issues?

[tpoliaw]

That was my thinking behind using discussions rather than the normal issues. With the 'new ideas' section it gives a single place where features/changes can be discussed before being agreed on.

I have a mostly working proof of concept of this change but would like feedback on the connection issue stuff before carrying on.

[DominicOram]

It's not explicitly stated above but after discussing with @tpoliaw I think this means that we can get an un-connected, real device (which will be a singleton owned by the manager) by doing:

from dodal.beamline.i03 import stage
my_stage = stage()

But if I want to pass arguments (so want a mock or a connected device or if the device needs arguments to build) you need to do:

from dodal.beamline.i03 import stage
my_stage = stage.build(...)

I think what this will mean for beamline users is that we will want to teach them to do my_stage = stage.build(connect=True) in the CLI/script because this gives them something connected and where they don't have to worry about the devices depending on each other. I'm not the biggest fan of it as it looks magic to a novice user e.g. you have a function (stage) and rather than calling the function you're calling a function on top of a reference to it. I think it will lead to questions/confusion (e.g. people writing my_stage = stage().build(connect=True)) but I can maybe accept that we just teach it as "that's what you do, don't think about it too hard". I discussed this with @tpoliaw in person and we decided that we couldn't find a simpler way to do it so we would just have to teach this.

We also discussed that we can remove caching and just treat calls to build individually, they will still build any dependencies for you but if I do parent.build() and child.build() I will have two instances of child. This has the potential to cause an issue if:

• People are calling individual builds (in most beamline cases they should be run using build_all e.g. as BlueAPI does)
• The child holds state outside of EPICS (we already advise against this https://diamondlightsource.github.io/dodal/main/reference/device-standards.html#holding-state)
• There is some conflict of state that means parent and child disagree and bad things happen

It was decided, with @abbiemery also in the convo, that the above was rare enough (and caused enough complexity in the building) to warrant no caching.

We also discussed that devices that depend on other devices should hold them as References (https://blueskyproject.io/ophyd-async/main/_api/ophyd_async/ophyd_async.core.html#ophyd_async.core.Reference). This means that when you call connect on a parent device it will not connect the children Reference devices. We should document in dodal how to use Reference and why.

Together I think these decisions answer all the questions above (but please add anything I've forgotten or we can rediscuss)

[tpoliaw]

Thanks @DominicOram, I think the main takeaway is that there will no longer be any caching. Calling stage.build(...) multiple times will create multiple instances of any devices meaning (which will be a singleton owned by the manager) will no longer be true.

Within a single call to build_all (or build for a single device with a complicated dependency tree) devices will be re-used but multiple calls to build_all will return an entire new set of devices.

I will get a PR together that implements the above (partial still-work-in-progress proof of concept in #1549)

---

For clarity against the original questions

• In this case, should the dependencies also be connected? Presumably yes?
• If mock=True is passed to stage.build, should the dependencies also be mocked?

Both yes, building a single device should use the same build arguments for all dependencies

• If a dependency is mocked as part of building a parent device, should building it directly via its own build method, return the same (mocked) instance even if mock=True is not set?
• Should passing mock=True to a dependency after building the parent device without mock...
• Should we support calling factories multiple times?

With no caching involved, a new instance will always be returned when calling <factory>.build(...). Passing mock true some times and false others is no longer an issue.

Should it handle connecting things at all or just concern itself with building things?

Yes. Connecting is still useful and the complexity is avoided by recreating devices each time

[tpoliaw]

it looks magic to a novice user e.g. you have a function (stage) and rather than calling the function you're calling a function on top of a reference to it [...] and we decided that we couldn't find a simpler way to do it

It would be good to hear from anyone with any ideas that would be make things more intuitive

[Villtord]

Does it mean that original idea of introducing DeviceManager is not needed anymore? If yes maybe we could still have it as a, well, rather device tracker such that we can always find out which instances of which class do we have currently initiated?

[tpoliaw]

There will still be a device manager so that build_all doesn't have to do any type inspection and dependencies can be provided. What sort of thing would you expect from a device tracker? Something like devices.instances_of(stage) to give you all the stages it has built? Could work but would have to be careful not to hold references to things for too long preventing them being dropped.

[Villtord]

Right, yes I was thinking of just a list of names of devices of certain class (possibly with passed arguments?), could be just a list of string names so no refs hold.

[tpoliaw]

Alternative to having to call ..build(...) instead of calling the method directly based on an idea from @callumforrester. Would it work to have a .auto_connect(...) method on the device manager so you could do something like

# ixx.py
devices = DeviceManager()
def m1() -> Motor:
    ...

def det() -> Detector:
    ...

# in cli
from ixx import *
devices.auto_connect(connect=True, mock=True)

motor = m1() # motor is mocked and connected

Thoughts, @DominicOram?