Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JMLX42
Copy link
Contributor

@JMLX42 JMLX42 commented Dec 4, 2024

…thods for AssitantAgent

Why are these changes needed?

Allow the user to add/remove tools dynamically.

Related issue number

#2666

Checks

@JMLX42 JMLX42 force-pushed the feat/add-remove-assistant-agent-tools branch from a68b3de to 8c25461 Compare December 4, 2024 18:52
@lokitoth
Copy link
Member

lokitoth commented Dec 5, 2024

What happens (or should happen?) if one thread adds / removes tools while another thread is running a task?

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 5, 2024

@lokitoth Currently, there is a note on AssistantAgent's API doc that says it is not thread/coroutine safe.

https://microsoft.github.io/autogen/dev/reference/python/autogen_agentchat.agents.html#autogen_agentchat.agents.AssistantAgent

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 6, 2024

@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 BaseAgent and implement a custom agent. I will include this for consideration for 0.4.1.

@ekzhu ekzhu added this to the 0.4.1 milestone Dec 6, 2024
@JMLX42 JMLX42 changed the title Add the add_tools(), remove_tools() and remove_all_tools()() me… Add the add_tools(), remove_tools() and remove_all_tools() me… Dec 6, 2024
@JMLX42
Copy link
Contributor Author

JMLX42 commented Dec 6, 2024

thanks for the PR, right now this needs a bit more design

@ekzhu thank you for the feedback.

AFAIK it meets the original goal: one can extend AssistantAgent and override on_messages_stream() to have a dynamic tool list:

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.

If you are in a hurry to depend on this feature you are welcome to create a community package that extends the BaseAgent and implement a custom agent.

I can do that. But since it does not break anything and it just moved some code out of __init__ into add_tools() it sounds a bit extreme to make a completely separate 3rd party package.

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 6, 2024

Thanks @JMLX42

I can do that. But since it does not break anything and it just moved some code out of init into add_tools() it sounds a bit extreme to make a completely separate 3rd party package.

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.

@husseinmozannar
Copy link
Contributor

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

@JMLX42
Copy link
Contributor Author

JMLX42 commented Dec 8, 2024

We just need to add appropriate tests to detect any unforeseen issues

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 on_messages_stream()? IMO that can easily be deal with by copying self._tools before executing the tool calls. Since AFAIK that's the only place where self._tools is read it should work just fine.

Anything else we should test for?

@rysweet rysweet requested a review from ekzhu December 10, 2024 17:32
@rysweet
Copy link
Collaborator

rysweet commented Dec 10, 2024

@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?

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 10, 2024

@JMLX42

Question:

  1. add_tool vs add_tools? Is there are a reason to use plular? same for remove_tools
  2. Can we use save_state on an existing AssistantAgent and load_state on a new instance of AssistantAgent with different tools?

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.

@JMLX42
Copy link
Contributor Author

JMLX42 commented Dec 10, 2024

@ekzhu

add_tool vs add_tools? Is there are a reason to use plular? same for remove_tools

Because add_tools([MyTool]) works for a single tool, but making add_tool() work with multiple tools involves a lot more code/work.

Can we use save_state on an existing AssistantAgent and load_state on a new instance of AssistantAgent with different tools?

I am not sure how save_state() is supposed to work. Is there some existing up to date documentation?

@husseinmozannar
Copy link
Contributor

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:

  • teams creating new tools and adding them on the fly for a use case of a continuously learning agent. One agent generates a tool and adds it to another agent
  • teams figure out to remove tools because the tools don't work or are no longer needed.
  • teams re-writing existing tools to improve them. a kind of dynamic tool re-writing

@gagb gagb requested review from Copilot and gagb December 11, 2024 19:57

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:
Copy link
Collaborator

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.

@gagb
Copy link
Collaborator

gagb commented Dec 11, 2024

In tinyRA here was the workflow I used:

  1. user request arrives
  2. backend constructs an assistant agent with given tools
  3. the agent does the work
  4. the user clicks a button which triggers an LLM call to extracts a new tool (e.g., python function)
  5. ui displays the tool draft, user edits it and saves it
  6. a bunch of backend checks validate the tool
  7. append the valid tool to the list of available tools
  8. a new user request arrives, and repeat step 2

@gagb gagb self-requested a review December 11, 2024 20:08
Copy link
Collaborator

@gagb gagb left a 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.

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 11, 2024

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

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 11, 2024

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:

  1. Add
  2. Remove
  3. List

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 11, 2024

Using Python, it's trivial to do collection modification with the List method and singular add and remove.

@jackgerrits
Copy link
Member

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.

@JMLX42
Copy link
Contributor Author

JMLX42 commented Dec 12, 2024

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:

  • add_tool(self, tools: List[Tool]) => add_tool(self, tool: Tool)
  • remove_tools(self, tool_names: List[str]) => remove_tool(self, tool_names: str)
  • remove_all_tools(self) is left unchanged
  • add a @property tools(self) -> List[Tool]

Correct?

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 13, 2024

add_tool(self, tools: List[Tool]) => add_tool(self, tool: Tool)
remove_tools(self, tool_names: List[str]) => remove_tool(self, tool_names: str)
remove_all_tools(self) is left unchanged
add a @Property tools(self) -> List[Tool]

yes. thank you

@JMLX42 JMLX42 force-pushed the feat/add-remove-assistant-agent-tools branch from af527fd to db76ae3 Compare December 16, 2024 10:50
@JMLX42
Copy link
Contributor Author

JMLX42 commented Dec 16, 2024

yes. thank you

@ekzhu done in db76ae3

@JMLX42 JMLX42 force-pushed the feat/add-remove-assistant-agent-tools branch from db76ae3 to 6efa035 Compare December 16, 2024 15:53
@JMLX42 JMLX42 force-pushed the feat/add-remove-assistant-agent-tools branch 2 times, most recently from 14e4dc1 to 6f3b0bb Compare December 16, 2024 18:30
Copy link
Collaborator

@ekzhu ekzhu left a 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.

@JMLX42 JMLX42 force-pushed the feat/add-remove-assistant-agent-tools branch from 6f3b0bb to 9b9ad51 Compare December 17, 2024 09:15
@JMLX42
Copy link
Contributor Author

JMLX42 commented Dec 17, 2024

Thanks for the update. I think the remaining issue is updating the doc string and add a usage example in the doc string.

@ekzhu done in 9b9ad51

@JMLX42 JMLX42 changed the title Add the add_tools(), remove_tools() and remove_all_tools() me… Add the add_tool(), remove_tool() and remove_all_tool() me… Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants