Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
haystack/tools/from_function.py
Outdated
| # Skip State-typed parameters — ToolInvoker injects them at runtime, hidden from the LLM | ||
| if param.annotation is State: | ||
| continue |
There was a problem hiding this comment.
@anakin87 the additional code to skip State in the function params isn't too much but I realize now with the pre-built tools we don't even need it since I directly create their parameter schemas.
|
Thank you for working on this! The POC looks nice and clean. Would it be worth getting a second opinion? Quick point: I'm not sure if we want to explicitly refer to skills here, unless we have broader support for them, like also reading from skills files, etc. WDYT? |
kacperlukawski
left a comment
There was a problem hiding this comment.
@sjrl, thanks for working on this! I love the idea of injecting State if the tool signature defines it, but didn't quite get why we want to have a specific set of tools doing the basic operations. Is that only for the POC purposes, or do you want to keep that in the core?
| return f"State key '{key}' updated successfully." | ||
|
|
||
|
|
||
| class LsStateTool(Tool): |
There was a problem hiding this comment.
Although it's clear to me what's the purpose of this tool, wouldn't it be a better idea to just call it ListStateTool? It would make it more explicit, and I imagine some users can be unfamiliar with unix.
There was a problem hiding this comment.
Yeah that's fair happy to go with ListStateTool
@kacperlukawski thanks for taking a look! I explained in more detail in the original issue but I'll repeat the main points here
|
Related Issues
Stateobject to aToolif it's in its function signature #10868Proposed Changes:
Example script
Example output
How did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.