- Notifications
You must be signed in to change notification settings - Fork 1.5k
Make end_strategy also work for output tools, not just tools #3523
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
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.
@Danipulok Thank you! I'll likely make some tweaks to the docs before merging, but first please have a look at my code comments
| @DouweM, I have upgraded the PR |
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.
@Danipulok Thanks!
Addresses PR pydantic#3523 comments: - Execute all output tool functions for side effects in exhaustive mode - Use first valid output result regardless of order - Don't fail run if already have valid result and output_retries=0 - Add tests for invalid-first/valid-second and valid-first/invalid-second scenarios - Update documentation to clarify 'first valid' behavior - Create dedicated End Strategy section in tools-advanced.md Changes: - _agent_graph.py: Handle UnexpectedModelBehavior/ToolRetryError when final_result exists - tests/test_agent.py: Add 2 critical exhaustive strategy tests - docs/output.md: Convert to note block, clarify first valid result - docs/tools-advanced.md: Add ### End Strategy section for ToC visibility
| Hey @DouweM! While I was writing tests for this PR, I discovered that output tools are called twice (or more times) in streaming mode (but only once in sync mode). The first call happens at DetailsOutput tool processors are invoked:
Here's some MRE, written by an LLM: gist QuestionThis causes issues for output tools, which should be called only once. What's the appropriate way to handle this - should the result be cached after the first call, or what's the appropriate solution? And should this be changed at all and in this PR or in PR for #3473 or some other PR? (I'm just asking for better understanding) About current PRShould I add tests for this behavior at all? I want to duplicate all tests in |
| @Danipulok Hmm I agree that's wrong or at the least unexpected. Can you file a new issue for that please? We'll want to discuss a solution separately from these PRs.
Sounds good |
….py:TestMultipleToolCalls`
| @DouweM, I'm proud to announce that I have finished everything I wanted I think most of the time was spent on organizing and understanding the tests, but I think it was worth it |
Co-authored-by: Douwe Maan <me@douwe.me>
Co-authored-by: Douwe Maan <me@douwe.me>
Co-authored-by: Douwe Maan <me@douwe.me>
| @DouweM , I have updated the PR, please check it The only question for me is: #3523 (comment) |
| @Danipulok I'll review and hopefully merge today; appreciate the patience! By the way are you on our Slack already? Could be useful to have a more direct line of communication than only GH :) |
Head branch was pushed to by a user without write access
Co-authored-by: Douwe Maan <me@douwe.me>
Closes #3485
Changes:
end_strategy='exhaustive';How I verified:
end_strategy='exhaustive'should call all output functions #3485 now successfully calls both output tools;MRE:
Old output:
New output: