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

Do not record tool call id if no tool has been called #571

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sysradium
Copy link
Contributor

@sysradium sysradium commented Feb 9, 2025

I came across this issue here: #570

The problem is that if you have callbacks which modify observations like it is done here https://github.com/huggingface/smolagents/blob/main/src/smolagents/vision_web_browser.py#L66

It may happen that there is no tool call recorded yet and hence self.tool_calls is None. (ref https://github.com/sysradium/smolagents/blob/web-tool-call-issue/src/smolagents/agents.py#L871)

For example in my case it happened with vision_web_browser.py when the model did not return any Python code on the first call to it. That led to the exception here https://github.com/sysradium/smolagents/blob/web-tool-call-issue/src/smolagents/agents.py#L867 which happens right before we set memory.step.tools_calls.

So essentially you end up having data:, as an observation but no recorded tool call.

Maybe not the best solution, but I decided to just not print Call id: ... if there has been no successful tool call.

@sysradium sysradium marked this pull request as ready for review February 9, 2025 19:01
@sysradium
Copy link
Contributor Author

@aymeric-roucher another one here if you have some time.

@aymeric-roucher
Copy link
Collaborator

Thank you @sysradium! Could you please add a regression test for this?

@sysradium
Copy link
Contributor Author

sysradium commented Feb 13, 2025

@aymeric-roucher sure, will try to figure out a way.

@sysradium
Copy link
Contributor Author

sysradium commented Feb 13, 2025

@aymeric-roucher

Added a test fixing some things along the way:

  1. Added mlx into dev deps. Otherwise after running pip install -e ".[dev]" on a clean setup would get:
FAILED tests/test_models.py::ModelTests::test_get_mlx_message_no_tool - ModuleNotFoundError: Please install 'mlx-lm' extra to use 'MLXModel': `pip install 'smolagents[mlx-lm]'`
  1. Modified Makefile target so that you could overrde python command. For example if you use uv now you can do:
PYTHON='uv run python' make test 
PYTHON='uv run python' make quality 

Or override it with python path you need. Just a bit of flexibility. It is backwards compatible, so if you don't override PYTHON variable you get the same behaviour.

  1. With a test fixture forced MultiAgentsTests to dump data into a tmpdir, otherwise you are getting unwanted guests:
>>> gst
Untracked files:
  (use "git add <file>..." to include in what will be committed)
        agent_export/
  1. Some minor type hint fixes.

@g-eoj
Copy link
Contributor

g-eoj commented Feb 16, 2025

Will pip install -e ".[dev]" fail on non-mac dev environments with this change?

@sysradium
Copy link
Contributor Author

@g-eoj I don't think so. You can check here https://pypi.org/project/mlx/#files that they have prebuilt binaries for linux and mac. As well as I found that it has a windows support ml-explore/mlx#1513

@aymeric-roucher a kind ping

@sysradium
Copy link
Contributor Author

@albertvillanova rebased the change.

@sysradium
Copy link
Contributor Author

@aymeric-roucher rebased

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.

Thanks.

The rebase seems did not work properly. I'm cherry-picking the relevant commits.

@sysradium
Copy link
Contributor Author

@albertvillanova what issues are there? I can't see conflicts atm

@sysradium sysradium force-pushed the web-tool-call-issue branch from a4673a9 to 60b91d2 Compare March 3, 2025 14:11
@sysradium sysradium force-pushed the web-tool-call-issue branch from 60b91d2 to db38e88 Compare March 3, 2025 14:12
@sysradium
Copy link
Contributor Author

Rebased one more time, ran quality checks and tests.

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.

5 participants