Feature: MCP client Resources support #3024
Conversation
…r to return these in preference to `mcp` upstream types.
|
I was just looking at this as I need resources support, and noticed this doesn't provide any way to determine whether the server even supports resources. The only way to tell with this code would be to attempt to make a resource call (e.g. I think this should be better exposed as I would like my application to switch to alternate behavior if the MCP server does not support resources. And would like to do so without having to make a call and see if it fails, and then differentiate whether that failure was because of lack of support, or some other reason. The MCP spec says that the |
|
Another thing I noticed going through the code, is that for I'm not sure if addressing this is even in scope of this PR, as |
That's a great point (and not something I was looking at in my changes). Capabilities presumably hasn't been an issue up until this point because MCP was only used for tools in Pydantic AI and it was assumed you wouldn't use an MCP server unless it had tool capabilities. I'm not quite sure the right way to support this. The upstream There is an existing pattern, which is how @property
def server_info(self) -> mcp_types.Implementation:
"""Access the information send by the MCP server during initialization."""
if getattr(self, '_server_info', None) is None:
raise AttributeError(
f'The `{self.__class__.__name__}.server_info` is only instantiated after initialization.'
)
return self._server_infoWe could follow this pattern and just return Maybe something like: ServerCapabilities = Literal[
"experimental",
"logging",
"prompts",
"resources",
"tools",
"completions"
]
# MCPServer
@property
def capabilities() -> set[ServerCapabilities]:
...Meh, I guess that's kind ugly: I don't want this PR to drag on any longer than it already has, but this seems like a reasonable/incremental change overall? |
|
What happens when you try to retrieve resources from a server that doesn't have that capability? |
Right now (current state) it would raise an Arguably we could choose to intercept this and fail-early if the server doesn't have the capability, but I'm not sure it really adds any value? There's also not much of a pattern in the codebase for this that I can tell (given how lightweight I think we can just leave it up to the client application/user to check which server capabilities exist via something like a |
|
Since "resources are not supported" or "the requested resource doesn't exist" are such common cases users will typically need to handle, I think they should be our own exceptions or the methods could just return |
I've done some additional testing. The specifics of errors are pretty server-implementation specific. For example, the official python-sdk's FastMCP server doesn't even have the ability to disable
I think returning For other errors in things like I'll take a pass at this direction to review. |
|
I'm fine with that. |
|
Just a head up, I haven't abandoned this, just gotten caught up in my day job. Will finish this up ASAP. Apologies for the delay. |
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| ) | ||
| result = await self.read_resource(str(part.uri)) | ||
| # If resource not found, return an empty string as it's impossible to fetch anyway | ||
| return result if result is not None else '' |
There was a problem hiding this comment.
This is a bit weird. The only two options here are to return empty string or raise an exception. I chose the former as the lesser of two evils, given the real fix would be addressed as part of #3099.
This problem has always existed, it was just less obvious without the underlying type safety.
There was a problem hiding this comment.
I think it should probably be an error, because this is the MCP server itself returning a (link to a) resource that doesn't actually exist. That's a server bug that I wouldn't want to hide
There was a problem hiding this comment.
Ok, I've changed this here.
I know this still looks a bit weird, but the behaviour is now consistent with how the code currently works in main (ie: any exceptions during read_resource() just get raised). The only difference is now it raises a native McpError type rather than the python mcp-sdk one.
I suspect we can't really fix this "properly" until we tackle #3099 as the entire concept of fetching a resource inline as part of a tool call result is perhaps a bit fraught in general?
What we have here is the "smallest" change that's keeps things (mostly) consistent.
fennb
changed the title
|
@DouweM this is ready for re-review (apologies for the delay)! |
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| ) | ||
| result = await self.read_resource(str(part.uri)) | ||
| # If resource not found, return an empty string as it's impossible to fetch anyway | ||
| return result if result is not None else '' |
There was a problem hiding this comment.
I think it should probably be an error, because this is the MCP server itself returning a (link to a) resource that doesn't actually exist. That's a server bug that I wouldn't want to hide
tests/test_mcp.py
Outdated
| async def test_read_resource_not_found(mcp_server: MCPServerStdio) -> None: | ||
| """Test that read_resource raises MCPError for non-existent resources with non-standard error codes.""" | ||
| async with mcp_server: | ||
| # FastMCP uses error code 0 instead of -32002, so it should raise |
There was a problem hiding this comment.
That's a bit annoying and something they should fix... How would you feel about testing for "Unknown resource" in the error message so we can ensure consistent behavior with different common MCP servers?
There was a problem hiding this comment.
I looked into this further. It's even more annoying than it looked and I ended up lodging (at least an initial) issue on the upstream SDK: modelcontextprotocol/python-sdk#1579
Unfortunately (as per that issue), neither the python nor the typescript SDKs follow the documented spec, so we can either:
- Try to implement special case handling for each broken upstream SDK server implementation;
- Give up and declare it broken upstream and just raise errors for users to deal with for their use case;
I'm the first to admit that neither option is great here. Presumably the server SDKs will be fixed at some point.
My instinct is that (for the moment) we should just raise exceptions if the server raises an unknown exception and it's up to the user to handle. Happy to follow your guidance on preferred path though!
I have, however, at least neatened up the test naming and logic here so it's more clear what's going on.
Co-authored-by: Douwe Maan <[email protected]>
|
This is ready for re-review. I noticed we're consistently getting test failures on a specific subset of test suites, but they really seem unrelated to these changes as far as I can see? Example here. |
| if TYPE_CHECKING: | ||
| from .mcp import Resource, ResourceTemplate, ServerCapabilities |
There was a problem hiding this comment.
I think it makes more sense to define those here.
There's a weird local import on most of the functions in this file.
There was a problem hiding this comment.
Actually... Do the methods here need to be functions here?
Since they are just transformations, can't they live in the dataclasses itself? e.g. instead of map_from_mcp_resource, you could do Resource.from_mcp_resource(mcp_resource).
There was a problem hiding this comment.
Yep, totally. I originally implemented them like so mostly as that was (roughly) the pattern that already existed in that file but I actually prefer Resource.from_mcp_resource(mcp_resource) too. Will change.
|
|
||
| @dataclass(repr=False, kw_only=True) | ||
| class ResourceAnnotations: | ||
| """Additional properties describing MCP entities.""" |
There was a problem hiding this comment.
Can we please have a link reference in the docstring so we can easily check it in the future?
| """Additional properties describing MCP entities.""" | |
| """Additional properties describing MCP entities. | |
| See the [resource annotations in the MCP specification](link). | |
| """ |
Or something like that.
| @classmethod | ||
| def from_mcp_sdk_error(cls, error: mcp_exceptions.McpError) -> MCPError: | ||
| """Create an MCPError from an MCP SDK McpError. | ||
| Args: | ||
| error: An McpError from the MCP SDK. | ||
| Returns: | ||
| A new MCPError instance with the error data. | ||
| """ | ||
| # Extract error data from the McpError.error attribute | ||
| error_data = error.error | ||
| return cls(message=error_data.message, code=error_data.code, data=error_data.data) |
There was a problem hiding this comment.
Just because we're returning our own/native MCPError type, rather than the mcp-sdk one and we're flattening it out. The MCP SDK error type has a nested error element as it exactly mirrors the protocol JSON, whilst ours is flattened out.
Apologies if I've misinterpreted your question 🙂.
There was a problem hiding this comment.
I'm still not sure this is necessary, since the developer has access to the exception cause, since we are doing raise ... from e.
Do we have a test demonstrates how useful this would be?
| try: | ||
| result = await self._client.list_resources() | ||
| except mcp_exceptions.McpError as e: | ||
| raise MCPError.from_mcp_sdk_error(e) from e |
There was a problem hiding this comment.
Can we have a test on the error output?
Also, on our side, I think we should be more specific on the errors, MCPError should be the parent of the other errors, e.g. in this case ResourceNotFound(MCPError) or ResourceNotSupported(MCPError) (?).
There was a problem hiding this comment.
Yeah, this is broadly how I originally implemented this, however, because we went the return [] | None route they got removed (more on this in following comment).
| try: | ||
| result = await self._client.read_resource(AnyUrl(resource_uri)) | ||
| except mcp_exceptions.McpError as e: | ||
| # As per https://modelcontextprotocol.io/specification/2025-06-18/server/resources#error-handling |
There was a problem hiding this comment.
I think we can explain a bit more here...
| # As per https://modelcontextprotocol.io/specification/2025-06-18/server/resources#error-handling | |
| # As per https://modelcontextprotocol.io/specification/2025-06-18/server/resources#error-handling | |
| # `-32002` defines "resource not found". |
There was a problem hiding this comment.
I disagree with @DouweM regarding raising an error vs returning None.
In this method, we can already see that None means different things:
- resource is not supported by the server.
- resource not found.
- no content in resource.
There was a problem hiding this comment.
Also, I think we can leverage the exceptions on the future exception handler mechanism.
There was a problem hiding this comment.
I disagree with @DouweM regarding raising an error vs returning
None.
Yeah, I can see arguments in both directions. FWIW, I think returning [] or None when the server doesn't have resource capabilities is reasonable, ie: list_resources() returning [] is materially the same and if you can't get any resource URIs, then you can't really call read_resource() anyway. I'm not sure raising an exception here adds much value for the user.
If a client really needs to know if a server supports resources (maybe to display UI elements or something) you can still access mcp_server.capabilities.
On returning None vs a ResourceNotFound - This is trickier. One related issue we have is that so far, none of the main reference MCP server implementations (typescript, python) actually implement the protocol spec correctly anyway (though it looks like a python fix might be on the way), which means it's hard to detect a real "not found".
FWIW 2, our closest related precedence for this is probably direct_tool_call(), however, it raises ModelRetry regardless of the actual underlying error, which arguably is also not correct if a user is calling it directly?
Maybe the right model for this is (roughly) how HTTP error codes are commonly handled? Rather than having different subclasses (ie: ResourceNotFound, ServerError, etc) we raise a generic error (maybe just MCPError), which has a code and a message and let users handle flow based on the specific codes they care about (much like users would with HTTP response codes 400-599)? ie:
try:
data = server.read_resource('url://here')
except MCPError as e:
print(f'An error occurred reading your resource: {e.message} (err: {e.code})')Maybe even define some useful common code maps?
RESOURCE_NOT_FOUND = -32002
INVALID_REQUEST = -32600
@Kludex you would have a better sense than I on this sort of thing 😄
Re:
Also, I think we can leverage the exceptions on the future exception handler mechanism.
Sorry, not quite sure what you mean by this - could you clarify?
There was a problem hiding this comment.
Just when you thought this couldn't get even more annoying, I just stumbled on this: modelcontextprotocol/modelcontextprotocol#1545
It turns out the spec itself is wrong/has a typo
This is probably a vote in the direction of raising errors with codes and letting clients work out what they want to do with them?
There was a problem hiding this comment.
I'm happy to defer to @Kludex's judgment when it comes to errors vs empty results etc, for what it's worth.
4 participants
Addresses #1783.
Adds support for MCP Resources to the client.
Changes
Resource Methods
list_resources()- Discover available resources on the serverlist_resource_templates()- List parameterized resource templates (RFC 6570)read_resource(uri)- Read resource contents, accepting string URI or Resource objectNative Types
Resource,ResourceTemplate,ResourceAnnotations, andServerCapabilitiestypesstrand binary content toBinaryContentmcplibrary types in the public APIError Handling
MCPServerCapabilitiesError,MCPServerErrorOther
MCPServer.capabilitiesproperty to inspect server featuresmcp>=1.18.0(required for Annotations tests)