Skip to content

Fix model construct #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 5, 2025
Merged

Fix model construct #71

merged 8 commits into from
Mar 5, 2025

Conversation

guillaq
Copy link
Collaborator

@guillaq guillaq commented Mar 5, 2025

Found an issue with autopilot where we were streaming dictionaries instead of pydantic objects when the output had nested objects.

The issue was undetected until now because:

@guillaq guillaq requested a review from yannbu March 5, 2025 02:13
partial=True,
)
for ingredient in validated.ingredients:
assert isinstance(ingredient, Recipe.Ingredient)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to check ingredient.name == "salt" ?

assert isinstance(constructed, ListOfModels)
assert isinstance(constructed.models, list)
assert isinstance(constructed.models[0], NestedModel)
assert isinstance(constructed.models[1], NestedModel)
Copy link
Contributor

@yannbu yannbu Mar 5, 2025

Choose a reason for hiding this comment

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

So models[1] is a NestedModel without the "field2" attribute?

That would mean that accessing NestedModel.field2 on the chunk processing will raise an AttributeError. And this error can be predicted by the type check... So that will crash at runtime.

I don't have the perfect solution for that. Maybe we should enforce all agent output classes to only have optionals when streaming in the newest SDK versions ? If not raise an error even before starting the stream ?

This way field2 will be None instead of absent; and you are forced by the type checker to handle the case where "field2" is None in the stream processing...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We mention the caveat of non optional fields in the Readme.
There is no way to do "partial validation" in Pydantic unfortunately.

Another way to handle the issue would be to compute "healthy defaults" for each field but it might get quite complex.. I'll make a quick attempt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok 👌, thank you!
Having warnings when streaming an output with non-optional would be useful too.

@yannbu
Copy link
Contributor

yannbu commented Mar 5, 2025

thanks @guillaq, one important comment, I think model_construct is a dangerous function because it makes type checking completely useless :/

Also, do we have an issue to update the SDK version for our API?

@guillaq
Copy link
Collaborator Author

guillaq commented Mar 5, 2025

@yannbu I managed to create an optional version of pydantic models to allow for partial validation of payloads.
Basically like a "simple" version of https://github.com/team23/pydantic-partial/tree/main

I'll check autopilot with it.

@guillaq
Copy link
Collaborator Author

guillaq commented Mar 5, 2025

Also, do we have an issue to update the SDK version for our API?

We do not

@guillaq guillaq merged commit fd8a163 into main Mar 5, 2025
5 checks passed
@guillaq guillaq deleted the guillaume/fix-model-construct branch March 5, 2025 16:18
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.

2 participants