Add unknown-key validation to config.toml merge (issues.md Issue 2)
CI / Lint (ruff check) (push) Successful in 30s
CI / Format (ruff format) (push) Successful in 29s
CI / Sync project version with tag (push) Has been skipped
CI / Lint (ruff check) (pull_request) Successful in 32s
CI / Type check (ty) (push) Successful in 34s
CI / Format (ruff format) (pull_request) Successful in 34s
CI / Sync project version with tag (pull_request) Has been skipped
CI / Type check (ty) (pull_request) Successful in 35s
CI / Tests (pull_request) Successful in 3m30s
CI / Tests (push) Successful in 3m42s
CI / Lint (ruff check) (push) Successful in 30s
CI / Format (ruff format) (push) Successful in 29s
CI / Sync project version with tag (push) Has been skipped
CI / Lint (ruff check) (pull_request) Successful in 32s
CI / Type check (ty) (push) Successful in 34s
CI / Format (ruff format) (pull_request) Successful in 34s
CI / Sync project version with tag (pull_request) Has been skipped
CI / Type check (ty) (pull_request) Successful in 35s
CI / Tests (pull_request) Successful in 3m30s
CI / Tests (push) Successful in 3m42s
A typo like `n_res_block` for `n_res_blocks` previously merged cleanly, passed validate_config, and silently trained a model that didn't match config.toml's documented settings. merge_cli_overrides now rejects any key not present in DEFAULT_CONFIG's schema via validate_config_keys, with a did-you-mean suggestion, while still allowing the genuinely dynamic composed-router axis keys and centers_init. Checkpoint model_config loading is untouched, so old checkpoints keep loading. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
+59
-1
@@ -1,6 +1,8 @@
|
||||
import copy
|
||||
import difflib
|
||||
import hashlib
|
||||
import random
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
import tomllib
|
||||
@@ -1045,6 +1047,59 @@ def migrate_config(cfg: dict) -> dict:
|
||||
return new
|
||||
|
||||
|
||||
# axis{i}_{field} composed-router keys (see network._parse_composed_axes) — field-name
|
||||
# agnostic, matching network.py's own _AXIS_KEY_RE, since anything after axis{i}_ is
|
||||
# passed straight through as a router kwarg there.
|
||||
_AXIS_KEY_RE = re.compile(r"^axis\d+_.+$")
|
||||
|
||||
# Paths (dotted, relative to the merged cfg root) that carry genuinely dynamic keys not
|
||||
# in DEFAULT_CONFIG's fixed schema — composed-router axis{i}_{field} flags and
|
||||
# pipeline.seed_router_centers's runtime-seeded centers_init (see RouterConfig.extra
|
||||
# above). validate_config_keys allows any key under these paths through unconditionally
|
||||
# apart from the axis-pattern/centers_init check below.
|
||||
_DYNAMIC_ROUTER_PATHS = {"stage1_model.router", "stage2_model.router"}
|
||||
|
||||
|
||||
def _unknown_key_error(full_path: str, key: str, valid_keys) -> ValueError:
|
||||
hint = difflib.get_close_matches(key, list(valid_keys), n=1)
|
||||
suggestion = f" — did you mean {hint[0]!r}?" if hint else ""
|
||||
return ValueError(f"unknown config key {full_path!r}{suggestion}")
|
||||
|
||||
|
||||
def _validate_keys(node: dict, default_node: dict, path: str) -> None:
|
||||
for key, value in node.items():
|
||||
if path == "" and key == "meta":
|
||||
continue
|
||||
full_path = f"{path}.{key}" if path else key
|
||||
if path in _DYNAMIC_ROUTER_PATHS and (key == "centers_init" or _AXIS_KEY_RE.match(key)):
|
||||
continue
|
||||
if key not in default_node:
|
||||
raise _unknown_key_error(full_path, key, default_node.keys())
|
||||
if isinstance(value, dict) and isinstance(default_node[key], dict):
|
||||
_validate_keys(value, default_node[key], full_path)
|
||||
|
||||
|
||||
def validate_config_keys(cfg: dict) -> None:
|
||||
"""Reject any config key not part of the known v0.3 schema (DEFAULT_CONFIG's tree).
|
||||
|
||||
Catches typos like `n_res_block` for `n_res_blocks` that would otherwise merge
|
||||
cleanly, pass `validate_config`, and silently build the wrong model — see
|
||||
issues.md Issue 2.
|
||||
|
||||
Only exercised on the config.toml/CLI-overrides path (called from
|
||||
`merge_cli_overrides` below). A checkpoint's `model_config` dict goes through
|
||||
`network._migrate_legacy_model_config`/`build_models` instead and must keep
|
||||
loading regardless of schema drift; old checkpoints predate this validator and
|
||||
are never passed through here.
|
||||
|
||||
Two areas are deliberately dynamic and excluded: `[meta]` (run provenance, no
|
||||
DEFAULT_CONFIG counterpart), and `stage{1,2}_model.router`'s `axis{i}_{field}`
|
||||
keys (composed-router axes, see `network._parse_composed_axes`) /
|
||||
`centers_init` (runtime-seeded by `pipeline.seed_router_centers`).
|
||||
"""
|
||||
_validate_keys(cfg, DEFAULT_CONFIG, "")
|
||||
|
||||
|
||||
def merge_cli_overrides(
|
||||
defaults: dict,
|
||||
config_path: Path | None,
|
||||
@@ -1057,7 +1112,9 @@ def merge_cli_overrides(
|
||||
deep-merge (see `_deep_merge`) — the shape stage-prefixed CLI flags
|
||||
naturally produce. A v0.2-shaped TOML file is transparently migrated
|
||||
(`migrate_config`) before merging, so old configs on disk keep working
|
||||
under the new schema.
|
||||
under the new schema. The result is checked against the known schema
|
||||
(`validate_config_keys`) before being returned, so a typo'd key raises
|
||||
here rather than silently building the wrong model.
|
||||
"""
|
||||
cfg = copy.deepcopy(defaults)
|
||||
if config_path is not None:
|
||||
@@ -1075,6 +1132,7 @@ def merge_cli_overrides(
|
||||
cfg[section] = _deep_merge(cfg.get(section, {}), values)
|
||||
else:
|
||||
cfg[section] = values
|
||||
validate_config_keys(cfg)
|
||||
return cfg
|
||||
|
||||
|
||||
|
||||
@@ -798,6 +798,97 @@ def test_validate_config_ar_checks_skipped_under_one_shot():
|
||||
gconfig.validate_config(cfg) # must not raise
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# validate_config_keys / merge_cli_overrides unknown-key rejection
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_validate_config_keys_default_config_passes():
|
||||
gconfig.validate_config_keys(gconfig.DEFAULT_CONFIG) # must not raise
|
||||
|
||||
|
||||
def test_validate_config_keys_rejects_unknown_top_level_key():
|
||||
cfg = _cfg_with(**{"bogus_section.foo": 1})
|
||||
try:
|
||||
gconfig.validate_config_keys(cfg)
|
||||
assert False, "expected ValueError"
|
||||
except ValueError as e:
|
||||
assert "bogus_section" in str(e)
|
||||
|
||||
|
||||
def test_validate_config_keys_rejects_unknown_nested_key_with_close_match_hint():
|
||||
cfg = _cfg_with(**{"stage1_model.n_res_block": 12})
|
||||
try:
|
||||
gconfig.validate_config_keys(cfg)
|
||||
assert False, "expected ValueError"
|
||||
except ValueError as e:
|
||||
assert "stage1_model.n_res_block" in str(e)
|
||||
assert "n_res_blocks" in str(e)
|
||||
|
||||
|
||||
def test_validate_config_keys_allows_composed_router_axis_keys():
|
||||
cfg = _cfg_with(
|
||||
**{
|
||||
"stage1_model.router.enabled": True,
|
||||
"stage1_model.router.type": "composed",
|
||||
"stage1_model.router.axis0_type": "energy",
|
||||
"stage1_model.router.axis0_n_experts": 4,
|
||||
"stage1_model.router.axis1_type": "pdg",
|
||||
"stage1_model.router.axis1_emb_dim": 8,
|
||||
}
|
||||
)
|
||||
gconfig.validate_config_keys(cfg) # must not raise
|
||||
|
||||
|
||||
def test_validate_config_keys_allows_centers_init():
|
||||
cfg = _cfg_with(**{"stage1_model.router.centers_init": [-1.0, 0.0, 1.0]})
|
||||
gconfig.validate_config_keys(cfg) # must not raise
|
||||
|
||||
|
||||
def test_validate_config_keys_rejects_unrelated_unknown_router_key():
|
||||
cfg = _cfg_with(**{"stage1_model.router.n_expert": 4}) # typo for n_experts
|
||||
try:
|
||||
gconfig.validate_config_keys(cfg)
|
||||
assert False, "expected ValueError"
|
||||
except ValueError as e:
|
||||
assert "stage1_model.router.n_expert" in str(e)
|
||||
assert "n_experts" in str(e)
|
||||
|
||||
|
||||
def test_validate_config_keys_skips_meta_section():
|
||||
cfg = _cfg_with()
|
||||
cfg["meta"] = {"config_version": 3, "git_hash": "abc123"}
|
||||
gconfig.validate_config_keys(cfg) # must not raise
|
||||
|
||||
|
||||
def test_merge_cli_overrides_rejects_typo_in_toml_file(tmp_path):
|
||||
path = tmp_path / "config.toml"
|
||||
path.write_text("[meta]\nconfig_version = 3\n\n[stage1_model]\nn_res_block = 12\n")
|
||||
try:
|
||||
gconfig.merge_cli_overrides(gconfig.DEFAULT_CONFIG, path, {})
|
||||
assert False, "expected ValueError"
|
||||
except ValueError as e:
|
||||
assert "n_res_block" in str(e)
|
||||
|
||||
|
||||
def test_merge_cli_overrides_rejects_typo_in_cli_overrides():
|
||||
try:
|
||||
gconfig.merge_cli_overrides(
|
||||
gconfig.DEFAULT_CONFIG,
|
||||
None,
|
||||
{"stage1_model": {"n_res_block": 12}},
|
||||
)
|
||||
assert False, "expected ValueError"
|
||||
except ValueError as e:
|
||||
assert "n_res_block" in str(e)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("fixture_name", ["default.toml", "wgan_h128_b4_physical.toml"])
|
||||
def test_merge_cli_overrides_real_config_fixtures_pass_key_validation(fixture_name, monkeypatch):
|
||||
monkeypatch.setattr(gconfig, "git_hash", lambda: "c3bf3abebfe29a10fe42b9cbafbb3460ab78d243")
|
||||
gconfig.merge_cli_overrides(gconfig.DEFAULT_CONFIG, _CONFIGS_DIR / fixture_name, {}) # must not raise
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# checkpoint config-mismatch warnings (unchanged surface, still exercised)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user