-
Notifications
You must be signed in to change notification settings - Fork 97
Allow client_tools to be defined only once
#142
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
|
Thanks! LGTM to improve SDK ergonomics. |
| ): | ||
| self.client = client | ||
| self.agent_config = agent_config | ||
| self.agent_config["client_tools"] = [client_tool.get_tool_definition() for client_tool in client_tools] |
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.
validate that there's no conflict if agent_config already sets this value?
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.
Thanks for the feedback @ehhuang! Would a simple if statement, where we only update the agent_config if client_tools does not already exist work? Like this:
if "client_tools" not in self.agent_config.keys():
self.agent_config["client_tools"] = [client_tool.get_tool_definition() for client_tool in client_tools]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.
After reading the other threads, I agree it would be nicer to have the tools defined in the agent_config instead of the agent. We could modify the Agent initilization with something like:
if "client_tools" in self.agent_config.keys():
self.client_tools = {t.get_name(): t for t in agent_config["client_tools"]}
self.agent_config["client_tools"] = [tool.get_tool_definition() for tool in agent_config["client_tools"]]That way it just modifies and sets the the client tools values from whatever is in the agent_config. WDYT?
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.
LGTM. Could you please go ahead and remove instances of the old API from the codebase?
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.
@ehhuang Sounds good. It looks like the only place where the old instances of the API will need to be removed here is in the react/agent.py. I can also make another PR to llama-stack-apps to update its use there as well. I think its only in a couple of spots. And I do not think this impacts the llama-stack repo at all. Let me know if you think there's somewhere else that needs to be updated too. Thanks!
98bf162 to
686a1ba
Compare
|
Update: Went ahead and updated the PR so that there is no longer a |
| instructions=instruction, | ||
| toolgroups=builtin_toolgroups, | ||
| client_tools=[client_tool.get_tool_definition() for client_tool in client_tools], | ||
| client_tools=client_tools, |
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 don't think this will work as client_tools are list of ClientTool objects, whereas AgentConfig assumes these are of JSON formats.
Could you test the change a script? E.g. https://github.com/meta-llama/llama-stack-apps/blob/main/examples/agents/react_agent.py
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 can confirm that this does work with https://github.com/meta-llama/llama-stack-apps/blob/main/examples/agents/react_agent.py. This is due to the fact that the way this PR is able to use AgentConfig instead of Agent to define client_tools is that it converts CustomTool into JSON format during the Agent initialization. But I admit this essentially violates the the current expected type for client_tools in AgentConfig (which is defined as an Iterable[ToolDefParam] )
yanxi0830
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.
See comment in https://github.com/meta-llama/llama-stack-client-python/pull/142/files#r1974435609
I think your previous version makes more sense.
|
Thanks for the feedback @yanxi0830 😄 After reading #160, it sounds like there is still some outstanding discussions on the best way to simplify the use of client tools with agents. For now I can go ahead and revert to the previous version and move any further discussions into that issue. Sounds like a proper fix might be a bit more involved and require some additional changes in LlamaStack. |
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
b2a75d6 to
95952a2
Compare
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
|
@MichaelClifford I'm closing this PR, as the functionality is added with #178 |
What does this PR do?
This PR aims to address an issue I noticed where
client_toolshas to be declared twice in two different way in order to work properly. It has to be declared in theAgentConfigwith something liketool.get_tool_definition(), as well as in theAgent. See the example below.This PR updates the
Agentclass initialization to setagent_config["client_tools"]based on theAgentclass'sclient_toolsparameter so that the user only needs to declareclient_toolsonce and not worry about the.get_tool_definition()list comprehension.Test Plan
I've confirmed that these code changes work as expected using the
llamastack/distribution-ollama:latestimage as the local Llama Stack server. You can run the code snippet below to verify.You should see an output below that has correctly called the CustomTool.