-
Notifications
You must be signed in to change notification settings - Fork 57
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
Allow client_tools
to be defined only once
#142
base: main
Are you sure you want to change the base?
Conversation
Thanks! LGTM to improve SDK ergonomics. |
@@ -29,6 +29,7 @@ def __init__( | |||
): | |||
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 |
@@ -83,7 +83,7 @@ def get_tool_defs(): | |||
model=model, | |||
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]
)
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>
What does this PR do?
This PR aims to address an issue I noticed where
client_tools
has to be declared twice in two different way in order to work properly. It has to be declared in theAgentConfig
with something liketool.get_tool_definition()
, as well as in theAgent
. See the example below.This PR updates the
Agent
class initialization to setagent_config["client_tools"]
based on theAgent
class'sclient_tools
parameter so that the user only needs to declareclient_tools
once 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:latest
image 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.