Allow client_tools to be defined only once#142
Allow client_tools to be defined only once#142MichaelClifford wants to merge 2 commits intollamastack:mainfrom
client_tools to be defined only once#142Conversation
|
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.
validate that there's no conflict if agent_config already sets this value?
There was a problem hiding this comment.
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.
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.
LGTM. Could you please go ahead and remove instances of the old API from the codebase?
There was a problem hiding this comment.
@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.
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.
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.
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.