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.

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

Fix final_answer issue in e2b_executor #319

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

keetrap
Copy link
Contributor

@keetrap keetrap commented Jan 22, 2025

Additional Context:

  • While working on the fix, I encountered an installation issue with the current PyPI release on smolagents on e2b.
  • I tested the changes by removing torchvision and transformers which allowed the package to be installed successfully.

@keetrap keetrap changed the title fix final_answer issue in e2b_executor Fix final_answer issue in e2b_executor Jan 23, 2025
@keetrap
Copy link
Contributor Author

keetrap commented Jan 26, 2025

@aymeric-roucher @albertvillanova
Please let me know if there is any problem with this PR. Should I close if it not needed.

@@ -97,6 +100,7 @@ def visit_Name(self, node):
or node.id in self.imports
or node.id in self.from_imports
or node.id in self.assigned_names
or node.id in self.typing_names
Copy link
Collaborator

Choose a reason for hiding this comment

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

@keetrap is this fixing any failing tool initialization? Normally, having Any in tool arguments is supported and tested in tests/test_tools.py::test_tool_supports_any_none, tell me if you have a counterexample!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aymeric-roucher
Yes, this fix the FinalanswerTool without this code the error is:

ValueError: Tool validation failed:
- forward: Name 'Any' is undefined.

This error only occurs when use_e2b_executor=True

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then this is due to calling method Tool.save! Could you add this line in test_tool_supports_any_none (in tests/test_tools.py):
get_weather.save()
This may force you to add None as well to self.typing_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the line and ran the tests. The error was: TypeError: Tool.save() missing 1 required positional argument: 'output_dir'. After providing the output_dir, the tests passed without adding None to self.typing_names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am still confused as to why there is an error, and this only occurs when use_e2b_executor=True. It is not occurring in the local Python executor because we are not validating the tool attributes there.

Thanks for merging this PR.

@aymeric-roucher aymeric-roucher merged commit 33b38e6 into huggingface:main Jan 28, 2025
2 of 3 checks passed
@keetrap keetrap deleted the Fix_E2B_Issue branch January 30, 2025 17:13
@keetrap keetrap mentioned this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use_e2b_executor=True does not seem to work
2 participants