Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions tests/torchtune/config/test_config_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
44 changes: 44 additions & 0 deletions torchtune/config/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -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)
Expand All @@ -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
Expand Down