-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Tool
dataclass - unified abstraction to represent tools
#8652
Conversation
Pull Request Test Coverage Report for Build 12391961949Details
💛 - Coveralls |
Tool
dataclassTool
dataclass - unified abstraction to represent tools
@dfokina take a look at this when possible |
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.
Only one comment - should we undermine UX/DX for agents and tools over 80kb binary? :-)
function: Callable | ||
|
||
def __post_init__(self): | ||
jsonschema_import.check() |
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.
Knowing that Tool is central piece of the Agents push, should be we make this dependency default? It's 80Kb binary
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'm not totally sure. Let's involve also @julian-risch in the decision.
- I remember that when we introduce new dependencies, we also have to pay attention to Anaconda.
- We had some issues in the past with
jsonschema
: ci: Skip collection oftest_json_schema.py
to fix CI failures #7353
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 seems to be not occurring on 3.9 and after, but ok, let's get this integrated and then we can experiment with including it as a default dependency. Or not.
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 capitalized all "Tools" in the docstrings so that it looks unified :)
* draft * del HF token in tests * adaptations * progress * fix type * import sorting * more control on deserialization * release note * improvements * support name field * fix chatpromptbuilder test * port Tool from experimental * release note * docs upd * Update tool.py --------- Co-authored-by: Daria Fokina <daria.fokina@deepset.ai>
Related Issues
Proposed Changes:
Tool
dataclass from experimental (introduced in feat: Tool class haystack-experimental#53 + feat: convert function into Tool haystack-experimental#114)How did you test it?
CI, several new tests
Notes for the reviewer
DO NOT MERGE
This PR is currently based on the
new-chatmessage
branch, to allow me to create other PRs for Chat Generators, which require both newChatMessage
andTool
.Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.