Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add support for MCP Servers tools as ToolCollection #232

Merged
merged 17 commits into from
Jan 17, 2025
Merged

add support for MCP Servers tools as ToolCollection #232

merged 17 commits into from
Jan 17, 2025

Conversation

grll
Copy link
Contributor

@grll grll commented Jan 16, 2025

This Pull Request add support for any MCP Servers by making tools from an MCP server accessible as a ToolCollection object to the user.

It changes ToolCollection to enable two behaviors one load from the hub via from_hub (same as previous behavior). one to load tools from an mcp server via from_mcp.

I updated the documentation and added example, let me know if there is any further additions to make!

Fix #60

@grll grll mentioned this pull request Jan 16, 2025
Closed
albertvillanova
albertvillanova reviewed Jan 17, 2025
View reviewed changes
Copy link
Member

@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are conflicts with the main branch, could you please fix them?

Additionally, I would suggest setting mcpadapt as an optional dependency with an mcp extra?

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

There are conflicts with the main branch, could you please fix them?

Additionally, I would suggest setting mcpadapt as an optional dependency with an mcp extra?

Hi @albertvillanova many thanks for the review!

  1. let me fix the conflicts.

  2. I can definitely put it as optional dependency but it's extremely lightweight as I am already using optional dependency in mcp adapt for the various agent framework so basically you get only the mcp official python sdk which is very small and mcpadapt it self with no extra also very small. But really as you prefer let me know!

@grll grll closed this Jan 17, 2025
@albertvillanova
Copy link
Member

Yes, I would suggest making it optional. In fact, we are in the process of moving most of the current dependencies to optional.

@albertvillanova
Copy link
Member

Oh, I noticed you closed your PR!?

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

that's odd not sure how

@grll grll reopened this Jan 17, 2025
@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

I put as optional dependencies and made sure to raise a nice error if someone try to load tool collection from_mcp without the extra

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

@albertvillanova let me know if any other changes required :)

albertvillanova
albertvillanova reviewed Jan 17, 2025
View reviewed changes
Copy link
Member

@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

Yes, that is the way we are doing it: trying the optional import, and raising a error message if not installed. See:

On the other hand, I would suggest setting both from_hub and from_mcp as class methods instead of static methods. What do you think?

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

Great.

Yes, that is the way we are doing it: trying the optional import, and raising a error message if not installed. See:

On the other hand, I would suggest setting both from_hub and from_mcp as class methods instead of static methods. What do you think?

I have used static here because I don't use the 'cls' arg but could change it to classmethod if you prefer. I see above the Tool.from_space is also defined as a staticmethod so I thought this was the way.

@albertvillanova
Copy link
Member

I think Tool.from_space is not defined as a class method because it returns an instance of SpaceToolWrapper (which is a subclass of Tool).

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

hmm ok let's go for classmethod as it relates to the class still then

albertvillanova
albertvillanova reviewed Jan 17, 2025
View reviewed changes
Copy link
Member

@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not have explained myself clearly earlier. My intention was to use class methods so that the returned instances are created with the cls argument.

grll and others added 2 commits January 17, 2025 16:58
@grll @albertvillanova
Update src/smolagents/tools.py
44f06a7 
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
@grll @albertvillanova
Update src/smolagents/tools.py
b9258a0 
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

@albertvillanova make sense haha a bit silly to not use cls to instanciate indeed

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

fixed

albertvillanova
albertvillanova reviewed Jan 17, 2025
View reviewed changes
Copy link
Member

@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure about the functionality, do you think it is feasible to include a test?

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

Yes definitely in the meantime you can try by running the example added

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

ah wait I think I have a better test :)

@albertvillanova
Copy link
Member

Oh, sorry, I thought you preferred me to try a test... I misunderstood! 😅

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

no problem I think it's better because we don't need to mock we can actually test the functionality for real :)

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

all good for me

albertvillanova
albertvillanova reviewed Jan 17, 2025
View reviewed changes
tests/test_tools.py Outdated
Comment on lines 93 to 98
# def test_agent_type_output(self):
# inputs = create_inputs(self.tool.inputs)
# output = self.tool(**inputs, sanitize_inputs_outputs=True)
# if self.tool.output_type != "any":
# agent_type = AGENT_TYPE_MAPPING[self.tool.output_type]
# self.assertTrue(isinstance(output, agent_type))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you commented this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry my bad I had issues to run the test on my ubuntu machine with the import with torch so I commented everything out let me fix

albertvillanova
albertvillanova reviewed Jan 17, 2025
View reviewed changes
tests/test_tools.py Outdated
Comment on lines 391 to 409
@pytest.fixture
def mock_server_parameters():
return MagicMock()


@pytest.fixture
def mock_mcp_adapt():
with patch("mcpadapt.core.MCPAdapt") as mock:
mock.return_value.__enter__.return_value = ["tool1", "tool2"]
mock.return_value.__exit__.return_value = None
yield mock


@pytest.fixture
def mock_smolagents_adapter():
with patch("mcpadapt.smolagents_adapter.SmolAgentsAdapter") as mock:
yield mock


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need these mocks for my unit test.

I think we can keep both tests: my unit and your integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure let's put it back

grll and others added 2 commits January 17, 2025 19:18
@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

let's hope we pass the tests now ;)

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

we have an issue with the unit test you want to fix or?

@albertvillanova
Copy link
Member

Let me fix it.

albertvillanova
albertvillanova approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@albertvillanova albertvillanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @grll! Thanks a lot! 🤗

@albertvillanova albertvillanova merged commit a4d029d into huggingface:main Jan 17, 2025
4 checks passed
@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

@albertvillanova Thanks for the support in merging this!

@aymeric-roucher
Copy link
Collaborator

Thanks a lot for your contribution @grll !

@grll
Copy link
Contributor Author

grll commented Jan 17, 2025

@aymeric-roucher thanks for the initial pointers! Any ideas around next release timeline plans ?

@albertvillanova
Copy link
Member

Just in progress...

@aymeric-roucher
Copy link
Collaborator

aymeric-roucher commented Jan 17, 2025 •
edited
Loading

Done! Release notes here 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertvillanova albertvillanova albertvillanova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Question : any plan for model context protocol integration ?
3 participants
@grll @albertvillanova @aymeric-roucher