Skip to content

Fix an issue with retry counting #749

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 3 commits into from
Jan 24, 2025
Merged

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Jan 22, 2025

This is related to #734 and came from my investigations of that issue, though I'll note that I don't think merging this will resolve the issue.

Right now, when processing function tools, we increase the retry count for each unknown tool call received, which I think is incorrect behavior — I think we should increment the retry count at most once per response.

This PR currently makes that change, but that change causes a new issue — with the implementation in this PR, raising a ModelRetry contributes to the maximum model retry count, rather than just to the tool's retries. (The test I've left failing demonstrates the issue.)

I'm not sure if I have surfaced a bug, and we should require you to increase the number of retries on Agent to avoid this raising the UnexpectedModelBehavior exception, or if the pre-existing behavior was preferred.

If the pre-existing behavior is preferred, I personally think the current distinction between treating unknown tool calls as incrementing the retry count, but tool calls that raise a ModelRetry not incrementing the retry count is rather confusing for end users (I mean, I was confused by it trying to understand what was happening in the example code). But even if you disagree with that, I definitely think the current implementation is confusing for developers — I've had to reverse engineer exactly what scenarios increment the retry count vs. not, and I think if I can get confirmation of the desired behavior, I can refactor the code to have that behavior (whatever it is) but also be easier to understand.

Copy link

cloudflare-workers-and-pages bot commented Jan 23, 2025

Deploying pydantic-ai with  Cloudflare Pages  Cloudflare Pages

Latest commit: b48558c
Status: ✅  Deploy successful!
Preview URL: https://cf236733.pydantic-ai.pages.dev
Branch Preview URL: https://dmontagu-fix-retry-counting.pydantic-ai.pages.dev

View logs

@dmontagu dmontagu merged commit 2ed3792 into main Jan 24, 2025
17 checks passed
@dmontagu dmontagu deleted the dmontagu/fix-retry-counting branch January 24, 2025 00:41
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