Skip to content

Fix: allow Union[str, dict, None] fields like deepspeed to be passed via CLI#39625

Open
JH-debug wants to merge 2 commits intohuggingface:mainfrom
JH-debug:fix-hfparser-union-str-dict
Open

Fix: allow Union[str, dict, None] fields like deepspeed to be passed via CLI#39625
JH-debug wants to merge 2 commits intohuggingface:mainfrom
JH-debug:fix-hfparser-union-str-dict

Conversation

@JH-debug
Copy link
Copy Markdown

This PR fixes an issue in HfArgumentParser that causes a ValueError when parsing fields of type Union[str, dict, None] from the command line, such as the deepspeed argument in TrainingArguments.

The original parser rejected this valid type signature, even though it is commonly used to support both JSON config files (via path strings) and dictionary input. This PR adds a specific check to treat such fields as str when parsed via CLI, while still allowing JSON/dict parsing from file-based or programmatic inputs.

Motivation & Context

When passing --deepspeed path/to/ds_config.json from the CLI, the parser raised the following error:

ValueError: Only Union[X, NoneType] (i.e., Optional[X]) is allowed for Union ...

This was because Union[str, dict, None] didn't meet the parser’s strict rule of "only one type allowed" in unions. However, this pattern is standard in transformers for flexible inputs (e.g., both string path and dict configs).

This patch modifies HfArgumentParser._parse_dataclass_field() to detect this specific case and allow it.

Fixes

Closes a CLI parsing limitation for common use cases involving fields like:

deepspeed: Optional[Union[str, dict]] = field(default=None, ...)

@Rocketknight1
Copy link
Copy Markdown
Member

Makes sense, but please check the CI! It seems like a lot of tests are broken, so you might have to iterate on this to get it to pass. Also, you can resolve the quality issues with pip install -e .[quality] followed by make fixup.

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