Skip to content

Fix ParseKwargs crash on a --model-kwargs value containing '=' or a non-literal - #2758

Merged
rwightman merged 2 commits into
huggingface:mainfrom
winklemad:fix/parsekwargs-value-with-equals
Sep 1, 2026
Merged

rwightman merged 2 commits into
huggingface:mainfrom
winklemad:fix/parsekwargs-value-with-equals

Conversation

@winklemad

Copy link
Copy Markdown
Contributor

What

ParseKwargs (used by --model-kwargs / --opt-kwargs in the train/validate scripts) crashes on some valid inputs:

$ python -c "import argparse; from timm.utils.misc import ParseKwargs; \
  p=argparse.ArgumentParser(); p.add_argument('--model-kwargs', nargs='*', default={}, action=ParseKwargs); \
  print(p.parse_args(['--model-kwargs','note=size=large']))"
ValueError: too many values to unpack (expected 2)

Two issues in the same spot:

  1. key, value = value.split('=') splits on every =, so any value containing an = (e.g. note=size=large, or a URL with a query string) raises too many values to unpack.
  2. The literal_eval fallback only catches ValueError. A value that isn't a valid Python literal but is invalid syntax (like size=large) raises SyntaxError, which isn't caught, so it crashes instead of falling back to a string.

Fix

  • Split on the first = only (split('=', 1)).
  • Catch SyntaxError as well as ValueError in the string fallback.

Both are minimal and the only key=value parser in the tree (grep confirms misc.py:29 is the sole .split('=')).

After

note=size=large            -> {'note': 'size=large'}
url=http://h/p?a=1&b=2     -> {'url': 'http://h/p?a=1&b=2'}
depth=12 act=nn.GELU       -> {'depth': 12, 'act': 'nn.GELU'}
cfg={"a": 1}               -> {'cfg': {'a': 1}}

Existing literal parsing (ints, floats, bools, dicts) is unchanged.

Tests

Added tests/test_utils.py covering literal values, a value containing =, and the non-literal string fallback. The note=size=large case fails on main (both bugs) and passes here.

…on-literal

value.split('=') split on every '=', so a value with an '=' in it (e.g.
note=size=large, or a URL with a query string) raised 'too many values to
unpack'. Split on the first '=' only. The literal_eval fallback also only
caught ValueError, so a value that is not a valid Python literal (invalid
syntax like 'size=large') raised SyntaxError instead of falling back to a
string; catch SyntaxError too.
@rwightman

Copy link
Copy Markdown
Collaborator

@winklemad there was an existing test_utils.py that this PR stomps over...

…lacing it

The branch predated the current tests/test_utils.py; rebased so the ParseKwargs
tests are added alongside the existing freeze/unfreeze and AverageMeter tests
rather than overwriting them.
@winklemad

Copy link
Copy Markdown
Contributor Author

Good catch, sorry about that @rwightman — the branch predated the current test_utils.py. Fixed: the ParseKwargs tests now append to the existing file alongside the freeze/unfreeze and AverageMeter tests instead of replacing it.

@rwightman
rwightman merged commit e055202 into huggingface:main Sep 1, 2026
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.

2 participants