Conversation
This reverts commit 2a0a2ff.
sreenithi
left a comment
sreenithi
left a comment
There was a problem hiding this comment.
Please double check if ClientSession / ServerSession is not being changed to ClientTransportSession / ServerTransportSession in files specific to the native transports like stdio, streamable-http, sse.
I've already commented on files which looked specific to these transports, but please double check once from your side too
| """Send a request""" | ||
| raise NotImplementedError | ||
|
|
||
| async def send_notification( |
src/mcp/shared/session.py
Outdated
| This class is an async context manager that automatically starts processing | ||
| messages when entered. | ||
|
|
||
| This is need for JSON-RPC based transports. |
There was a problem hiding this comment.
this should be at the beginning, and please write it in a better way combining this and the first line of the docstring, instead of 2 separate sentences.
tests/shared/test_sse.py
Outdated
|
|
||
| @pytest.fixture | ||
| async def initialized_sse_client_session(server: None, server_url: str) -> AsyncGenerator[ClientSession, None]: | ||
| async def initialized_sse_client_session(server: None, server_url: str) -> AsyncGenerator[ClientTransportSession, None]: |
There was a problem hiding this comment.
I think this should be unchanged because it is yielding a ClientSession object in this function.
Similarly, I think even changes in tests/shared/test_streamable_http.py and tests/shared/test_ws.py shouldn't be done, because I'm expecting these tests are meant for only those specific transports like SSE, Streamable HTTP and WebSocket transports which are still under the ClientSession object only.
When there are new tests written for other transports like gRPC or anything else, they can use the upper level ClientTransportSession type, but I don't think it makes sense to change it for these 3 native transports.
There was a problem hiding this comment.
As mentioned in previous comment I don't think this file should be changed.
There was a problem hiding this comment.
As mentioned in previous comment I don't think this file should be changed.
There was a problem hiding this comment.
This file is specific to the studio transport, and studio transport uses ClientSession only, so we shouldn't be updating this.
src/mcp/client/session.py
Outdated
There was a problem hiding this comment.
What about the type here and in the classes below this one. Don't we need to change them?
|
|
||
| SessionT_co = TypeVar( | ||
| "SessionT_co", | ||
| bound=Union[BaseSession[Any, Any, Any, Any, Any], "ClientTransportSession", "ServerTransportSession"], |
There was a problem hiding this comment.
I think binding it to the new Session abstract class we created will be better
2 participants
No description provided.