-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: Pass through and expose additional parameters in ClientSessionGroup.call_tool and .connect_to_server
#1576
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
…ClientSession.callback`
…nnect_to_server` method
|
we need this :) |
|
I need this function to refresh tools. |
kylestratis
left a comment
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.
Some feedback on the structure of this compared to the rest of the codebase and a proposed simplification.
src/mcp/client/session_group.py
Outdated
| @overload | ||
| @deprecated("The 'args' parameter is deprecated. Use 'arguments' instead.") |
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.
| @overload | |
| @deprecated("The 'args' parameter is deprecated. Use 'arguments' instead.") |
I'd avoid adding conventions that aren't used elsewhere in the SDK.
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.
Thank you for your feedback!
Personally, I felt it might be more consistent to rename args to arguments, since the original method ClientSession.call_tool uses arguments as its parameter name, so I added the deprecation warning. But per your suggestion, I reverted the changes in commit 4b9546d (#1576).
src/mcp/client/session_group.py
Outdated
| return self._tools | ||
|
|
||
| async def call_tool(self, name: str, args: dict[str, Any]) -> types.CallToolResult: | ||
| @overload |
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.
| @overload |
src/mcp/client/session_group.py
Outdated
| session_params: ClientSessionParameters | None = None, | ||
| ) -> mcp.ClientSession: | ||
| """Connects to a single MCP server.""" | ||
| server_info, session = await self._establish_session(server_params) | ||
| server_info, session = await self._establish_session(server_params, session_params) |
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.
session_params should follow the pattern of server_params and be required or at least default to an empty session_params object (I prefer the former for consistency's sake). This will make _establish_session() able to take session_params as a required argument, removing the need to check if session_params() exists and having different calls to enter_async_context() in lines 322-328.
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 default value is changed into ClientSessionParameters() as required in commit 4ab9757 (#1576).
…ject instead of `None`
… parameter" This reverts commit 47bc4ca.
Exposed additional optional parameters in the
call_toolandconnect_to_servermethods so that they can fully pass through supported arguments to the underlying session-level methods.Replace the parameter
argsincall_toolwithargumentsso that it's aligned with the originalClientSession.call_toolmethod.Use
@overloadand@deprecatedto markargsas deprecated while still supporting its usage.Motivation and Context
Previously,
call_toolandconnect_to_serverignored several optional parameters supported by the lower-levelClientSessionand session methods.As a result, certain advanced use cases (e.g., custom timeouts, callbacks, or client metadata) could only be supported by directly accessing lower-level APIs, which was cumbersome and inconsistent.
This change:
call_toolfor timeouts, progress tracking, and metadata.session_paramstoconnect_to_serverto configureClientSessioncreation directly.ClientSessionParametersdataclass for clear typing.How Has This Been Tested?
Pyright static type check and pytest.
Breaking Changes
argsparameter incall_toolis deprecated in favor ofarguments.Types of changes
Checklist
Additional context
ClientSessionParametersusesdataclasses.dataclassinstead ofpydantic.BaseModelbecauseBaseModeldoes not supportProtocol-typed fields. Attempts withSkipValidationandarbitrary_types_allowed=Truedid not resolve the issue.