-
Notifications
You must be signed in to change notification settings - Fork 1k
[ag-ui-adk] Fixes #903 Using Copilokit Frontend tools with Orchestrator-style ADK agents #904
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
base: main
Are you sure you want to change the base?
Conversation
…an implementation for abstract method 'get_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.
@jplikesbikes Thank you for the submission... there is a lot going on here beyond handling issue #903 - in particular the addition of tool filtering.
For starters, I don't see any reason why this needs to be a breaking change. The tool filtering could be contingent upon the presence of AGUIToolset - without AGUIToolset the prior behavior (i.e., all frontend tools are accepted, barring conflicts with backend tools) should continue.
Second, it seems that the prior behavior that prevented a frontend tool from hijacking a backend tool with the same name has been eliminated, along with the specific exclusion of frontend tools with the name of "transfer_to_agent". This is concerning.
This all highlights the need for extensive test cases for both happy path and error/edge cases. A quick glance through the open and closed issues related to the ADK Middleware will reveal that tool calling is hands down the most challenging aspect of this integration, so any related change needs an extra level of scrutiny. So please add test cases.
But again, thank you for this submission. I'm confident that this, along with #908 will be useful contributions!
|
@contextablemark first off thank you for reviewing this and providing feedback! I believe strongly in consistency in api design, and this pr tries to align ag-ui tool usage with how other tools are imported and used in google-adk. To do this a few breaking changes are required, but I believe the end result is worth it. This pr does the following to align ag-ui tools with how other toolsets are used in google-adk:
These changes allow for a much more consistent developer experience when using ag-ui tools in google-adk agents. Additionally, it allows for greater flexibility in how ag-ui tools are used. Adk allows tools to be overridden by later tools in the LLMAgent.tools array. A very real world example of this could be adding a feature to an agent to notify the user when a task is complete. When the user is accessing the agent via agui the desired behavior is to show a popup in the browser. However, when the user is accessing the agent via a different interface the desired behavior is to send an email instead. With the current implementation this is not possible Here's how that would be implemented with this pr task_agent = LlmAgent(
name='TaskAgent',
model='gemini-2.5-flash',
instruction="""
Notify the user when you complete a task by calling the `notify_user` tool.
""",
tools=[
EmailToolset(), # The default notify_user tool is added here
AGUIToolset() # The AguiToolset overrides the notify_user tool, possibly adding other agui tools as well
],
)I believe even the simple filtering of I hope this explanation helps clarify why I made these changes. I am open to further discussion and feedback on this pr. My goal is to make ag-ui tools as easy to use and consistent with google-adk as possible. With regards to testing this change is there anything in particular you would like to see? I purposely did not change anything related to how agui tools functionally work, only how they are presented to adk agents. |
|
@jplikesbikes You do make some good points (and you sold me on "overriding is a feature, not a bug" and ruling out the exclusion - at the time that was written there weren't as many internal tools, so now blocking "transfer_to_agent" just seems oddly arbitrary considering the existence of web_search, vertex_ai_search, etc.) but I'm still not sold on introducing breaking changes for existing consumers, many of them (I assume, since I'm not privy to any of that information) in production for whom breaking changes would likely be unwelcome. I've reached out to the team inside CopilotKit to see if they can shed some light on the topic. In the meantime, the kind of testing I would expect to see is mainly testing that verifies all of the behaviors we've talked about and that are expressed in comments in the code, for example :
and then some edge cases :
These test cases all seem valid regardless of the backwards compatibility decision. I'd really like to get an impact assessment on that from people with that knowledge. I've spent the better part of my career developing and maintaining SDKs for paying enterprise customers and they tend not to like it when things break without warning (and when it happens, tend to ask for continued maintenance on old branches until they're ready to "take the plunge") and try to hold my products to a similar standard here. |
|
@jplikesbikes Thanks for the update. While I haven't received any direct answer from CopilotKit to my question regarding whether breaking changes should be optional, there is some philosophical direction provided by the existing FilterToolCallsMiddleware that already exists on the frontend as of this PR (along with the enhancements to that functionality pending in this PR), namely that it's up to the implementer to decide whether or not they want this functionality (and even whether it should be an allowList or a a denyList). Anyway, I've reached out again to CopilotKit - hopefully I can get some clarification in the coming week. |
|
@contextablemark I just wanted to note that I believe this pr is tackling a different problem from the FilterToolCallsMiddleware which seems to focus on the agui client filtering tool calls from an agui server. That usecase should follow any patterns established by agui client. On the other hand this pr is about adk agents consuming agui tools and following the established patterns of the adk framework. In particular that means removing auto agui tool injection and instead using explicit toolsets to add agui tools to adk agents anywhere in the agent hierarchy. As for breaking changes, the current release is version 0.4.1 so following semantic versioning a public api change can be expected in any release until 1.0.0 and it is up to the developer to verify compatitability with existing code. I understand wanting to minimize breaking changes to keep current developers happy and build trust, but anyone using this package should already be prepared for breaking changes. Additionally unlike some breaking changes the previous behavior can be restored in just a few lines of code. This way existing developers can easily restore previous behavior if they need it, while new developers get a better experience out of the box. In terms of copilot kit this doesn't affect the protocol between agui client and server so there should be no impact except to update their documentation. |
|
Hi @jplikesbikes - Just wanted to let you know that I haven't forgotten about this submission; I'd just still like to get the opinion of CopilotKit engineers. And your migration instructions should be sufficient to mitigate the breaking change (which is much more limited than I was thinking at first, since it only "breaks" the root agent because the subagents never worked with tools to begin with). In the meantime, I was looking for test cases for these two scenarios and couldn't find them. Could you add them?
Thanks, |
…r ones with the same name
|
@contextablemark Thanks for the update. I didn't add those tests because it is not a behavior of I did add a test to prove that this is current behavior of google-adk since I didn't see it in their test suite, but I'm torn on whether it belongs in the code base. I'll leave that decision to you. |
This is a somewhat simple possible solution to issue #903
It draws from the discussion here and implementation here
This is a breaking change as now frontend tools are not injected into the root agent by default and instead any agent that wants access to frontend tools needs to be provided with the
AGUIToolsetin it's tools parameter.Here's an example:
The follow assumes two agui tools
sayHelloandsayGoodbye. Isolation is achieved by prodiving theHelloAgentwithaccess to only the
sayHellotool and theGoodbyeAgentaccess to only thesayGoodbyetool. The root agentMyAgenthas no tool access and can only transfer to the
HelloAgentandGoodbyeAgent.No configuration except for creating the FrontendToolset and providing it to the appropriate agent is required.