Support the --accelerate_config=<name> equals-sign CLI syntax - #6953
Closed
amanyagami wants to merge 1 commit into
Closed
Support the --accelerate_config=<name> equals-sign CLI syntax#6953amanyagami wants to merge 1 commit into
amanyagami wants to merge 1 commit into
Conversation
resolve_accelerate_config_argument() only recognized the two-token form (--accelerate_config single_gpu). The equals-sign form (--accelerate_config=single_gpu), which argparse-style CLIs commonly support, was left untouched in the launch args and passed straight through to accelerate, which doesn't understand --accelerate_config at all, silently ignoring the requested config instead of resolving it to --config_file. Normalize the =-joined form into the two-token form up front so the rest of the function only has to handle a single case; both syntaxes now resolve identically. Fixes huggingface#6874
Contributor
|
Correction to my earlier overlap note: the earliest implementation is #6875, opened on August 23 for #6874. It already covers both CLI spellings, empty-value validation, trailing arguments, and CLI integration. Linking the canonical earlier PR so the implementations can be consolidated rather than reviewed independently. |
Author
|
Closing in favor of #6875, which was opened earlier (Aug 23) and covers the same fix (both CLI spellings, empty-value validation, trailing arguments, CLI integration tests). Thanks @DaoyuanLi2816 for the pointer — apologies for the duplicate effort. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
trl.cli.accelerate_config.resolve_accelerate_config_argument()is responsible for turning--accelerate_config <name_or_path>on thetrl <command>CLI into the--config_file <path>argument that getsforwarded to
accelerate launch. It only recognized the two-token form(
--accelerate_config single_gpu): it looks up"--accelerate_config"as a standalone element oflaunch_argswith
launch_args.index("--accelerate_config")and reads the following element as the value.The equals-sign form that argparse-style CLIs commonly support,
--accelerate_config=single_gpu, doesn't matchthat lookup at all. Because
"--accelerate_config" not in launch_argsis true in that case, the function returnslaunch_argsunchanged,--accelerate_config=single_gpugets passed straight through toaccelerate launch(which has no idea what to do with it) instead of being resolved to
--config_file .../single_gpu.yaml, and therequested config is silently ignored.
Root cause
resolve_accelerate_config_argument()assumed--accelerate_configand its value are always two separate argvelements, with no handling for the
--flag=valueform.Fix
Before doing the existing lookup, scan
launch_argsfor an element starting with"--accelerate_config=", and iffound, split it into the two-token form (
["--accelerate_config", "<value>"]) and rebuildlaunch_argswith thatsubstitution. The rest of the function is untouched and now only ever has to deal with the two-token form, so both
syntaxes resolve identically (same
--config_file <path>output and remaining args preserved).Tests
Added
tests/test_cli_accelerate_config.pywith regression tests for:--accelerate_config=<name>form resolves correctly,--accelerate_configabsent from args is a no-op,--accelerate_configstill raisesValueError,ValueError, for both syntaxes.I confirmed these tests fail on
main(3 of the 6 fail: the equals-form test, the equivalence test, and theinvalid-name-for-equals-form test) and pass after the fix.
Test output (after fix)
Also ran the full existing CLI test suite to check for regressions:
pre-commit run --files trl/cli/accelerate_config.py tests/test_cli_accelerate_config.pypasses (ruff check, ruffformat, doc-builder style check all green).
Fixes #6874
Before submitting
AI writing disclosure
We welcome the use of AI tools to help with contributions. For transparency and to help us improve our review process, please indicate the level of AI involvement in this PR.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.
Note
Low Risk
Small, localized CLI argument parsing change with dedicated tests; no auth, data, or training logic touched.
Overview
Fixes a bug where
--accelerate_config=single_gpuwas passed through toaccelerate launchunchanged instead of being rewritten to--config_filewith the bundled YAML path.resolve_accelerate_config_argumentnow normalizes any--accelerate_config=<value>argv token into the existing two-token form before lookup, so both CLI styles behave the same and the rest of the resolver is unchanged.Adds
tests/test_cli_accelerate_config.pywith regression coverage for both forms, equivalence, no-op when the flag is absent, missing values, and invalid config names (including equals form).Reviewed by Cursor Bugbot for commit deb6ae0. Bugbot is set up for automated code reviews on this repo. Configure here.