Skip to content

feat: MethodToolCallback add reactive return value support #2884

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

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

Conversation

Cui-xf
Copy link

@Cui-xf Cui-xf commented Apr 25, 2025

Signed-off-by: Cui-xf [email protected]

Thank you for taking time to contribute this pull request!
You might have already read the [contributor guide][1], but as a reminder, please make sure to:

  • Sign the contributor license agreement
  • Rebase your changes on the latest main branch and squash your commits
  • Add/Update unit tests as needed
  • Run a build and make sure all tests pass prior to submission

@markpollack
Copy link
Member

Currently tool calling is not reactive in it's implementation, so the signatures proposed in this PR this would present a false expectation.

To address this we would need to dig deeper into the tool calling implementations and can look into that post 1.0 GA.

@markpollack
Copy link
Member

@tzolov thoughts?

@ThomasVitale
Copy link
Contributor

I'm afraid that making this change here would not lead to the expected benefits. I would suggest postponing this since it would require a larger discussion around reactive support and reactive vs. asynchronous. The inner tool calling logic needs some redesign to support running tools on virtual threads, and running tools as reactive streams.

@Cui-xf
Copy link
Author

Cui-xf commented May 7, 2025

I'm afraid that making this change here would not lead to the expected benefits. I would suggest postponing this since it would require a larger discussion around reactive support and reactive vs. asynchronous. The inner tool calling logic needs some redesign to support running tools on virtual threads, and running tools as reactive streams.

Thanks for your reply, and sorry for getting back late!

I've been working on building an mcp-server with spring-ai recently, and I ran into an issue where tool calling doesn't support reactive return values. That's why I submitted this PR.

I agree that the current changes aren't very elegant. The main reason is that the ToolCallback interface is used in multiple scenarios:

  1. It's used for active calls by AI models (org.springframework.ai.model.tool.DefaultToolCallingManager).
  2. It's also used as a tool provider for mcp-server, handling passive calls.

This makes the definition of ToolCallback a bit unclear, and its responsibilities aren't well defined. Maybe we need to rethink the tool calling logic (possibly splitting out the mcp-server scenario).

I think providing reactive support in the mcp-server scenario is actually very useful. It would let us integrate perfectly with other reactive tools like WebClient, Lettuce, etc. Ideally, it should work like a WebFlux Controller, supporting both regular and reactive return types.

By the way, my English isn't very good, so this reply was machine translated. Hope it's clear enough!

Thanks again and looking forward to more feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants