-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support models that return output tool args as {"response': "<JSON string>"}
#2836
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
Conversation
@shaheerzaman Thanks! I think we can implement this more cleanly using https://docs.pydantic.dev/2.0/usage/types/json/. Specifically, I think we can change this section: pydantic-ai/pydantic_ai_slim/pydantic_ai/_output.py Lines 631 to 642 in 96d895d
We can use the existing type adapter for the JSON schema generation, which should be strict, but for the Can you give that a try please? Let me know if you'd like any additional pointers. |
@DouweM I have made the changes as you suggested. |
|
||
return tool_result | ||
|
||
def _validate_tool_args( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shaheerzaman I don't think we need these changes anymore now that we use Json[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
json_schema['description'] = self._function_schema.description | ||
else: | ||
type_adapter: TypeAdapter[Any] | ||
schema_validator: SchemaValidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the PluggableSchemaValidator
type here, so that we only need to do the cast
once on the self.validator =
line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
'response_validation_typed_dict', | ||
{'response': cast(type[OutputDataT], output) | Json[cast(type[OutputDataT], output)]}, # pyright: ignore[reportInvalidTypeForm] | ||
) | ||
validation_type_adapter: TypeAdapter[Any] = TypeAdapter(response_validation_typed_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the type hint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
{'response': cast(type[OutputDataT], output) | Json[cast(type[OutputDataT], output)]}, # pyright: ignore[reportInvalidTypeForm] | ||
) | ||
validation_type_adapter: TypeAdapter[Any] = TypeAdapter(response_validation_typed_dict) | ||
schema_validator = cast(SchemaValidator, validation_type_adapter.validator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't use validation_type_adapter
elsewhere, we can inline it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
type_adapter = TypeAdapter(response_data_typed_dict) | ||
|
||
# More lenient validator: allow either the native type or a JSON string containing it | ||
# i.e., response: OutputDataT | Json[OutputDataT] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's mention the specific model that does this wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add a test for the case where the model returns nested JSON and verifies that it's parsed correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mentioned bedrock model in the comment and added a test file
… SchemaValidatorProt-based validator path
tests/test_output_json_union.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to see a single test like this one:
pydantic-ai/tests/test_agent.py
Lines 66 to 75 in 0047a68
def test_result_tuple(): | |
def return_tuple(_: list[ModelMessage], info: AgentInfo) -> ModelResponse: | |
assert info.output_tools is not None | |
args_json = '{"response": ["foo", "bar"]}' | |
return ModelResponse(parts=[ToolCallPart(info.output_tools[0].name, args_json)]) | |
agent = Agent(FunctionModel(return_tuple), output_type=tuple[str, str]) | |
result = agent.run_sync('Hello') | |
assert result.output == ('foo', 'bar') |
Can you create one like it, for the output type you ran into issues with, with the nested-JSON API response that has been failing but will now work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
# More lenient validator: allow either the native type or a JSON string containing it | ||
# i.e., response: OutputDataT | Json[OutputDataT] for some models that respond well to | ||
# instructions. in this case BedrockConverseModel - 'us.meta.llama3-2-11b-instruct-v1:0'model | ||
response_data_typed_dict = TypedDict( # noqa: UP013 # pyright: ignore[reportGeneralTypeIssues] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please show me the error we were seeing without this ignore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypedDict must be assigned to a variable named "response_validation_typed_dict"PylancereportGeneralTypeIssues
Convert response_data_typed_dict
from TypedDict
functional to class syntaxRuffUP013
(variable) response_data_typed_dict: type[response_validation_typed_dict]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shaheerzaman Ah interesting. Then maybe we can use the original variable name for the new type adapter, that will actually be used for validation, and have a new variable name for the one only used for JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
{"response': "<JSON string>"}
@shaheerzaman Thanks Shaheer! |
This PR fixes #2829
Summary
Changes
Impact
Notes for Reviewers
Applies only when outer_typed_dict_key is present (e.g., non-object output schemas).
No API changes or doc updates required.