Skip to content

Conversation

@rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Oct 17, 2025

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

codecov bot commented Oct 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.69%. Comparing base (812c427) to head (015ecae).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1242      +/-   ##
==========================================
+ Coverage   94.67%   94.69%   +0.01%     
==========================================
  Files          41       41              
  Lines        2611     2618       +7     
==========================================
+ Hits         2472     2479       +7     
  Misses        139      139              

☔ 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.

@rtuck99 rtuck99 marked this pull request as ready for review October 20, 2025 10:20
@rtuck99 rtuck99 requested a review from a team as a code owner October 20, 2025 10:20

## Injecting multiple devices

If a plan requires multiple devices to be injected at once, rather than have a plan with several device parameters each of them with their own injection default, it is preferable to define a device composite which can be accepted as a parameter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Successfully merging this pull request may close these issues.

3 participants