feat: Support for dataclasses as composite types

[rtuck99]

This modifies the previous PR #1231 and adds the following:

• Simplifies the creation of composites, the composite no longer needs to be an instance of BaseModel and instead can be a dataclass
• The composite does not need to specify a default and instead determines the device to inject from the attribute name

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.93%. Comparing base (d8a989c) to head (5d7952f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1242      +/-   ##
==========================================
+ Coverage   94.92%   94.93%   +0.01%     
==========================================
  Files          42       42              
  Lines        2717     2724       +7     
==========================================
+ Hits         2579     2586       +7     
  Misses        138      138              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DiamondJoseph]

I don't agree that it is preferable- if you require overriding one device you must override the whole Composite.

I just think it's an alternative option that may be useful quite often, depending on how a specific beamline operates.

I'll look through the code change later, but I am admitting upfront that I am opinionated

[rtuck99]

Hmm. Perhaps I should rephrase that to reflect the difference between UDC and non-UDC use cases.

[DominicOram]

I'm not sure it's a UDC vs. non-UDC thing. It's just if you have loads of devices it's cleaner to use a composite. If you have a couple it's cleaner not to

[DiamondJoseph]

I don't think it's number but flexibility

class XYComposite(BaseModel):
  x: Motor
  y: Motor

def my_composite_plan(xy: XYComposite = inject()) -> MsgGenerator:
  ...


def my_individual_plan(x: Motor = inject(), y: Motor = inject()) -> MsgGenerator:
  ...

the json for the former if I wanted to scan theta/y instead of x/y:

{"xy": {"x": "theta", "y": "y"}}

compared to the latter:

{"x": "theta"}

The more devices I have in my BaseModel, the more verbose my plan signature would become if I don't use one, but the more duplicates I need to have in my JSON if I want to replace one of them.

If I'm always scanning x, y or I have multiple plans that all need x, y then a composite is neater. I'd prefer to define a specific device like a Stage to enforce a meaning to the relationship between x and y, but it's not always possible.

Or we support multiple levels of injection:

class MyComposite(BaseModel):
  x: Motor = inject()
  y: Motor = inject()

def my_plan(xy: MyComposite = inject()) -> MsgGenerator:  # uses x, y
  ...

{"xy": {"x": "theta"}}

Creating a new MyComposite with x=theta, y=inject("y") which finds y to pass in as the xy argument?

[rtuck99]

The way that I see it, if you are writing a plan which is intended to be consumed directly by scientists, for commissioning or one-off scripts explicitly inject the individual devices in the parameters, since that provides more flexibility over device selection. But I think most of those will probably not be called via blueapi.

There may be a use case with blueapi for individual device injection for things such as UI components if there are multiple similar devices e.g. slits, mirrors etc. and we want to provide direct control / monitoring over them for commissioning purposes. (although currently this functionality is mainly provided by the EDM)

If you have a plan that is intended to support a single frequently invoked high-level plan that will be called from a client application such as the hyperion supervisor, or GDA, then it's better to have a composite, because you almost certainly don't want or need the flexibility to inject different devices - client applications probably don't want to have to know about a beamline's specific configuration or what the naming of devices is, they just want to run a plan with whatever experimental parameters it requires.

If there's a choice of different devices for some section of the plan, the plan probably has to do other things that are bespoke for each device. For example, on i03, we can run with the panda or the zebra, but the initialisation and tear down code is different for each, so there would be no benefit to injecting only one device or the other.

[tpoliaw]

you almost certainly don't want or need the flexibility to inject different devices

Should we have an option/annotation to hide a parameter from the API so a plan that takes a single composite appears to have no parameters?

[rtuck99]

you almost certainly don't want or need the flexibility to inject different devices

Should be have an option/annotation to hide a parameter from the API so a plan that takes a single composite appears to have no parameters?

Currently composite parameters don't appear in the json schema, due to this in _type_spec_for_function():

            if (
                isclass(arg_type)
                and (issubclass(arg_type, BaseModel) or is_dataclass(arg_type))
                and isinstance(para.default, str)
            ):
                default_factory = self._composite_factory(arg_type)
                _type = SkipJsonSchema[self._convert_type(arg_type, no_default)]

I added this because otherwise schema generation was broken and didn't work.

Unless you actually would like the option to specify the composite? Then I guess we'd have to do something different.

[tpoliaw]

Do we have any ophyd(-async) devices which are also BaseModels or dataclasses? If not we could remove the str requirement and not have to use inject("") for composite devices where it doesn't really make sense.

[rtuck99]

The issue I see with this is that it would also mean that Pydantic models which are not device composites (i.e. parameter models) would also automatically become defaulted, which is probably not what we want.

There is a PR DiamondLightSource/dodal#1649 which at least makes the string argument to inject optional