-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
@@ -179,7 +179,9 @@ async def send_progress_notification( | |||
progress: float, | |||
total: float | None = None, | |||
message: str | None = None, | |||
# TODO decide whether clients can send resource progress too? | |||
# TODO check whether MCP spec allows clients to create resources |
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.
On my interpretation of the spec, resources are owned and exposed by the "server". How the resource is created (say it's a file, media, API response) is not relevant. It may be that the same application can act as a client and a server (e.g. Claude Code can be a server but it is also a client, maybe the Claude Code application produces log files that the same application then exposes as a resource), but conceptually I don't think this fits the protocol, as currently framed. Nevertheless, it clearly states that both parties can request progress notifications about long-running processes (https://modelcontextprotocol.io/specification/2025-03-26/basic/utilities/progress#progress-flow), but IMO conceptually it is the server (or "the application when it's acting as a server") that creates resources. This is splitting hairs, but if Claude Code the server exposes a resource that Claude Code the application itself is producing, it's still inaccurate to say CC the client is creating the resource.
In any case, I'm curious what is the importance of this TODO? Is it to determine whether a client can send a resource/updated notification? If so, I would say no, IMO. But if the answer is "yes", like how would that affect the code/this PR?
progress: float, | ||
total: float | None = None, | ||
message: str | None = None, | ||
resource_uri: Annotated[AnyUrl, UrlConstraints(host_required=False)] |
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.
Local testing suggests it might be better to allow several resources to be reported as part of a single progress notification.
e.g.
context.report_progress(
progress=3,
total=10,
message='Reticulating splines',
resource_uris=['resource://spline1', 'resource://spline2', 'resource://spline3']
)
I might refactor along these lines if this patch has a chance of making it into the protocol/sdk and it makes sense to others.
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.
sorry i need to review what you're trying to do more carefully, but ProgressNotificationParams
, like most of the things in the Request/Response models, have model_config = ConfigDict(extra="allow")
, which afaik means you can just add more data to the progress notification, without changing the protocol.
EDIT: Well, strictly-er speaking, without changing the SDK. It's a little unclear to me if the protocol itself allows this.
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 did wonder about this also, the motivation for this patch is as an alternative to modelcontextprotocol/modelcontextprotocol#549 which appears to be trying to allow tasks to return references to resources that will eventually be created on the server. The thread of conversation there points out that by allowing resources to be in a pending state complicates the protocol as clients then need to always check if a resource is ready. This is an alternative approach that rather than making resources pending they are always 'ready' but the server can report intermediary temporary resources as state along the way.
Take an example of a tool that allows training of a model. This patch allows the tool to report several different types of info to the client via one interface whilst the training is happening, e.g. metrics such as training loss as a json report, training artifacts and model checkpoints. The client can then decide to display and/or take action based on this progress resources as it sees fit.
I guess I could just add arbitary data to the low level protocol request params and achieve the same result, however to make this easy for tool implementers this needs to be exposed via the FastMCP interface, hence adding the resource_uri (s) to the context interface. Also if going with the low level protocol approach the client would also need to implement quite a lot of low level code and there would likely end up quite a lot of different approaches among tool providers.
However, as an argument against all this, I'm also debating whether long running tasks such as training are a good fit for this approach, as this involves a very long connection to the server for a single task. An alternative to all of this might be to have a tool that uses the structuredContent approach to start a server task and then the client can then poll via another tool to check on the status e.g.
@mcp.tool("Start a long running task")
async def start_task() -> Ticket:
...
return Ticket(id=1234)
@mcp.tool("Check if a task has completed")
async def check_task(ticket: Ticket) -> Status:
...
return Status(pending=True)
It's not obvious to me whether a LLM Agent would be able to reason about either behaviour so hence this branch https://github.com/davemssavage/python-sdk/tree/integration_test_1 puts together a few things I've been tinkering with to try out various approaches.
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.
Futher thoughts, the downside of the manual start/check approach is it also requires tools to create a cancel method which is then duplicating behaviour of the MCP protocol itself at a higher level.
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.
ok I didn't catch this was aimed at improving FastMCP mostly. During my initial foray into this, I found that this was something kinda neglected in FastMCP which is why I went to lowlevel. Resource-update subscription notification I think is not really built into FastMCP API anyway, they just have something for ProgressNotification I believe (so maybe this is the missing piece really). I just got up so I have to read this a few more times, but a few thoughts (a) a long-running task could be treated as a resource (like "job://") and then the client could make a read request for the job, or subscribe to change notifications for the job (that's like your tool approach but you don't need any new tools like "check_task", it's just a read_resource where the resource is a "task://"... (b) something else lol I gotta chew on it.
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.
Ok I think I get where you're going with the task itself as a job resource, however I think in that scenario it would be quite complex to support the type of progress updates seen in training, e.g. in the case of training a model there are quite a few different elements that can be notifiied as progress and I'm slightly struggling to see now a single resource could satisfy all of these.
I guess I could pass back several resources one for task metrics, one for task validation artefacts and one for checkpoints. However that only allows for the latest version of this information rather than being able to plot metrics over time as is possible if the resource is associated with a progress state. This also leaves open the question of how to cancel the task itself which would require a new tool to cancel a previous task.
I took a pass at simplifying cancelling a tool call initiated via the client session with this pull request #628. I'll see if I can get this training example working and and aim to add it as an example so there's something concrete to point to.
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 think in that scenario it would be quite complex to support the type of progress updates seen in training, e.g. in the case of training a model there are quite a few different elements that can be notifiied as progress and I'm slightly struggling to see now a single resource could satisfy all of these.
I think the idea is you're not asking for "ProgressNotifications" in the sense of like 50% complete, etc., the task/job is a resource, which can be whatever kind of data structure you like, with all kinds of different statuses/properties, etc., and so the server can update the resource with all the relevant data as things go along, and then client reads it and the server can give it whatever data structure supports your needs.
However that only allows for the latest version of this information rather than being able to plot metrics over time as is possible if the resource is associated with a progress state.
So then you could solve this by subscribing to get "resources/updated" notifications for the "job"/"task" resource, and then when anything is updated (new checkpoint, whatever), the server tells the client "there's a change" and the client reads the resource again, where the resource is the job itself. No polling involved either.
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