-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Pass pydantic validation context #3448
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
base: main
Are you sure you want to change the base?
Pass pydantic validation context #3448
Conversation
Add tests involving the new 'validation context' for: - Pydantic model as the output type - Tool, native and prompted output - Tool calling - Output function - Output validator
| try: | ||
| self._next_node = await self._handle_text_response(ctx, text, text_processor) | ||
| self._next_node = await self._handle_text_response( | ||
| ctx, ctx.deps.tool_manager.validation_ctx, text, text_processor |
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 already pass in ctx, we don't need a new arg do we?
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.
Would it make sense to store the validation context directly on ctx.deps, instead of going through the tool manager?
| self, | ||
| data: str, | ||
| run_context: RunContext[AgentDepsT], | ||
| validation_context: Any | None, |
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 have a default of None
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.
Also while we're at it, let's require kwargs for everything after data, by inserting , *,
| def validate( | ||
| self, | ||
| data: str | dict[str, Any] | None, | ||
| allow_partial: bool = False, |
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.
Same as up, let's require kwargs for everything but data
| model_settings: Optional model request settings to use for this agent's runs, by default. | ||
| retries: The default number of retries to allow for tool calls and output validation, before raising an error. | ||
| For model request retries, see the [HTTP Request Retries](../retries.md) documentation. | ||
| validation_context: Additional validation context used to validate all outputs. |
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 link to the Pydantic doc
| result_data = await text_processor.process(text, run_context, validation_context) | ||
|
|
||
| for validator in ctx.deps.output_validators: | ||
| result_data = await validator.validate(result_data, run_context) |
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.
Ideally, output validators would have access to the validation context as well, as they could call model_validate themselves. They already have access to RunContext, so maybe it'd make sense to store the validation context on there? As the validation context callable itself needs RunContext, building run context could look like ctx = <run context>; validation_ctx = callable(ctx); ctx = replace(ctx, validation_ctx = validation_ctx)
I think that'd be a refactor worth exploring that could allow us to drop a lot of the new arguments.
This PR adds a new
validation_contextparameter to theAgent.This object is either
Anyor a callable on the agent's run context returningAny. It is fed to the Pydantic validation of all outputs as additional contextRelated issue