Skip to content

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

davemssavage
Copy link

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Quick patch to test idea will need tests to prove it works

@hesreallyhim
Copy link
Contributor

Looks nice, it sounds from your language like maybe this is a draft?

@davemssavage davemssavage marked this pull request as draft May 24, 2025 17:39
@davemssavage
Copy link
Author

Yep, couple of tweeks to make, forgot to tag it as draft

@@ -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?
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Author

@davemssavage davemssavage May 24, 2025

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

Copy link
Contributor

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.

Copy link
Author

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
Copy link
Contributor

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)]
Copy link
Author

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.

Copy link
Contributor

@hesreallyhim hesreallyhim May 26, 2025

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.

Copy link
Author

@davemssavage davemssavage May 26, 2025

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.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants