feat(config): warn on CLI overrides introducing unknown top-level keys - #2979
Open
n-dlms wants to merge 1 commit into
Open
feat(config): warn on CLI overrides introducing unknown top-level keys#2979n-dlms wants to merge 1 commit into
n-dlms wants to merge 1 commit into
Conversation
When a CLI override (e.g. 'foobbar=1') introduces a top-level key that is not present in the YAML config, the user has likely either misspelled a YAML key or passed a kwarg the recipe does not support. Previously the merge happened silently and the user would believe the kwarg was active when in fact the recipe ignored it. This commit adds a post-merge warning that lists each unknown top-level key and surfaces close YAML-key candidates via a simple difflib + substring heuristic in _closest_yaml_keys. The warning is emitted through log_rank_zero so it only appears on rank zero. Added tests in tests/torchtune/config/test_config_utils.py: - test_merge_warns_on_unused_cli_key verifies the warning fires for unknown top-level keys and does not include existing keys. - test_merge_no_warning_when_all_keys_known verifies no warning is emitted when all CLI overrides correspond to YAML keys. - test_closest_yaml_keys verifies the heuristic surfaces typo matches, returns empty list for wildly different keys, and respects max_suggestions. Fixes meta-pytorch#1646
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.
Summary
When a CLI override (e.g.
foobbar=1) introduces a top-level key that is not present in the YAML config, the user has likely either misspelled a YAML key or passed a kwarg the recipe does not support. Previously the merge happened silently viaOmegaConf.merge(yaml_conf, cli_conf)and the user would believe the kwarg was active when the recipe ignored it.This commit adds a post-merge warning that lists each unknown top-level key and surfaces close YAML-key candidates via a simple
difflib+ substring heuristic in_closest_yaml_keys. The warning is emitted throughlog_rank_zeroso it appears only on rank zero.What changed
torchtune/config/_utils.py_merge_yaml_and_cli_args: collects top-level keys from each CLI override, tracks ones not present in the YAML top-level keys, and emits a single aggregated warning vialog_rank_zerobefore the merge._closest_yaml_keys(new helper):difflib.SequenceMatcherratio + substring containment reward, returns up tomax_suggestions(default 3) YAML keys that look similar to the offending key.tests/torchtune/config/test_config_utils.pytest_merge_warns_on_unused_cli_key: two unknown keys both surface in the warning; existing keys do not.test_merge_no_warning_when_all_keys_known: clean override set produces no warning output.test_closest_yaml_keys: typo "batch_siz" matches "batch_size"; unrelated keys return empty list;max_suggestionsis respected.Why this is the right place
The merge happens in
_merge_yaml_and_cli_args(called fromTuneRecipeArgumentParser.parse_known_args). By the time we reach the merge, the YAML keys are known (vars(yaml_args)) and the CLI dotlist is fully parsed. Tracking top-level keys during the CLI loop avoids any extra pass and adds zero overhead when no warning fires.The implementation intentionally uses
log_rank_zero(notlogger.warning) because:_merge_yaml_and_cli_argson every rank; emitting the warning on all ranks would spam logs.log_configuses the same helper for the same reason (issue Log config to output only on rank zero #2700).Limitations
model.lora_rankwhenmodel._component_exists in YAML) are not flagged because they would produce too many false positives during early recipe prototyping.difflibheuristic is intentionally simple; it is a did-you-mean hint, not a guaranteed typo detector. False positives are possible; the warning copy acknowledges this: "If this was intentional ... you can ignore this warning".Verification
python3 -m py_compile torchtune/config/_utils.py tests/torchtune/config/test_config_utils.pypassespython3 -m ruff check torchtune/config/_utils.py tests/torchtune/config/test_config_utils.pypasses_closest_yaml_keysstandalone validation (without torchao import) produces expected outputs:closest_yaml_keys("batch_siz", {batch_size,epochs,batch,optimizer,lr_scheduler})->["batch_size", "batch"]closest_yaml_keys("zzz", {batch_size,epochs})->[]closest_yaml_keys("batch", {batch_size,batch,optimizer}, max_suggestions=1)->["batch"]torchaodependency (per issue install instructions); new tests follow the existing pattern of mockingdist.is_availableandget_logger.Backward compatibility
get_logger("WARNING")and routed throughlog_rank_zero; users who have configured their root logger to ERROR will not see it.Fixes #1646