Repository navigation
Fix TypeError on Python 3.14 for union-typed fields - #70
devangpratap wants to merge 1 commit into
Conversation
On Python 3.14, building a parser for any config containing a union or
Optional field raises before a single argument is parsed:
TypeError: str | None is not callable
3.14 validates that argparse's `type` is callable inside `add_argument`,
and union objects are not callable there: `typing.Optional[str]` is a
`_GenericAlias` on 3.13 but a `Union` on 3.14. One `Optional[str]` field
is enough to make draccus unusable.
The value is display-only. `parse_known_args` overwrites `action.type`
with `str` before parsing whenever --help is absent, since draccus decodes
every value itself, so `type` only renders the type name in --help.
Wrap every field's type in a pass-through and keep the declared type
reachable through `__draccus_type__` so `get_metavar` still renders
`--path [str]` rather than `--path str | None`. Wrapping unconditionally
rather than only for non-callables keeps behaviour identical on every
version: it also fixes `prog --path foo --help`, which errored with
"invalid Optional[str] value" on 3.9-3.13, and avoids writing `__name__`
onto cached typing objects.
Adds 3.12, 3.13 and 3.14 to the CI matrix, which stopped at 3.11 and is
why this was not caught.
a40c8ee to
6f3f164
Compare
|
Pushed an update after putting this through a second review pass. Two real problems in my original patch, plus one scope decision: 1. The first version regressed My "snapshots unchanged" claim above was true but did not prove what I implied: no 2. The That second point widens this slightly beyond "fix the 3.14 crash". I think it belongs here since it is the same code path and removes the inconsistency the fix would otherwise introduce, but say the word and I will split it out. 3. CI matrix now adds 3.12 and 3.13 alongside 3.14, since those are exactly where the old callable branch used to run. Still happy to drop any of these lines if they cause friction. Updated results:
The 3.9 failure is still only I left |
Fixes #69.
The bug
On Python 3.14, building a parser for any config containing a union or
Optionalfield raises before a single argument is parsed:One
Optional[str]field is enough to make draccus unusable on 3.14.Root cause
Python 3.14 added a callability check to
argparse.add_argument, which raisesTypeError: {type_func!r} is not callableat registration time. It previously accepted any object and only failed if a conversion was actually attempted.Union objects do not pass that check on 3.14, because
typing.Unionwas reimplemented:typing.Optional[str]callable(...)typing.Optional[str](_GenericAlias)Truestr | None(Union)FalseFieldWrapper.arg_optionspasses the canonicalized field type straight through astype, so every union field trips the new check.Why the fix is safe
The
typevalue is display-only.ArgumentParser.parse_known_argsalready replaces it before parsing:draccus decodes every value itself in
_postprocessingviadecoding.decode, sotypeis never used for conversion. It survives only when--helpis requested, purely to render the type name, which is what the existing__name__assignment was for.This is also why the bug does not show up as a conversion error on older versions: with
type=Optional[str]a plain argparse parser fails on 3.12 and 3.13 withinvalid Optional value: 'foo', but draccus never lets argparse convert.So the fix keeps the displayed name and wraps only non-callable types in a pass-through. Callable types are untouched.
Test results
The single 3.9 failure is
tests/test_literals.py::test_literal_help_text, which fails identically on unmodifiedmaster; I confirmed it by stashing the change and re-running. It is unrelated to this PR.All 15 syrupy snapshots pass unchanged on every version, which is the check that matters most here since it confirms
--helpoutput is byte-identical.What is in the diff
draccus/wrappers/field_wrapper.py:_display_typehelper, used wheretypewas previously assigned raw.tests/test_union.py:test_union_field_builds_parser_and_parses. It fails withTypeErroratargparse.py:1549on 3.14 without the fix and passes with it..github/workflows/pytest.yml: adds3.14to the matrix, which stopped at3.11. That gap is why this shipped. If adding it causes any friction in your CI, drop that line and I am happy to follow up separately.Downstream
This currently takes out LeRobot entirely:
PreTrainedConfigthere has twodict[str, PolicyFeature] | Nonefields, so everyPolicy.from_pretrainedraises. Filed as huggingface/lerobot#4509.