DiamondLightSource / blueapi

Apache License 2.0
5 stars 6 forks source link

Support composite devices in plans #506

Open olliesilvester opened 3 months ago

olliesilvester commented 3 months ago

As a user of Blueapi, I would like to run plans which contain composite devices.

In Hyperion, our plans use a lot of devices so we have something like

class FlyScanXRayCentreComposite:
    aperture_scatterguard: ApertureScatterguard = inject(...)
    attenuator: Attenuator = inject(...)
    ...

def flyscan_xray_centre(
    composite: FlyScanXRayCentreComposite,
    ...

BlueAPI doesn't seem to be happy with this kind of thing right now. When doing a blueapi controller plans, the server outputs: ValueError: Value not declarable with JSON Schema, field: name='aperture_scatterguard' type=ApertureScatterguard required=True

callumforrester commented 3 months ago

FlyScanXrayCentreComposite is a dataclass rather than an ophyd/ophyd-async device, which is why blueapi doesn't like it. It checks whether things being injected as arguments are plans or devices as different behaviour is needed for each.

I guess we have 2 options:

  1. Make blueapi consider dataclasses to be a type of "device"
  2. Make the composite devices actual devices (probably StandardReadables or just plain ophyd-async Devices)

How would you feel about option 2? I'm not hugely against option 1 but it would be nice to keep blueapi's definition of a device somewhat idomatic for future-proofing purposes.

olliesilvester commented 3 months ago

I can't see any issues with doing 2. I think @DominicOram also mentioned this as a possibility

DominicOram commented 3 months ago

If it's a Device do we have to do the instantiation of the sub devices inside it? I'm not a big fan of that because we'll end up with potentially multiple instances. Alternatively we could get something like https://github.com/DiamondLightSource/hyperion/blob/main/src/hyperion/utils/context.py#L47 in to blueapi?

callumforrester commented 3 months ago

Hmm, I don't think so, if the devices already exist and you pass them in via the constructor, they may get .connect() called on them twice, but it is idempotent and cached to reduce network calls so that's not the end of the world. @coretl or @abbiemery can you see any problems with this approach?

coretl commented 3 months ago

@coretl or @abbiemery can you see any problems with this approach?

The other issue is naming. If you name the parent it will attempt to name the child.

The other option is to do the connect explicitly in a plan as proposed in https://github.com/DiamondLightSource/dodal/issues/415#issuecomment-2072066251

callumforrester commented 3 months ago

From a discussion on Slack we think the idiomatic way to do this in blueapi is:

class MyComposite(BlueapiBaseModel):
    x: Motor = inject("x")
    y: Motor = inject("y")
    det: Eiger = inject("det1")

def my_plan(composite: MyComposite) -> MsgGenerator:
    ...

Unsure if it will do this out of box, if it doesn't, this ticket will become a bug report.

stan-dot commented 2 weeks ago

@olliesilvester have you tried the implementation suggested above?

olliesilvester commented 2 weeks ago

No, I haven't tried this yet