diff --git a/tests/torchtune/config/test_config_utils.py b/tests/torchtune/config/test_config_utils.py index e8b7ecf1e7..6289d4038b 100644 --- a/tests/torchtune/config/test_config_utils.py +++ b/tests/torchtune/config/test_config_utils.py @@ -242,3 +242,94 @@ def test_remove_key_by_dotpath(self): cfg = copy.deepcopy(_CONFIG) with pytest.raises(KeyError, match="'i'"): _remove_key_by_dotpath(cfg, "i") + + @mock.patch( + "torchtune.config._parse.OmegaConf.load", return_value=OmegaConf.create(_CONFIG) + ) + def test_merge_warns_on_unused_cli_key(self, mock_load, capsys): + parser = TuneRecipeArgumentParser("test parser") + yaml_args, cli_args = parser.parse_known_args( + [ + "--config", + "test.yaml", + "d=6", # Override existing key — should NOT warn + "foo_bar=1", # Typo or unsupported key — SHOULD warn + "foobbar=2", # Another unknown key similar to "foobar" + ] + ) + + logger = logging.getLogger(__name__) + logger.setLevel("DEBUG") + stream = StringIO() + handler = logging.StreamHandler(stream) + logger.addHandler(handler) + try: + with ( + mock.patch( + "torchtune.config._utils.get_logger", return_value=logger + ), + mock.patch( + "torchtune.utils._logging.dist.is_available", return_value=False + ), + ): + _merge_yaml_and_cli_args(yaml_args, cli_args) + output = stream.getvalue() + # The unknown keys should both be reported + assert "foo_bar" in output, ( + f"Expected 'foo_bar' in warning, got: {output!r}" + ) + assert "foobbar" in output, ( + f"Expected 'foobbar' in warning, got: {output!r}" + ) + # Existing key 'd' should not appear in the unused list + assert "'d' (did you mean" not in output + finally: + logger.removeHandler(handler) + + @mock.patch( + "torchtune.config._parse.OmegaConf.load", return_value=OmegaConf.create(_CONFIG) + ) + def test_merge_no_warning_when_all_keys_known(self, mock_load, capsys): + parser = TuneRecipeArgumentParser("test parser") + yaml_args, cli_args = parser.parse_known_args( + [ + "--config", + "test.yaml", + "a=2", # override existing + "d=6", # override existing + ] + ) + logger = logging.getLogger(__name__) + logger.setLevel("DEBUG") + stream = StringIO() + handler = logging.StreamHandler(stream) + logger.addHandler(handler) + try: + with ( + mock.patch( + "torchtune.config._utils.get_logger", return_value=logger + ), + mock.patch( + "torchtune.utils._logging.dist.is_available", return_value=False + ), + ): + _merge_yaml_and_cli_args(yaml_args, cli_args) + assert not stream.getvalue().strip(), ( + f"Expected no warning output, got: {stream.getvalue()!r}" + ) + finally: + logger.removeHandler(handler) + + def test_closest_yaml_keys(self): + from torchtune.config._utils import _closest_yaml_keys + + yaml_keys = {"batch_size", "epochs", "batch", "optimizer", "lr_scheduler"} + # exact-ish typo: 'batch_siz' should match 'batch_size' (and maybe 'batch') + matches = _closest_yaml_keys("batch_siz", yaml_keys) + assert "batch_size" in matches, f"Expected 'batch_size' in {matches}" + + # No matches for a wildly different key + assert _closest_yaml_keys("zzz", yaml_keys) == [] + + # max_suggestions is respected + assert len(_closest_yaml_keys("batch", yaml_keys, max_suggestions=1)) <= 1 diff --git a/torchtune/config/_utils.py b/torchtune/config/_utils.py index 734f073e05..a9deb319dd 100644 --- a/torchtune/config/_utils.py +++ b/torchtune/config/_utils.py @@ -155,6 +155,8 @@ def _merge_yaml_and_cli_args(yaml_args: Namespace, cli_args: list[str]) -> DictC """ # Convert Namespace to simple dict yaml_kwargs = vars(yaml_args) + yaml_top_level_keys = set(yaml_kwargs.keys()) + unused_keys: list[str] = [] cli_dotlist = [] for arg in cli_args: # If CLI override uses the remove flag (~), remove the key from the yaml config @@ -183,6 +185,13 @@ def _merge_yaml_and_cli_args(yaml_args: Namespace, cli_args: list[str]) -> DictC if k in yaml_kwargs and _has_component(yaml_kwargs[k]): k += "._component_" + # Track CLI overrides that introduce a new top-level key not defined in the + # YAML config. These are likely typos or recipe-unsupported kwargs and should + # be surfaced to the user as a warning (see issue #1646). + top_level_key = k.split(".")[0] + if top_level_key not in yaml_top_level_keys and top_level_key not in unused_keys: + unused_keys.append(top_level_key) + # None passed via CLI will be parsed as string, but we really want OmegaConf null if v == "None": v = "!!null" @@ -194,6 +203,24 @@ def _merge_yaml_and_cli_args(yaml_args: Namespace, cli_args: list[str]) -> DictC v = "!!str " + v cli_dotlist.append(f"{k}={v}") + if unused_keys: + logger = get_logger("WARNING") + suggestions = ", ".join( + f"'{key}' (did you mean one of: " + f"{', '.join(_closest_yaml_keys(key, yaml_top_level_keys)) or 'no matches'}?)" + for key in unused_keys + ) + log_rank_zero( + logger=logger, + msg=( + "The following CLI override key(s) are not present in the YAML config " + f"and may be unused by the recipe: {suggestions}. If this was " + "intentional (e.g. adding a brand new field), you can ignore this " + "warning. Otherwise, please check the key name against the config and " + "recipe for a typo or unsupported option." + ), + ) + # Merge the args cli_conf = OmegaConf.from_dotlist(cli_dotlist) yaml_conf = OmegaConf.create(yaml_kwargs) @@ -202,6 +229,23 @@ def _merge_yaml_and_cli_args(yaml_args: Namespace, cli_args: list[str]) -> DictC return OmegaConf.merge(yaml_conf, cli_conf) +def _closest_yaml_keys(key: str, yaml_keys: set[str], max_suggestions: int = 3) -> list[str]: + """Return up to ``max_suggestions`` keys from ``yaml_keys`` that look similar to + ``key``. Uses a simple substring + difflib heuristic to surface likely typos.""" + import difflib + + candidates: list[tuple[float, str]] = [] + for yk in yaml_keys: + score = difflib.SequenceMatcher(None, key, yk).ratio() + # also reward substring containment so prefix typos rank high + if key in yk or yk in key: + score = max(score, 0.75) + if score >= 0.6: + candidates.append((score, yk)) + candidates.sort(reverse=True) + return [yk for _, yk in candidates[:max_suggestions]] + + def _remove_key_by_dotpath(nested_dict: dict[str, Any], dotpath: str) -> None: """ Removes a key specified by dotpath from a nested dict. Errors should handled by