-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Feature/resource progress #800
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?
Feature/resource progress #800
Conversation
Looks nice, it sounds from your language like maybe this is a draft? |
Yep, couple of tweeks to make, forgot to tag it as draft |
…s naming params consistently
src/mcp/client/session.py
Outdated
@@ -173,6 +179,7 @@ async def send_progress_notification( | |||
progress: float, | |||
total: float | None = None, | |||
message: str | None = None, | |||
# TODO decide whether clients can send resource progress too? |
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.
hey i'm working on resources stuff right now so i hope you don't mind if I'm commenting as you work, what is the notion of a client resource in general? i thought they were owned by the server
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 protocol indicates resources are offered by servers to clients
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.
to me a client progress notification would not be about a resource, but some other process.
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.
Yep I think that makes, I think that's what I guessed too but hadn't got around to validating my hunch, I might tidy this TODO up if there really is no use case for a client resource, it was more a reminder to think about it before finishing off the patch
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.
well client progress notifications are within the spec, it's just they're not about resources, they're about long-running process:
The Model Context Protocol (MCP) supports optional progress tracking for long-running operations through notification messages. Either side can send progress notifications to provide updates about operation status.
Not sure what the use case is or if it's relevant to your goal, but yeah.
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.
I will update the TODO as I don't currently have a use case for client side resource progress but just wanted to set this as a reminder to look into the spec to see if it was supported
Experimenting with the idea of passing back resources with progress notification
Motivation and Context
Tinkering to see if this idea works modelcontextprotocol/modelcontextprotocol#549 (comment)
How Has This Been Tested?
Unit testing only so far
Breaking Changes
None expected
Types of changes
Checklist
Additional context
Quick patch to test idea will need tests to prove it works