Repository navigation
fix: stop forwarding Hub download options to the tool constructor - #2935
TINGyu123644 wants to merge 4 commits into
Conversation
`Tool.from_hub()` documents that extra kwargs are split between Hub download options and tool-constructor options. The download options (`cache_dir`, `force_download`, `proxies`, `revision`, `subfolder`, `local_files_only`) were only read with `kwargs.get(...)`, so they stayed in `kwargs` and were forwarded to `Tool.from_code(tool_code, **kwargs)`, which eventually calls `tool_class(**kwargs)`. A tool with a no-argument constructor therefore failed with `TypeError: __init__() got an unexpected keyword argument 'cache_dir'`. Pop the download options instead of reading them, so only tool-specific constructor arguments reach `from_code`. Tool-specific arguments are still forwarded unchanged. Fixes huggingface#2928
VANDRANKI
left a comment
There was a problem hiding this comment.
Community review (not a maintainer, does not clear the merge gate). I read the full diff and compared it with its merge base (96f33fa, current main) byte for byte.
The first thing to know: the diff shows +2536 -2485 across src/smolagents/tools.py and tests/test_tools.py, but almost all of that is a line-ending change. In the merge base both files use LF (0 carriage returns in tools.py); on this PR head tools.py has 1422 CRLF lines out of 1422, and tests/test_tools.py is CRLF throughout as well. With git diff -w --ignore-cr-at-eol the real change is 6 lines in tools.py plus 51 added lines in the tests. As submitted, this would rewrite every line of two files, destroy git blame, and conflict with any other open PR touching them. I would redo the commit with LF endings (git config core.autocrlf input, or re-save the files as LF) so the diff is the 6 + 51 lines. The PR description also looks mangled (a BOM at the start and stray backslashes in the code spans), which is probably the same editor or shell problem.
The code change itself is right. Tool.from_hub now uses kwargs.pop(...) for cache_dir, force_download, proxies, revision, subfolder and local_files_only, so they reach hf_hub_download and no longer reach Tool.from_code(tool_code, **kwargs). What I ran, with PYTHONPATH=C:/sm/src and a mocked hf_hub_download (no network):
- The new
test_from_hub_pops_hub_download_optionspasses on this PR head. - With only
tools.pyput back to the merge base, the same test fails withTypeError: EchoTool.__init__() got an unexpected keyword argument 'cache_dir'. So the test reproduces #2928 and the fix removes it. - I did not run the rest of the suite and did not call the real Hub.
Duplicates: #2931 (opened 06:59Z, 12 lines, one file) and #2936 (85 lines) fix the same issue, and this one is the third. Of the three, #2931 is the smallest, and this one's clean diff would be about the size of #2936. The maintainers will probably want only one, so it may be worth agreeing on which among you before more time goes into it. I only checked that the three target the same lines; I did not diff #2931 and #2936 against this one.
The tests use patch on smolagents.tools.hf_hub_download and assert the download kwargs, which is a good shape; a second assertion that token and repo_type are still forwarded would guard against a future regression, but that is optional.
The prior commit wrote src/smolagents/tools.py and tests/test_tools.py with CRLF line endings, turning the PR diff into full-file noise. Normalize both files to LF so only the actual Tool.from_hub change remains in the diff.
|
@VANDRANKI Thanks for the byte-for-byte review — you were right about the line endings. Fixed on the branch (
On the duplication with #2931/#2936: the code here is functionally the same fix, and I'm happy to close this PR in favor of whichever one the maintainers prefer to keep — let me know. |
|
Thanks for the LF fix. I re-checked the new head (f736a64): the diff is now What I ran (Windows, Python 3.13,
One thing will fail CI. On the overlap with #2931 and #2936: all three are still open and I cannot pick between them, that is for the maintainers. I did not run the other two. |
ruff format --check reports three small hunks in the new regression test: blank line before the def, triple-quote style for the dedent source, and the with-statement header collapsed to one line. Apply them so the quality CI (ruff format --check tests) passes on this head.
Only the three hunks ruff format --check (line-length 119) reports for the new regression test: blank line before the def, triple-quote style of the dedent source, and the with-statement header collapsed to one line.
|
@VANDRANKI Thanks for the precise report — fixed on the branch. The three hunks are applied to
Verified with the repo's own config ( Thanks again for the byte-for-byte review. |
|
@VANDRANKI One heads-up: the CI workflows on head |
Fixes #2928
What changed
Tool.from_hub() documents that extra kwargs are split between Hub
download options and tool-constructor options. In practice the download
options (cache_dir, orce_download, proxies,
evision,
subfolder, local_files_only) were only read with kwargs.get(...),
so they stayed in kwargs and were forwarded to Tool.from_code(tool_code, **kwargs), which eventually calls ool_class(**kwargs).
A tool with a no-argument constructor therefore failed with:
ext TypeError: EchoTool.__init__() got an unexpected keyword argument 'cache_dir'This change pops the Hub download options before the constructor is
invoked, so only tool-specific constructor arguments reach rom_code.
Why it matters
The documented split now actually happens: cache_dir,
local_files_only, etc. are used by the download layer and excluded from
the tool-constructor arguments, while tool-specific kwargs are still
forwarded unchanged.
How it was tested
loads a no-argument-constructor tool through rom_hub with Hub
download options plus a tool-specific kwarg, and asserts the download
layer received the Hub options while the constructor only received its
own kwarg.
(TypeError) and passes with this fix.