-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Defer call of getToolCallbacks() to prevent initializing ToolCallbackProvider eagerly #4459
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
9e83282
to
41dc27c
Compare
36fc176
to
74f8bff
Compare
Although this is helpful, it only solves a part of the problem: as soon as the connection gets lost (e.g. reboot of the MCP Server) the connection does not get reestablished again and one needs to reboot the client too. |
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 mention MCP specifically, but we're now wrapping ALL ToolCallbackProvider
s, including MethodToolCallbackProvider
- but those do not have an initialization issue. Is that desired?
This is a real issue for bootstrapping MCP, and this addresses the bootstrapping part. There's another issue linked to this: remote MCP tools can change on the server, over time, and the client gets notified. In this case, the cached MCP tools are not updated. I wonder if we should have an MCP-specific abstraction rather than a ToolCallbackProvider
abstraction.
If we want to delay this MCP-focused abstraction, then this PR is neat 👌
.../java/org/springframework/ai/model/tool/autoconfigure/ToolCallingAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
ToolCallbackProvider toolCallbackProvider = MethodToolCallbackProvider.builder() | ||
.toolObjects(new WeatherService()) | ||
.build(); | ||
return Mockito.spy(toolCallbackProvider); |
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.
Prefer a mock over a spy.
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.
Existing tests rely on spy.
...model/src/main/java/org/springframework/ai/tool/resolution/ProviderToolCallbackResolver.java
Outdated
Show resolved
Hide resolved
I was hoping this makes it into 1.1.0-M3 :( |
…Provider eagerly Before this commit, `McpSyncClient` is initialized even if `spring.ai.mcp.client.initialized` is set to `false`. See spring-projectsGH-3232 Signed-off-by: Yanming Zhou <[email protected]>
hmm, I tried this locally but did not succeed - the mcp tools still get initialised at startup and the server start fails if the mcp server is not available. This is how I configure my
|
@imod doing |
ok, so I always have to set them directly on the ChatClient when executing the prompt? e.g.
|
Correct @imod You can put your |
Before this commit,
McpSyncClient
is initialized even ifspring.ai.mcp.client.initialized
is set tofalse
.See GH-3232