[oliwenmandiamond]

Fixes #1719

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.46%. Comparing base (4353ae2) to head (47d0af8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1720      +/-   ##
==========================================
- Coverage   96.46%   96.46%   -0.01%     
==========================================
  Files          49       49              
  Lines        3791     3787       -4     
==========================================
- Hits         3657     3653       -4     
  Misses        134      134              

☔ View full report in Codecov by Harness.
📢 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.

[oliwenmandiamond]

@tpoliaw I've had another go at this. I've tried to make it easier to understand the intention and the flaws with previous implementation via doc strings and tests. Please have another review and let me know what you think

[tpoliaw]

I still think this is making it much more complicated than it needs to be. A lot of the changes here are duplicating the logic we're already doing when registering the plan in the context.

I tested this and this incorrectly adds to args when should add to kwargs

This is what I meant by "do we care how things get passed to the plan". For a plan foo(a: int, b: int = 2) there is no difference between calling it as foo(3,4), foo(3, b=4) or foo(a=3, b=4).

the issue with the current approach is that the model would provide all default values, making default values provided by plan not used

This is already the case and was before any of the vararg support was added. A plan defined as

def foo(shared: list[int] = []) -> MsgGenerator:
    shared.append(42)
    print(shared)
    yield from ()

Will never print a list with more than a single element as the python defaults are only used to set pydantic defaults, and then everything is passed to the function.

I'll have a look at this and try and figure out if anything breaks by making the handling here as light as possible.

[oliwenmandiamond]

I still think this is making it much more complicated than it needs to be. A lot of the changes here are duplicating the logic we're already doing when registering the plan in the context.

I tested this and this incorrectly adds to args when should add to kwargs

This is what I meant by "do we care how things get passed to the plan". For a plan foo(a: int, b: int = 2) there is no difference between calling it as foo(3,4), foo(3, b=4) or foo(a=3, b=4).

the issue with the current approach is that the model would provide all default values, making default values provided by plan not used

This is already the case and was before any of the vararg support was added. A plan defined as

def foo(shared: list[int] = []) -> MsgGenerator:
    shared.append(42)
    print(shared)
    yield from ()

Will never print a list with more than a single element as the python defaults are only used to set pydantic defaults, and then everything is passed to the function.

I'll have a look at this and try and figure out if anything breaks by making the handling here as light as possible.

Just because that is the current behaviour doesn't necessarily make it the right behaviour.

I think there are two separate concerns here:

1. Binding and validating explicitly supplied arguments, including variadic parameters.
2. Avoiding supplying ordinary defaults explicitly, so Python applies the plan's own defaults.

I don't think the second point is redundant just because the Pydantic model uses Python defaults when it is constructed. Once the model is validated and all its fields are passed to the plan, those defaults are explicitly supplied, rather than Python applying them from the plan instead. This can change behaviour for mutable defaults and other cases where the actual Python default matters.

I also think this hides potential edge cases that could trip someone up in the future. We shouldn't have a situation where the plan's defaults are effectively ignored because blueapi always supplies its own defaults. What we pass to the plan in terms of *args and **kwargs absolutely matters. Ideally, we should preserve exactly what the user supplied and only intervene when we need to inject devices (hopefully objects in the future).

We can simplify this if we can identify injected parameters more directly rather than inferring them from string defaults and annotations. We previously discussed replacing inject("device") with something like Inject("device") to make injection explicit, but that is outside the scope of this PR.

The implementation is approximately 40 lines of actual code, with the remainder 110 being documentation and tests. I don't think that is unreasonable given this is a bug fix and the parameter types and edge cases we're supporting, particularly with regression tests demonstrating the behaviour.

I'd prefer to get this fix merged so we can test it against a new blueapi release, rather than holding it up for a potentially larger refactor?

[tpoliaw]

This can change behaviour for [...] other cases where the actual Python default matters

I might have missed these. What other cases are there where the actual default matters?

We shouldn't have a situation where the plan's defaults are effectively ignored because blueapi always supplies its own defaults

The values blueapi supplies via the plan model defaults are the same as the plan defaults. Nothing is being ignored.

What we pass to the plan in terms of *args and **kwargs absolutely matters

Why?

The implementation is approximately 40 lines of actual code, with the remainder 110 being documentation and tests

The issue isn't in lines of code, it's the fact it's duplicating logic from the context. Sure, the way we bind and convert arguments could be improved (#1624 et al) but while we're still using pydantic, it makes sense to only use one approach.

[oliwenmandiamond]

This can change behaviour for [...] other cases where the actual Python default matters

I might have missed these. What other cases are there where the actual default matters?

See below

We shouldn't have a situation where the plan's defaults are effectively ignored because blueapi always supplies its own defaults

The values blueapi supplies via the plan model defaults are the same as the plan defaults. Nothing is being ignored.

This isn't necessarily true though. The main example I had in mind for Python defaults is mutable defaults. Although the values supplied by the Pydantic model may be equal to the function defaults, they aren't necessarily the same objects. Pydantic deep-copies non-hashable defaults when constructing model instances, whereas Python function defaults retain the same object between calls. Consequently, passing the model's default explicitly could change the behaviour of a plan that relies on that state being retained. I agree this is a fairly specific case and isn't necessarily how I want plans to be constructed in DAQ, but it is a difference in Python's default-argument semantics. I don't want to write a plan and tests in one environment and they all pass fine and then when I switch to use it through blueapi it behaves completely differently because blueapi decides to handle the arguments to my plan different and weirdly again. Another unnecessary edge case.

What we pass to the plan in terms of *args and **kwargs absolutely matters

Why?

This issue hangs around being able to provide *args to plans when blueapi only made everything **kwargs. That is now resolved, but changing what the user supplied as args and kwargs is why we got in this situation to begin with so it matters if something is supplied as *args and **kwargs. This bug occurred because blueapi attempts to add defaults for values that aren't needed. Again, if blueapi doesnt interfere with user supplied args and kwargs, this bug wouldn't appear. We have to interfere to resolve names to devices, and that is it.

The implementation is approximately 40 lines of actual code, with the remainder 110 being documentation and tests

The issue isn't in lines of code, it's the fact it's duplicating logic from the context. Sure, the way we bind and convert arguments could be improved (#1624 et al) but while we're still using pydantic, it makes sense to only use one approach.

Pydantic is more used for generating a schema though for UI's and client so user knows what a plan args and kwargs expect. It is also used to resolve devices. Again, I don't agree with using it to override plan defaults as the defaults this can lead to more edge cases down the line. Sure, we can maybe make the logic overall better. This is a bug fix though and fixes the bug that is resulted from the current logic, blueapi overriding plan defaults with its own defaults. That is more refactoring which outside the scope of this issue

[oliwenmandiamond]

Okay, having talked about this IRL I understand the differences in point of view. I am viewing this as a means to call Python, core see this as rest api calls only. This isn't a hill I will die on and would rather get it fixed. @tpoliaw I have done the changes you have asked, please have another review