-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: allow grouping multiple tools under a single confirmation strategy #10522
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
feat: allow grouping multiple tools under a single confirmation strategy #10522
Conversation
|
@srini047 is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
Pull Request Test Coverage Report for Build 21859363526Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
julian-risch
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.
@srini047 The code changes look good to me! I only added an example to the release note. Thank you for your contribution! I will merge this PR.
| function=multiplication_tool, name="multiplication_tool", description="A tool that multiplies two integers." | ||
| ) | ||
|
|
||
| return [add_tool, mult_tool] |
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.
Changing this fixture here looks good to me. In general, we need to be careful because of side effects on other tests that use this fixture. In particular, test_confirmation_strategy_context_passed_to_strategy_async is using tools. The change seems okay to me but next time I would recommend creating a separate fixture to prevent any unwanted effects on other tests.
…egy (#10522) * feat: allow grouping multiple tools under a single confirmation strategy * fix: revert unrelated change * fix: add rn * fix: refactor fixture logic * fix: cosmetic change * extend release note with an example --------- Co-authored-by: Julian Risch <julian.risch@deepset.ai>
Related Issues
Proposed Changes:
Make use of tuple based key for
confirmation_strategiesat the same time still acceptstrfor backward compatibility.How did you test it?
Run all tests to ensure no breakage and add new test to validate multiple tool call.
Notes to the reviewer
Not sure how to link a issue from haystack-experimental
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.