-
Notifications
You must be signed in to change notification settings - Fork 5.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
Add the add_tool()
, remove_tool()
and remove_all_tool()
me…
#4545
base: main
Are you sure you want to change the base?
Add the add_tool()
, remove_tool()
and remove_all_tool()
me…
#4545
Conversation
a68b3de
to
8c25461
Compare
What happens (or should happen?) if one thread adds / removes tools while another thread is running a task? |
@lokitoth Currently, there is a note on AssistantAgent's API doc that says it is not thread/coroutine safe. |
@JMLX42 thanks for the PR, right now this needs a bit more design. If you are in a hurry to depend on this feature you are welcome to create a community package that extends the |
add_tools()
, remove_tools()
and remove_all_tools()()
me…add_tools()
, remove_tools()
and remove_all_tools()
me…
@ekzhu thank you for the feedback. AFAIK it meets the original goal: one can extend AssistantAgent and override class MyAgent(AssistantAgent):
async def on_messages_stream(
self, messages: Sequence[ChatMessage], cancellation_token: CancellationToken
) -> AsyncGenerator[AgentMessage | Response, None]:
query = "\n".join([msg.content for msg in messages if isinstance(msg, TextMessage)])
logger.debug(f"query: {query}")
self.remove_all_tools()
self.add_tools(
get_tools_for_query(
index=self.index,
tools=self._openapi.tools,
query=query,
embedding_func=self._embedding_func,
top_k=self._top_k,
)
)
logger.info(f"selected tools: {[tool.name for tool in self._tools]}")
async for msg in super().on_messages_stream(messages, cancellation_token):
yield msg
self.remove_all_tools() And it does not break anything.
I can do that. But since it does not break anything and it just moved some code out of |
Thanks @JMLX42
It may not break anything but put additional constraints potentially more surface area for bugs. If it turns out to be a popular feature and has a lot of usage cases we can add it. Right now, we can point folks to your extension package when need arises. |
I like the idea to add and remove tools dynamically, I think this will be useful for many users. We just need to add appropriate tests to detect any unforeseen issues. Otherwise, seems like not a bad idea to merge to main |
I would be happy to add tests. But I am not sure what "unforeseen issues" we're talking about here. Tools being removed during the tool execution phase of Anything else we should test for? |
@ekzhu or @husseinmozannar any suggestions on what tests would be appropriate? @JMLX42 maybe test for get_tools_for_query permutations/failures and for cases where on_messages_stream doesn't have the expected content? |
Question:
For unit tests, we can have unit tests for basic usage of these API methods. But I would like to understand the usage scenarios for these API methods. |
Because
I am not sure how |
It makes sense to have add_tools instead of add_tool Along with this, maybe we should have a way for the agent to export tools (because now tools could be created dynamically and we want to re-use them in the future) Some use cases for this I imagine:
|
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (1)
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py:235
- The remove_tools method should check if the tool name exists in the lists before attempting to remove it to avoid unexpected behavior.
def remove_tools(self, tool_names: List[str]) -> None:
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.
Improve the doc strings of the added methods so that the API reference renders the behavior nicely.
In tinyRA here was the workflow I used:
|
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Outdated
Show resolved
Hide resolved
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.
The PR simplifies the constructor and adds useful methods.
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Show resolved
Hide resolved
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Outdated
Show resolved
Hide resolved
I have some bias toward singular API e.g., remove_tool instead of remove_tools, for simplicity of the API and avoid creating unnecessary objects (thinking about a parallel API in .NET). Please convince me. cc @lokitoth |
I also think instead of remove all tools we should offer an api for listing the tools, and for removal we always remove one by one. So:
|
Using Python, it's trivial to do collection modification with the List method and singular add and remove. |
I agree on singular APIs to avoid unnecessary list creation. I know it's more code to add a list but it could easily be added as a mixin and it is the simplest primitive required to implement the functionality. |
@jackgerrits @ekzhu to summarize:
Correct? |
yes. thank you |
af527fd
to
db76ae3
Compare
db76ae3
to
6efa035
Compare
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Outdated
Show resolved
Hide resolved
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Outdated
Show resolved
Hide resolved
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Outdated
Show resolved
Hide resolved
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Outdated
Show resolved
Hide resolved
python/packages/autogen-agentchat/src/autogen_agentchat/agents/_assistant_agent.py
Show resolved
Hide resolved
14e4dc1
to
6f3b0bb
Compare
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 update. I think the remaining issue is updating the doc string and add a usage example in the doc string.
…s for `AssistantAgent`
6f3b0bb
to
9b9ad51
Compare
add_tools()
, remove_tools()
and remove_all_tools()
me…add_tool()
, remove_tool()
and remove_all_tool()
me…
…thods for
AssitantAgent
Why are these changes needed?
Allow the user to add/remove tools dynamically.
Related issue number
#2666
Checks