-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added a mechanism to extract metadata from MCP tool call response #3339
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 6 commits
8f0c481
466e094
39e0a49
e021d47
ae1b882
e9d6475
22ded1b
191de6c
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -490,12 +490,23 @@ async def direct_call_tool( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The MCP SDK wraps primitives and generic types like list in a `result` key, but we want to use the raw value returned by the tool function. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # See https://github.com/modelcontextprotocol/python-sdk#structured-output | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return_value = structured | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(structured, dict) and len(structured) == 1 and 'result' in structured: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return structured['result'] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return structured | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return_value = structured['result'] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return messages.ToolReturn(return_value=return_value, metadata=result.meta) if result.meta else return_value | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mapped = [await self._map_tool_result_part(part) for part in result.content] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return mapped[0] if len(mapped) == 1 else mapped | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if result.meta: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The following branching cannot be tested until FastMCP is updated to version 2.13.1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # such that the MCP server can generate ToolResult and result.meta can be specified. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # TODO: Add tests for the following branching once FastMCP is updated. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( # pragma: no cover | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messages.ToolReturn(return_value=mapped[0], metadata=result.meta) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(mapped) == 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else messages.ToolReturn(return_value=mapped, metadata=result.meta) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return mapped[0] if len(mapped) == 1 else mapped | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def call_tool( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -574,16 +585,18 @@ async def list_resource_templates(self) -> list[ResourceTemplate]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return [ResourceTemplate.from_mcp_sdk(t) for t in result.resourceTemplates] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @overload | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def read_resource(self, uri: str) -> str | messages.BinaryContent | list[str | messages.BinaryContent]: ... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def read_resource( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, uri: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.TextPart | messages.BinaryContent | list[str | messages.TextPart | messages.BinaryContent]: ... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @overload | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def read_resource( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, uri: Resource | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.BinaryContent | list[str | messages.BinaryContent]: ... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.TextPart | messages.BinaryContent | list[str | messages.TextPart | messages.BinaryContent]: ... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def read_resource( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, uri: str | Resource | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.BinaryContent | list[str | messages.BinaryContent]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.TextPart | messages.BinaryContent | list[str | messages.TextPart | messages.BinaryContent]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Read the contents of a specific resource by URI. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -682,24 +695,29 @@ async def _sampling_callback( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def _map_tool_result_part( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, part: mcp_types.ContentBlock | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.BinaryContent | dict[str, Any] | list[Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str | messages.TextPart | messages.BinaryContent | dict[str, Any] | list[Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # See https://github.com/jlowin/fastmcp/blob/main/docs/servers/tools.mdx#return-values | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(part, mcp_types.TextContent): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| text = part.text | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if text.startswith(('[', '{')): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return pydantic_core.from_json(text) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except ValueError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return text | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if part.meta: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return messages.TextPart(content=text, metadata=part.meta) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def model_response_str(self) -> str: | |
| """Return a string representation of the content for the model.""" | |
| if isinstance(self.content, str): | |
| return self.content | |
| else: | |
| return tool_return_ta.dump_json(self.content).decode() | |
| def model_response_object(self) -> dict[str, Any]: | |
| """Return a dictionary representation of the content, wrapping non-dict types appropriately.""" | |
| # gemini supports JSON dict return values, but no other JSON types, hence we wrap anything else in a dict | |
| json_content = tool_return_ta.dump_python(self.content, mode='json') | |
| if isinstance(json_content, dict): | |
| return json_content # type: ignore[reportUnknownReturn] | |
| else: | |
| return {'return_value': json_content} |
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.
Okay, I see. Then should messages.TextContent and messages.TextPartlook exactly the same except thatmessages.TextContentcan havemetadataand it should not haveid` indicating an identifier for the part?
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 added a messages.TextContent, which is a bit of a problem with imports as TextContent because it conflicts with MCP types TextContent. Perhaps, a different name, such as messages.TextData is better unless we always import it as messages.TextContent or alias an import such as from pydantic_ai.messages import TextContent as PydanticAITextContent, which looks ugly.
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 we'll need a new
messages.TextContentobject, as the way we're usingTextParthere is incompatible with the"""A plain text response from a model."""docstring, and we do try to distinguish between parts and content.Then we should add
TextContentto theUserContentunion, so that it's allowed in place ofstr.And we should update the logic here to treat
UserContentjust likestrin terms of what's sent back to the model, i.e. not serializing the whole object which would include the metadata:pydantic-ai/pydantic_ai_slim/pydantic_ai/messages.py
Lines 816 to 830 in c27e5e4
def model_response_str(self) -> str: """Return a string representation of the content for the model.""" if isinstance(self.content, str): return self.content else: return tool_return_ta.dump_json(self.content).decode() def model_response_object(self) -> dict[str, Any]: """Return a dictionary representation of the content, wrapping non-dict types appropriately.""" # gemini supports JSON dict return values, but no other JSON types, hence we wrap anything else in a dict json_content = tool_return_ta.dump_python(self.content, mode='json') if isinstance(json_content, dict): return json_content # type: ignore[reportUnknownReturn] else: return {'return_value': json_content}
For this part, isn't if isinstance(self.content, str) already okay for my newly added messages.TextContent since it has a content, which is of type str?
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.
This will cause us to run into trouble here, because we're putting
BinaryContentinsideToolReturn.return_value:pydantic-ai/pydantic_ai_slim/pydantic_ai/_agent_graph.py
Lines 1161 to 1172 in c27e5e4
So we should make that logic less strict, so that if it finds a
ToolReturn.return_valuewithBinaryContentin it, it'll go through this same logic to move them to thecontentfield:pydantic-ai/pydantic_ai_slim/pydantic_ai/_agent_graph.py
Lines 1140 to 1159 in c27e5e4
(Although part of me is thinking that maybe I should do #3253 first, because that's going to affect so much of this logic and likely make it easier, as we can keep binary parts on
ToolReturn.return_valuewithout having to move them... If you can wait with this PR, I could maybe look at that one first?)We should also update the tests to not just test
direct_call_tool, but actually test using these tools in an agent run, to ensure we go through that part of the agent graph tool call result parsing code.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.
Okay, maybe I should wait for #3253 to be completed first?
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.
Okay, I will get to these.