-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Feature: Minimal MCP client Resources support #3024
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?
Changes from 7 commits
9fde764
a7404a2
ecea466
1886299
dfb54db
f52bb71
232ff04
38675da
317416c
67f9b07
e6cb086
b8424d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
import httpx | ||
import pydantic_core | ||
from anyio.streams.memory import MemoryObjectReceiveStream, MemoryObjectSendStream | ||
from pydantic import BaseModel, Discriminator, Field, Tag | ||
from pydantic import AnyUrl, BaseModel, Discriminator, Field, Tag | ||
from pydantic_core import CoreSchema, core_schema | ||
from typing_extensions import Self, assert_never, deprecated | ||
|
||
|
@@ -275,6 +275,36 @@ def tool_for_tool_def(self, tool_def: ToolDefinition) -> ToolsetTool[Any]: | |
args_validator=TOOL_SCHEMA_VALIDATOR, | ||
) | ||
|
||
async def list_resources(self) -> list[mcp_types.Resource]: | ||
"""Retrieve resources that are currently present on the server. | ||
|
||
Note: | ||
- We don't cache resources as they might change. | ||
- We also don't subscribe to resource changes to avoid complexity. | ||
""" | ||
async with self: # Ensure server is running | ||
result = await self._client.list_resources() | ||
return result.resources | ||
|
||
async def list_resource_templates(self) -> list[mcp_types.ResourceTemplate]: | ||
"""Retrieve resource templates that are currently present on the server.""" | ||
async with self: # Ensure server is running | ||
result = await self._client.list_resource_templates() | ||
return result.resourceTemplates | ||
|
||
async def read_resource(self, uri: str) -> list[mcp_types.TextResourceContents | mcp_types.BlobResourceContents]: | ||
"""Read the contents of a specific resource by URI. | ||
|
||
Args: | ||
uri: The URI of the resource to read. | ||
|
||
Returns: | ||
A list of resource contents (either TextResourceContents or BlobResourceContents). | ||
""" | ||
async with self: # Ensure server is running | ||
result = await self._client.read_resource(AnyUrl(uri)) | ||
return result.contents | ||
|
||
|
||
async def __aenter__(self) -> Self: | ||
"""Enter the MCP server context. | ||
|
||
|
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.
@Kludex You suggested having these methods return types of our own, but I'm not super convinced it's worth it to redefine this:
And
Annotations
that it references:And
Role
that that references:... instead of just letting the user use MCP types directly for certain things. Although I suppose in that case, instructing users to just use
client
directly to get access to MCP stuff directly would suffice as well.There's also the fact that
list_tools
already returnslist[mcp_types.Tool]
.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.
Actually I'm fine with this. @fennb Can you define our own matching types please, as dataclasses with
snake_case
fields rather thancamelCase
?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.
Yeah, I think it's one or the other, right? Either just expose
client
directly and let users use the nativemcp
types or convert the types and expose directly onMCPServer
?The place I've left it (which is kind of a mix) is probably "wrong" now that I reflect on it.
Happy to take this further and try to add the native type conversion (or @Kludex can run with it) if we think this is the right path.
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.
Hah, we submitted comments at the same time @DouweM - just saw your most recent comment. I'll run with this this direction and push an update.
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.
If we are not redefining the types, I don't think we should expose those methods. I think exposing the
client
would be enough.Is it really worth defining those types here, tho? 👀
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.
@Kludex If we'll have
read_resource
that returns our types, which would be useful, I think we should also havelist_resources
, which should then also return our typesThere 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.
Great question.
Yeah, having had a bit more of a chance to look at/think about this, I think this is where I landed too. All public
MCPServer
methods should return native Pydantic AI types, which is (sort of) whyMCPServer
wrapsmcp
'sClientSession
in the first place.I'll get going on implementing this and you guys can let me know what you think.
Side note 1:
list_tools(self) -> list[mcp_types.Tool]
is an oddity and I suspect shouldn't exist (or shouldn't be public) -MCPServer.get_tools()
is the only thing that uses it and is probably the right "public" method for users to use. Do we want to do anything here?Side note 2: As part of investigating things, I looked into the origins of the
ResourceLink
/_get_content()
functionality mentioned above: #2094 (comment) - I'm not sure this behaviour is quite right (though I absolutely understand why the decision was made that way). The MCP spec says that tools can return both Resource Links and Embedded Resources, but they're not the same thing (see here).At the moment, Pydantic transparently fetches resources from the link and returns them inline to the model as part of the tool call result. But surely that's not the intended use given the fact there's already embedded resources as a separate type?
ie: Imagine a hypothetical tool called
find_relevant_documents(query: str, limit: int = 50)
that returned 50 resource links to documents (each linked document being 1MB) so that a user can click on the one they want to load/select it/whatever. At the moment, Pydantic AI would try to fetch all the docs immediately and include them in context transparently, which seems... wrong.An approach would be to leave them as links and then it's up to the user to prompt the agent appropriately in telling it what to do with them (ie: display them verbatim for the actual client to handle or whatever). This is less controversial if the
ResourceLink.uri
ishttps://some.server.com/path/file.ext
but weirder if it'sdocument://legal/archive/blah.txt
(or whatever). I don't really know what an agent would do in that case. Arguably,ResourceLinks
maybe shouldn't be used this way, but I think blindly fetching them may not be the right default?Either way, I've gotten off topic and can separately lodge the above as a separate issue if you think it's worth it. I don't know how much changing this will break existing users expectations or not.
Uh oh!
There was an error while loading. Please reload this page.
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 agree, but we can't change that as it'd be a breaking change and we're on v1 now so can't make those until v2.
I agree the resource link behavior is probably incorrect. I tried to get clarification on that in modelcontextprotocol/modelcontextprotocol#872 (reply in thread), but didn't hear back. It would be easier if the model had a
read_resource
tool available to it, so it can choose which to load, but that'd need to be provided by the user. MaybeMCPServer
should have a flag to include that as a tool, and if provided, we wouldn't auto-read resource links?A separate issue would be great!
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 didn't even think about
list_tools
because it's called internally anyway, but yeah, I agree it should return the PydanticAI types.As for the new methods, I think they should return the PydanticAI types - where are we on 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.
Good timing (and sorry for slow follow up). Just pushed updates. This is, admittedly, a fair amount of boilerplate to not achieve a massive amount (at this point), but I can see some future reasons related to my commentary on ResourceLink above that this will become more important (more on this in the other issue which I'll create shortly).