-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Feat: Content Filtering Exception Handling #3634
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?
Feat: Content Filtering Exception Handling #3634
Conversation
|
Is it possible to handle AWS Bedrock as well? Thanks. |
dsfaccini
left a comment
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.
Hey @AlanPonnachan thank you for the PR! I've requested a couple small changes, let me know if you have any questions
|
one more thing I missed, please include the name of the new exception in the fallback model docs
|
|
@dsfaccini Thank you for the review. I’ve made the requested changes. |
|
hey @AlanPonnachan thanks a lot for your work! It looks very good now, I requested a couple more changes but once that's done and coverage passes I think the PR will be ready for merge. |
|
@dsfaccini Thanks again for the review! I’ve applied the requested changes. Test coverage is now at 100%. |
DouweM
left a comment
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.
@AlanPonnachan Thanks for working on this! My main concern is that this doesn't actually make it consistent for all models that respond with finish_reason=='content_filter', just for Anthropic/Google/OpenAI.
| self.message = message | ||
|
|
||
|
|
||
| class PromptContentFilterError(ContentFilterError): |
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 name this RequestContentFilterError for consistency with our ModelRequest/ModelResponse
| """Raised when the prompt triggers a content filter.""" | ||
|
|
||
| def __init__(self, status_code: int, model_name: str, body: object | None = None): | ||
| message = f"Model '{model_name}' content filter was triggered by the user's prompt" |
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.
Or by the instructions, right? I'd prefer it to be a bit more generic
| provider_details = {'finish_reason': raw_finish_reason} | ||
| finish_reason = _FINISH_REASON_MAP.get(raw_finish_reason) | ||
| if finish_reason == 'content_filter': | ||
| raise ResponseContentFilterError( |
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 think we should do this in ModelRequestNode, so that it'll work for all models, not just Anthropic.
That would mean that it wouldn't trigger FallbackModel to try another model, but that will be possible once we do #3640.
| if finish_reason == 'content_filter' and raw_finish_reason: | ||
| raise UnexpectedModelBehavior( | ||
| f'Content filter {raw_finish_reason.value!r} triggered', response.model_dump_json() | ||
| raise ResponseContentFilterError( |
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.
This is a breaking change for users that currently have except UnexpectedModelBehavior. So if we add a new exception, I think it should be a subclass of UnexpectedModelBehavior, not ModelAPIError (as it isn't really an API/HTTP/connection error).
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.
Combined with the above, the solution may be to remove this check here, and to implement the same if response.finish_reason == 'content_filter' check in ModelRequestNode that will apply to all models.
If we want to get some details from the response up to that level, we can store them in response.provider_details
| if (error := body_dict.get('error')) and isinstance(error, dict): | ||
| error_dict = cast(dict[str, Any], error) | ||
| if error_dict.get('code') == 'content_filter': | ||
| raise PromptContentFilterError( |
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.
What do you think about instead handling this by returning a ModelResponse with empty parts and finish_reason == 'content_filter', so that it can go through the same logic as the Anthropic and Google models?
I also don't think they distinguish between input/request and output/response content filter errors, so I don't think we need to here either.
| By default, the `FallbackModel` only moves on to the next model if the current model raises a | ||
| [`ModelAPIError`][pydantic_ai.exceptions.ModelAPIError], which includes | ||
| [`ModelHTTPError`][pydantic_ai.exceptions.ModelHTTPError]. You can customize this behavior by | ||
| [`ModelHTTPError`][pydantic_ai.exceptions.ModelHTTPError] and [`ContentFilterError`][pydantic_ai.exceptions.ContentFilterError]. You can customize this behavior by |
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.
This is no longer true
| ctx: GraphRunContext[GraphAgentState, GraphAgentDeps[DepsT, NodeRunEndT]], | ||
| response: _messages.ModelResponse, | ||
| ) -> CallToolsNode[DepsT, NodeRunEndT]: | ||
| if response.finish_reason == 'content_filter': |
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.
Before this change, some content filter errors would likely have gone unnoticed because the model could've returned some response parts before the filter was hit and the agent run would've continued as normal, or the model could've returned an empty parts list and we would've auto-retried, which may have succeeded.
So I'm very hesitant to turn this behavior, which is likely working fine for some in production, into a hard error, especially one that can't be recovered by FallbackModel...
So at the very least I think we should only do this if the model returned no parts and it's very unlikely that retrying will help, by moving the check here:
pydantic-ai/pydantic_ai_slim/pydantic_ai/_agent_graph.py
Lines 602 to 609 in eaedf8a
| if not self.model_response.parts: | |
| # Don't retry if the model returned an empty response because the token limit was exceeded, possibly during thinking. | |
| if self.model_response.finish_reason == 'length': | |
| model_settings = ctx.deps.model_settings | |
| max_tokens = model_settings.get('max_tokens') if model_settings else None | |
| raise exceptions.UnexpectedModelBehavior( | |
| f'Model token limit ({max_tokens or "provider default"}) exceeded before any response was generated. Increase the `max_tokens` model setting, or simplify the prompt to result in a shorter response that will fit within the limit.' | |
| ) |
Note that that's also what the GoogleModel logic did.
Of course there are also scenarios where you would want to catch every content filter hit, and always try a different model or fail hard.
The previous ModelAPIError based approach would've worked with FallbackModel, but still feels too aggressive to me (unless we limit it to responses with 0 parts, which would also make it less useful).
So perhaps the upcoming #3640 that will make it very easy to fallback based on response.finish_reason is enough? That in combination with only raising this error if there are 0 parts.
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.
Either way the behavior we end up with will need documentation!
| chat.chat_completion.Choice( | ||
| finish_reason='content_filter', | ||
| index=0, | ||
| message=chat.ChatCompletionMessage(content='', role='assistant'), |
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.
Do we need this? I'd want it to result in a ModelResponse with 0 parts, and this may result in a TextPart(content='')
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.
It may be cleaner to just build that ModelResponse directly...
Unified Content Filtering Exception Handling
This PR closes #1035
Standardizing how content filtering events are handled across different model providers.
Previously, triggering a content filter resulted in inconsistent behaviors: generic
ModelHTTPError(Azure),UnexpectedModelBehavior(Google), or silent failures depending on the provider. This PR introduces a dedicated exception hierarchy to allow users to catch and handle prompt refusals and response interruptions programmatically and consistently.Key Changes:
ContentFilterError(base),PromptContentFilterError(for input rejections, e.g., Azure 400), andResponseContentFilterError(for output refusals).PromptContentFilterErrorfor Azure's specific 400 error body andResponseContentFilterErrorwhenfinish_reason='content_filter'._process_responseto raiseResponseContentFilterErrorinstead ofUnexpectedModelBehaviorwhen safety thresholds are triggered.ResponseContentFilterError.tests/models/.Example Usage: