-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add WikipediaSearchTool to default tools #514
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
Add WikipediaSearchTool to default tools #514
Conversation
Anything else you want me to add @aymeric-roucher ? |
Co-authored-by: Aymeric Roucher <[email protected]>
One last thing: we don't allow init arguments that don't have a default in tools (to be able to serialize them easily). So could you set a default value like "smolagents" to the user_agent arg? |
Ok got it Something like this:
|
Set default value of user_agent to "Smolagents ([email protected])"
Nice ! And one last last thing: please add a test with a pytest.parameterize to test un summary mode vs full mode, two different languages and requests! |
@aymeric-roucher test cases for Default Tools are written using |
@aymeric-roucher let know if this is ok or you want me to change anything |
tests/test_default_tools.py
Outdated
@@ -33,6 +39,40 @@ def test_ddgs_with_kwargs(self): | |||
result = DuckDuckGoSearchTool(timeout=20)("DeepSeek parent company") | |||
assert isinstance(result, str) | |||
|
|||
def test_wikipedia_search_tool(self): |
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.
@albertvillanova should we prefer pytest for these kind of grid tests?
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.
@aymeric-roucher current test test_wikipedia_search_tool
is not ok?
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.
Yes @touseefahmed96 it's better to use pytest.parametrize
: you can find examples in other tests.
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.
@aymeric-roucher i did use the pytest.parametrize
but for that to work we have to write seperate class for this tool because we cannot integrate it in DefaultToolTests
as its is unittest
and pytest.parametrize
does not work with unittest.TestCase
- Replaced unittest subTest with pytest.mark.parametrize for WikipediaSearchTool as suggested.
- Replaced unittest subTest with pytest.mark.parametrize for WikipediaSearchTool as suggested.
@aymeric-roucher I created a new test function for Wikipedia search tool and used |
@touseefahmed96 please check failing tests (https://github.com/huggingface/smolagents/actions/runs/13364705034/job/38867123683?pr=514): you need to add your dependancy to test requirements for tests to pass. |
@aymeric-roucher I added the wikipedia in test requirements but i dont know why the second one is failing the Models test one |
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.
Thanks, @touseefahmed96!
Just a comment below.
To fix the CI, please merge the main branch:
git fetch upstream main
git merge upstream/main
Done @albertvillanova |
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.
You forgot to update your tests after the last modification.
Done @albertvillanova |
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.
Thank you! 🤗
Example Code:
output :