Support inject for default plan arguments again #773
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #773 +/- ##
==========================================
+ Coverage 93.32% 93.36% +0.04%
==========================================
Files 38 38
Lines 2083 2081 -2
==========================================
- Hits 1944 1943 -1
+ Misses 139 138 -1 ☔ View full report in Codecov by Sentry. |
callumforrester
approved these changes
Jan 10, 2025
tpoliaw
commented
Jan 10, 2025
tpoliaw
force-pushed
the
default_arg_conversion
branch
from
cbc830f to
34d501f
Compare
|
Added a couple of tests to cover the 'normal' use and the failure case in 633. They're in |
tpoliaw
force-pushed
the
default_arg_conversion
branch
from
34d501f to
be46b33
Compare
|
Tagging @olliesilvester for awareness |
When preparing the parameters for a plan, pydantic creates an instance of the dynamically generated BaseModel, relying on the 'Reference' types to convert from strings to instances of the devices. This includes strings that are used as default field values (default parameters created using the `inject` method). We then convert the model back to a dictionary to pass as kwargs to the plan. The previously used `model_fields_set` field on the model only iterates over the fields set via the input JSON (the user supplied arguments) and skips the fields populated via the default factories in the base model. Using the `__pydantic_fields__` class variable, allows all fields to be used including the defaults. This was previously avoided as it generated warnings for unknown types but it is possible to opt-out of these warnings.
callumforrester
force-pushed
the
default_arg_conversion
branch
from
be46b33 to
de4b5e2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let inject work again by using all fields of the built pydantic model including the default ones provided by inject
Fixes #647, #633
Instructions to reviewer on how to test:
injectfor default parameters, (egblueapi controller run stp_snapshot '{"detectors":["x"]}'to run the plan fromexample_plans)sample_temperatureandsample_pressure. (It may fail at the end of the run with a different error relating to path providers for the detector but this is unrelated).inject("<misssing_device_name>")(as for Blueapi context should except when it can't find an injected device #633), the plan will not be accepted and blueapi will return422 Unprocessable Entity. The detailed error message is generated for this but it's not immediately obvious how to access it. Either way, that should be its own issue.Checks for reviewer