-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Set ToolRetryError message
#3718
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?
Conversation
ToolRetryError was not passing a message to Exception.__init__(), causing str(error) to return an empty string. This breaks error monitoring tools like Sentry that rely on str(exception) to display error messages. The fix passes tool_retry.model_response() to the parent class, ensuring the error message is properly accessible via str(error) and error.args.
| def __init__(self, tool_retry: RetryPromptPart): | ||
| self.tool_retry = tool_retry | ||
| super().__init__() | ||
| super().__init__(tool_retry.model_response()) |
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 tool_retry.content that contains just the error message, without the surrounding text instructing the model to retry?
Besides a str, content can also be a list of Pydantic error details, so we would need to stringify/dump those as to 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.
Great, thanks for the info! Using tool_retry.content now. Added a bit of formatting code to make list[ErrorDetail] more human readable.
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.
Plz wait for re-review - need to fix CI.
ToolRetryError message
3b384a2 to
09a0ca4
Compare
4f7147a to
916a8e1
Compare
tests/test_exceptions.py
Outdated
| expected = """\ | ||
| Tool 'my_tool' failed: 2 validation errors | ||
| field1: Field required | ||
| field2: Input should be a valid string""" |
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 bit lossy because we don't get to see the original input or error type. Can you see if we can use the same rendering that is used when Pydantic ValidationError is raised and printed in terminal? We get the ErrorDetails from a ValidationError here, so maybe we can pass in the string message here where we still have that error:
pydantic-ai/pydantic_ai_slim/pydantic_ai/_tool_manager.py
Lines 184 to 190 in d2b08ad
| if isinstance(e, ValidationError): | |
| m = _messages.RetryPromptPart( | |
| tool_name=name, | |
| content=e.errors(include_url=False, include_context=False), | |
| tool_call_id=call.tool_call_id, | |
| ) | |
| e = ToolRetryError(m) |
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.
Thank you for the suggestion! I simplified the scope of the PR to explicitly pass str(e) from the call sites.
If callers do not provide message, then it will be blank.
Another option would be to fallback to lossy behavior as a backup if message is not passed.
| tool_call_id=call.tool_call_id, | ||
| ) | ||
| e = ToolRetryError(m) | ||
| e = ToolRetryError(m, message=e.message) |
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 wouldn't mind setting message automatically if isinstance(tool_retry.content, str)
| assert d[exc] == 'value' | ||
|
|
||
|
|
||
| def test_tool_retry_error_str(): |
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 test that it is set correctly in the ValidationError case
| content=e.errors(include_url=False), | ||
| ) | ||
| raise ToolRetryError(m) from e | ||
| raise ToolRetryError(m, message=str(e)) from e |
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.
If ToolRetryError.__init__ is able to access the __cause__ that's set by from e (I'm not sure if it can), we could do this automatically
Summary
ToolRetryErrorwas not passing a message toException.__init__(), causingstr(error)to return an empty string.This came up when the agent loop crashes due to max tool retries, which breaks error monitoring tools like Sentry or Langsmith that rely on
str(exception)to display and group errors.Problem
Solution
Pass
tool_retry.model_response()to the parentException.__init__():Changes
pydantic_ai_slim/pydantic_ai/exceptions.py: Pass message to parent classtests/test_exceptions.py: AddToolRetryErrorto hashable test + add dedicated string representation test