-
Notifications
You must be signed in to change notification settings - Fork 2k
Simplify filtering MCP ToolCallbackProviders for ToolCallingAutoConfiguration #4764
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
|
@quaff your current architecture introduces a dependency cycle, specifically in the case of sampling: McpClient -> methods annotated with @McpSampling -> ChatClient (to do the sampling) -> ToolCallingManager -> ToolCallingResolver -> McpToolCallbackProvider -> McpClient But since the "methods annotated with @McpSampling" do not have a declared dependency in the container (but work with some temporal coupling), sampling will fail. Please run Note that this area is currently in flux, and this complicated code might go away (or at least be moved out) |
I force pushed commit to use
Sorry I can't access Anthropic in my area, could you run it to verify? @Kehrlann |
Yes, that's much better 👏
Ah, good point. Feel free to update the test locally to use whatever model suits you best, as long as it supports tool calling. |
…guration See spring-projectsGH-4751 Signed-off-by: Yanming Zhou <[email protected]>
| @ConditionalOnMissingBean | ||
| ToolCallbackResolver toolCallbackResolver(GenericApplicationContext applicationContext, | ||
| List<ToolCallback> toolCallbacks, List<ToolCallbackProvider> tcbProviders) { | ||
| List<ToolCallback> toolCallbacks, ObjectProvider<ToolCallbackProvider> tcbProviders) { |
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.
For backwards compatibility, Would this work if there is a List instead of just multiple ToolCallbackProvider instances?
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.
List<ToolCallbackProvider> bean will not be injected via ObjectProvider<ToolCallbackProvider>, but I don't think anyone would do that, and ObjectProvider<ToolCallbackProvider> is the recommended way by Spring Boot, you can refer to other AutoConfigurations.
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 agree on using ObjectProvider being the better approach. My question is mainly about backwards compatibility. Since we already had "List tcbProviders", we don't want to break the compatibility for those who already uses "List tcbProviders". Perhaps, we should use ObjectProvider<List>. cc @tzolov
|
I believe #4851 is a replacement for this PR |
I think febf86c introduced unnecessary complexity, this commit make the code more simple and readable.
Did I miss something? @Kehrlann