Ensure action output models are models #239
Merged
Conversation
This file contains hidden or 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
This removes the check that the ThingClient returns an ndarray instance, instead only checking that the right value is returned as a list of numbers. I've also added a test for reading a property that's a numpy array.
I've used `wrap_plain_types_in_rootmodel` to encapsulate action return types in a RootModel if they are not already BaseModel subclasses. This stops annotations from being lost when actions are returned. I needed to slightly adjust a test that checked action output models, I think that change is uncontroversial.
Contributor
|
This is very minimal way to achieve the state of affairs we thought we were at before the bug was discovered. So I certainly think this should be merged as is. I think we keep #232 as an open draft as it is a partial solution to #200. It would also be good to make a note in #200 or in another issue about ThingClient returning a numpy array not a list. |
julianstirling
approved these changes
Jan 5, 2026
Collaborator
Author
|
Thanks. I agree this only solves part of the problem - but I'm hoping it's the part that is currently on your critical path, so that's useful. It's always satisfying and frustrating in equal measure when lots of thinking results in a tiny code change! |
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.
This uses
wrap_plain_types_in_rootmodelso thatActionDescriptor.output_modelis always a subclass ofpydantic.BaseModel. This brings its behaviour in line with that oflt.property(where we differentiate between thevalue_typeand itsmodel, with the latter always being aBaseModelsubclass).This fixes a bug that was ignoring annotations on action return types, identified in #231. There's an explanation there of what I believe is going on. When the specific invocation model is converted to a generic one by FastAPI, it preserves the value (and type) of each field but not annotations. Using a
RootModelinstead of an annotated type effectively bakes in any annotation as part of the class, and so it gets preserved and serialises correctly.I had thought the fix was more involved and would require separate endpoints for invocations of each action. I think this fix is much neater.