-
Notifications
You must be signed in to change notification settings - Fork 64
feat: unify agent types #139
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
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
…docs Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
this is ready for review, but i still would like to add tests to the PR before we send it. I looked and there is an e2e test github workflow that would be good to run, but looks like that still needs some setting up... so i guess that'll be another separate effort. |
Should this pr also consider #121 as well since it's refactoring the base agent types? |
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Thanks for the feedback @cecilphillip ! Yes, good point. I am working on a different approach with just oneeeee agent type. So let me get some more progress there and can add the dapr chat client param for agent instantiation in the better option :) |
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. I need to run it locally now.
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
@Cyb3rWard0g Thanks for the feedback! I'm going to make a few more changes just to rm the OpenAPIReAct agent and ReAct agents under the hood so it's the case that we have:
For visibility, the reasoning is that the DurableAgent workflow implementation uses ReAct pattern anyways, so we don't really need it and the underlying ReAct existing class. Also, OpenAPIReAct agent can just be transformed into a quickstart and we can keep the vector store capabilities, but not need to keep that as a concrete class. I need to add tests too :) Will mark the PR ready for review when I'm done with all of the above in this comment! |
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
…for now Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
@cecilphillip DaprChatClient still needs toolcall capabilities, so I'm not setting as the default yet, but will eventually when it's merged in dapr/dapr. Agent & DurableAgent does take an llm client, so you can technically already use the DaprChatClient as that parameter :) |
ready for rechecks pls |
dapr_agents/agents/agent/agent.py
Outdated
content = response.get_content() | ||
if content: | ||
self.memory.add_message(AssistantMessage(content=content)) | ||
# # TODO(@Sicoyle): we should not clear the tool history here, but rather when the agent is done, or not at all. |
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.
we should always clear self.tool_history
when the assistant returns a final response (i.e., when there are no more tool calls in the message and the conversation is ready to return a user-facing answer).
This ensures:
- Tool results are only kept for the current round of tool calls.
- We do not accidentally send tool results for tool calls that are no longer relevant in the next round.
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 guess this maybe makes sense for the Agent, and we have a different implementation for DurableAgent. I guess my question here would be if folks using the Agent class would want audit-ability to debug agent behavior, or understand its decision making process, or just have their tool history handy. It might be nice to make this more configurable either with a TTL, or allowing users to manually purge, or yeah as we have here.
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.
got it! Yeah, it would be good to give control to the user. With the open telemetry feature, all of that should be captured ;) , but for those not using the observability
feature, it would be good to enable that capability.
Signed-off-by: Samantha Coyle <[email protected]>
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.
When I run the dapr run -f dapr-random.yaml
following the README, I see error messages like these
== APP - WorkflowApp == ERROR:dapr_agents.workflow.base:Error on transaction attempt: 1: No etag found for key: agents_registry
== APP - WorkflowApp == INFO:dapr_agents.workflow.base:Sleeping for 1 second before retrying transaction...
and it seems that agents are not registering their metadata
== APP - WorkflowApp == INFO:dapr_agents.workflow.task:Executing task 'select_random_speaker'
== APP - WorkflowApp == INFO:dapr_agents.workflow.task:Invoking regular Python function
== APP - WorkflowApp == INFO:dapr_agents.workflow.agentic:Agents found in 'agentstatestore' for key 'agents_registry'.
== APP - WorkflowApp == INFO:dapr_agents.workflow.agentic:No other agents found after filtering.
== APP - WorkflowApp == WARNING:dapr_agents.workflow.orchestrators.random:No agents available for selection.
== APP - WorkflowApp == ERROR:dapr_agents.workflow.task:Error in task 'select_random_speaker'
== APP - WorkflowApp == Traceback (most recent call last):
== APP - WorkflowApp == File "/Users/wardog/Documents/GitHub/TEST/dapr-agents/dapr_agents/workflow/base.py", line 165, in run_sync
== APP - WorkflowApp == loop = asyncio.get_running_loop()
== APP - WorkflowApp == RuntimeError: no running event loop
== APP - WorkflowApp ==
== APP - WorkflowApp == During handling of the above exception, another exception occurred:
== APP - WorkflowApp ==
== APP - WorkflowApp == Traceback (most recent call last):
== APP - WorkflowApp == File "/Users/wardog/Documents/GitHub/TEST/dapr-agents/dapr_agents/workflow/task.py", line 114, in __call__
== APP - WorkflowApp == raw = await self._run_python(data)
== APP - WorkflowApp == ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
== APP - WorkflowApp == File "/Users/wardog/Documents/GitHub/TEST/dapr-agents/dapr_agents/workflow/task.py", line 155, in _run_python
== APP - WorkflowApp == return self.func(**data)
== APP - WorkflowApp == ~~~~~~~~~^^^^^^^^
== APP - WorkflowApp == File "/Users/wardog/Documents/GitHub/TEST/dapr-agents/dapr_agents/workflow/decorators.py", line 88, in wrapper
== APP - WorkflowApp == return f(*args, **kwargs)
== APP - WorkflowApp == File "/Users/wardog/Documents/GitHub/TEST/dapr-agents/dapr_agents/workflow/orchestrators/random.py", line 176, in select_random_speaker
== APP - WorkflowApp == raise ValueError(
== APP - WorkflowApp == "Agents metadata is empty. Cannot select a random speaker."
== APP - WorkflowApp == )
== APP - WorkflowApp == ValueError: Agents metadata is empty. Cannot select a random speaker.
Exited App successfully
I can only see the orchestrator registering itself but not the agents

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.
This example in particular had pinned the last released version of dapr agents. So if you did the pip install on the requirements.txt file as is, then I guess I'm not surprised you saw an err. I updated the file to use the local dapr agents package required dependencies and saw no issues. I also see agents_registry showing agents registering locally. I was sure to try a fresh venv in checking this btw :)

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.
Based on our conversation, rhe following is needed:
# Prepare agent metadata
self._agent_metadata = {
"name": self.name,
"role": self.role,
"goal": self.goal,
"instructions": self.instructions,
"topic_name": self.agent_topic_name,
"pubsub_name": self.message_bus_name,
"orchestrator": False,
}
# Register agent metadata
self.register_agentic_system()
right after:
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.
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
readyyyyy 🙌
|
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
…ead of old release tag Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
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 the agent registration
update, all workflows in this quickstart worked :)
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 tested all quickstarts but 05-multi-agent-workflow-k8s
. All seems to work as expected! There was some issues with mcp stdio, but it is not related to the unification of agents. I will open an issue for mcp stdio. Finally, I noticed several imports in a few classes that are not used.
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
readyyyyyy 🚀 |
Signed-off-by: Samantha Coyle <[email protected]>
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.
Thank you very much @sicoyle ! 🔥🎉
Currently, there are a few different agent types:
This PR refactors things so there are two agents from an end user POV:
OpenAPIReAct,ToolCalling, or ReActagent. This is the simpler agent to use for more concrete tasks.I've moved both agents to live side by side under dapr_agents/agents/agent and dapr_agents/agents/durableagent. This way they can leverage the same AgentBase class. There was a
AgentWorkflowBase
class that I totally removed in this refactor to give DurableAgent and Agent the same base class as it had basically the same fields plus a few things as the AgentBase class. So this is a lot cleaner now :)I removed my initial thoughts on adding in config file support to keep this PR more focused on just cleaning up agent types for now. In a follow up PR I will tackle:
I've added a common config class to bring in the basic config support; however, please note that the config will be one of the next things I tackle, so I am open to feedback here, but this is just a starting point aimed at making the DX better for agent instantiation by moving the params to a config file for users.I've also fixed random things I came across such as supporting more graceful shutdowns if someone kills the terminal as the agent is working on different tasks, fixed a few things on the template formatting, etc.
Context: #126
Still todo for this PR:
double check my config setup after all the refactorstox -e type
Additional context
Mental mapping / mostly UML-ish 😄 diagram for the agent factory class

These are diagrams I created as I went through things and did my refactoring, so sharing in case useful for others
Mental mapping / mostly UML-ish 😄 diagram for the now AssistantAgentClass

Note: I did not add in my config class to my diagrams