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

Support composite devices in plans #506

Open
olliesilvester opened this issue Jun 14, 2024 · 6 comments
Open

Support composite devices in plans #506

olliesilvester opened this issue Jun 14, 2024 · 6 comments

Comments

@olliesilvester
Copy link

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
Copy link
Collaborator

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

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

@DominicOram
Copy link
Contributor

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
Copy link
Collaborator

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

coretl commented Jun 18, 2024

@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 DiamondLightSource/dodal#415 (comment)

@callumforrester
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants