-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: add partial results as part of progress notifications #669
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
@@ -322,6 +327,19 @@ class PingRequest(Request[RequestParams | None, Literal["ping"]]): | |||
method: Literal["ping"] | |||
params: RequestParams | None = None | |||
|
|||
class PartialResult(BaseModel): | |||
chunk: dict[str, Any] |
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.
disregard below -- these points need to go on the spec PR. i will add them there.
question:
- there is nothing technically wrong with modeling this as a
dict[str, Any]
, but this is not howCallToolResult
models its content. Just curious if there is a specific reason not to keep same structure asCallToolResult
? Intuitively i would expect them to be very similar. - Also a nitpick on naming but should we call this chunk? why not "content" or something (similar to
CallToolResult
)
@@ -259,14 +259,15 @@ async def call_tool( | |||
name: str, | |||
arguments: dict[str, Any] | None = None, | |||
read_timeout_seconds: timedelta | None = None, | |||
**meta |
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.
Can we model this as an argument with Meta type rather than treating it as **kwargs? If the idea is not to put all extra keyword arguments, that don't match other call_tool arguments, in meta then we should use an argument with type Meta instead.
@@ -47,7 +47,12 @@ class Meta(BaseModel): | |||
parameter is an opaque token that will be attached to any subsequent | |||
notifications. The receiver is not obligated to provide these notifications. | |||
""" | |||
|
|||
partialResults: bool | None = None |
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.
Might be a comment for the spec PR.
Nit: I wonder if streamPartialResults or notifyPartialResults would be more apt than adding a boolean value under partialResults?
This PR implements the specifications defined in modelcontextprotocol/modelcontextprotocol#383, introducing partial result streaming capabilities within progress notifications from Server to Client.
Motivation and Context
This enhancement addresses two key improvements to the protocol:
Changes
call_tool
api to enable feature access from specHow Has This Been Tested?
Breaking Changes
No breaking changes as the new attributes and parameters are all optional.
Types of changes
Checklist
Additional context