-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added MCP elicitation support for tool injection in mcp-run-python #2258
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?
Conversation
322696f
to
393f025
Compare
@yamanahlawat Thank you! This is a great feature, but we'll want to think a bit about where certain code lives. For example, the 2 callbacks methods are too MCP/ Can you try to move those to Those functions then wouldn't have access to |
@DouweM Thanks. make sense about the placement. i was initially skeptical about adding these methods directly to the Agent class. i'll:
and if |
cee4c16
to
7730660
Compare
cf1f4f4
to
04124c2
Compare
@DouweM Please review. |
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.
@yamanahlawat Thanks Yaman! This looks a lot better already but I think it'll take a few more cycles until this is ready for merge -- check out my feedback. I also suggest pulling out elicitation_callback
into a new PR so we can merge that one quickly and others can start using it, while we work on the rest.
pass | ||
|
||
if available_tools: | ||
tool_arguments = {**tool_arguments, 'tools': available_tools} |
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.
Can we pass this through metadata instead?
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, what if we make mcp-run-python use elicitiation to request the available tools, so that we only need to set an elicitation_callback
and not also a process_tool_call
?
@@ -66,6 +235,8 @@ class MCPServer(AbstractToolset[Any], ABC): | |||
allow_sampling: bool = True | |||
max_retries: int = 1 | |||
sampling_model: models.Model | None = None | |||
allow_elicitation: bool = True | |||
elicitation_callback: ElicitationFnT | None = None |
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.
Please pull this new elicitation_callback
property out into a new PR so we can merge that pretty quickly and close #2330?
) | ||
|
||
|
||
def create_tool_elicitation_callback(toolset: AbstractToolset[Any]) -> ElicitationFnT: |
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.
Please move the mcp-run-python specific helper functions to pydantic_ai.ext.mcp_run_python
so that this file and its elicitation stuff can stay generic.
self.wrapped = wrapped | ||
|
||
# If it's a CombinedToolset, filter out mcp-run-python toolsets before any get_tools() calls | ||
if isinstance(wrapped, CombinedToolset): |
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 won't work if there are multiple levels of nested toolsets, unfortunately.
Could we require users to specify their own toolset to provide to mcp_run_python, and make them responsible for not including mcp_run_python itself? Maybe there's another point where we could detect this and raise an error, instead of trying to filter proactively?
def _set_sampling_model(toolset: AbstractToolset[AgentDepsT]) -> None: | ||
if isinstance(toolset, MCPServer): | ||
toolset.sampling_model = sampling_model | ||
|
||
self._get_toolset().apply(_set_sampling_model) | ||
|
||
def set_mcp_elicitation_toolset(self, toolset_for_elicitation: AbstractToolset[Any] | None = None) -> None: |
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 second thought, I think we should drop this method as it's too mcp-run-python specific to live on agent. I think it's fine to require the user to pass their own toolset to create_tool_elicitation_callback (and create_auto_tool_injection_callback)
""" | ||
filtered_toolset = _FilteredToolset(toolset) | ||
|
||
async def elicitation_callback(context: Any, params: Any) -> mcp_types.ElicitResult | mcp_types.ErrorData: |
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.
Instead of Any
, can we use the actual types from the mcp
package's ElicitationFnT
definition?
raise ValueError(f'Tool {tool_name} not found in toolset') | ||
|
||
tool = available_tools[tool_name] | ||
return await toolset.call_tool(tool_name, tool_arguments, ctx, 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.
Note that this will not perform argument validation. I think we'll need to build a ToolManager
and use handle_call
async def elicitation_callback(context: Any, params: Any) -> mcp_types.ElicitResult | mcp_types.ErrorData: | ||
"""Handle elicitation requests by routing to the toolset.""" | ||
try: | ||
tool_execution_data = json.loads(params.message) |
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.
Can we use TypeAdapter(ToolCallPart)
to validate this, which we can then directly pass through to tool_manager.handle_call
(see my other suggestion about using that instead of call_tool
)?
Uh oh!
There was an error while loading. Please reload this page.