SmolAgents MCP Client #1200
SmolAgents MCP Client #1200
Conversation
| def __enter__(self) -> list[Tool]: | ||
| """Connect to the MCP server and return the tools directly. | ||
|
|
||
| Note that because of the `.connect` in the init, the mcp_client |
There was a problem hiding this comment.
You don't think it is better to do a connect here? :)
There was a problem hiding this comment.
in my opinion it is better here and separately to be more explicit but the CrewAI folks told me they prefer to have it in the init so I thought it might be just me ;). Happy to make the change
There was a problem hiding this comment.
Alright, let's see what consensus on this is.
My feeling is that connect is a "side-effect" and __init__ is not the place for things like that. It gives a client more freedom on what when to connect.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
|
@albertvillanova we might want to add some doc to advertise the capabilities somewhere? |
|
I can fix the quality issue if it is OK for you: you will need to pull before adding your changes. |
|
I can have a shot at it later tonight I will pull your change before (I mean the doc) |
src/smolagents/mcp_client.py
Outdated
|
|
||
| def __init__( | ||
| self, | ||
| serverparams: StdioServerParameters | dict[str, Any] | list[StdioServerParameters | dict[str, Any]], |
There was a problem hiding this comment.
Why not calling this server_params or server_parameters?
There was a problem hiding this comment.
it seems the official python sdk use server_parameters so we could go for that
|
Hi @grll, we're planning to do a release this morning. Could you let me know if you'd prefer to wrap up this PR yourself, or if you'd like me to take care of the remaining changes? Just let me know what's most convenient for you. |
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
|
Are you ok with adding the doc? Is there something else left? |
|
I think it is the only missing thing... EDIT: And fixing quality and tests. |
There was a problem hiding this comment.
Thanks a lot for this excellent contribution!
The implementation of the MCPClient looks great and aligns well with the goals discussed. I really appreciate the addition of thorough tests: they'll be very helpful for maintainability. Also, thank you for the constructive discussions throughout the process; they've helped shaping a solid design.
|
@albertvillanova thanks a lot for pushing it over the finish line! I have plenty of ideas to improve this further and bring a lot more MCP features in smolagents, I really like the way this is going with a |
Fixes #1179
As discussed in the issue above the idea is to introduce a new class:
MCPClientallowing our users to manage the connection with one or several MCP servers.It supports 2 syntaxes:
ToolCollection.from_mcpis doing today.Note 1: I have moved the
.connect()inside the init of the MCPClient I am not sure it is desirable here. Initially I kept it outside even though it's one more call in my opinion it makes things more explicit while if you want less boilerplate you can always use the with syntax.Note 2: I have added a couple of tests but documentation is missing, waiting for a confirmation of the design and then I can add.
cc: @aymeric-roucher / @albertvillanova