feat: support progress_callback propagation in ClientSession (issue #1248)#1249
feat: support progress_callback propagation in ClientSession (issue #1248)#1249nkaushal99 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
|
link to issue #1248 |
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
I don't think we want to change internal implementation details of the SDK here in response to what other downstream tools like LangChain might or might not do unless this is a very common use case.
Also if I follow here, you're attempting to set a progress callback on the session as a whole - however, progress callbacks are internally associated with request_id in BaseSession as different requests might require different progress handling within the same session:
python-sdk/src/mcp/shared/session.py
Lines 178 to 179 in 9301924
fastmcp seems to allow setting a default progress_handler for a Client as a whole, but that's not necessarily supported in this lower level API. Internally fastmcp also explicitly passes the progress_handler as we do here: https://github.com/jlowin/fastmcp/blob/7770e3a6bc838a1b3c04c6e543b5b804f8868c93/src/fastmcp/client/client.py#L822
The correct solution here seems to be to leverage the arguments arg allowed in lang_chain, not to modify the SDK here.
| read_timeout_seconds=read_timeout_seconds, | ||
| ) | ||
| self._client_info = client_info or DEFAULT_CLIENT_INFO | ||
| self._progress_callback = progress_callback |
There was a problem hiding this comment.
should this have a _default_progress_callback?
| types.CallToolResult, | ||
| request_read_timeout_seconds=read_timeout_seconds, | ||
| progress_callback=progress_callback, | ||
| progress_callback=progress_callback or self._progress_callback, |
There was a problem hiding this comment.
This seems like it's just inviting hard to debug behavior where we make a call_tool call without any progress_callback yet there still somehow ends up being one via some separate state object.
|
Hi @nkaushal99 thank you for this contribution! I'm closing this PR based on the reasoning provided above for now - please feel free to ping here again though if you disagree. |
felixweinberger
added
enhancement
2 participants
closes #1248
Motivation and Context
This change introduces support for injecting a default
progress_handlerintoClientSession. It solves the issue where downstream libraries (e.g., LangChain's MCP adapter) access the rawclient.sessionand make tool calls that bypass the top-levelfastmcp.Client’sprogress_handler, leading to missing progress reports. With this enhancement, consistent progress reporting is ensured whether tools are called through the high-level client or the lower-level session.How Has This Been Tested?
This feature has been tested within an async environment using:
ClientSession.call_tool(...)to ensure the default handler is triggered.progress_callbackto ensure override behavior works correctly.langchain_mcp_adaptersto confirm downstream compatibility and improved reporting.Breaking Changes
No breaking changes. This enhancement is backward-compatible. Existing users who do not pass a
progress_handlerwill see no change in behavior. Users who want consistent progress reporting can optionally provide a handler toClientSession.Types of changes
Checklist
Additional context
This feature enables clean integration with ecosystem libraries like LangChain without requiring redundant progress callback plumbing at every call site.