Extract giant train/new-run's CLI override mapping into a table-driven function (issues.md Issues 3 & 4)

train()'s ~140-line hand-written flag->config translation (three different ad hoc "more specific flag wins" patterns) and new_run()'s near-verbatim copy are replaced by a shared FlagSpec/FLAG_SPECS table and overrides_from_flags() in config.py, reused by both commands. This makes the override/precedence logic directly unit-testable without CliRunner, closing coverage gaps that had zero tests (e.g. --emb-dim/--conditioning dual-axis fan-out, three of four WGAN knob legs, --stage2-generator overriding --mode, router's stage1-only asymmetry). No CLI flags, help text, or precedence semantics changed -- `giant train --help`/`giant new-run --help` are byte-identical before and after, and all previously-passing CliRunner tests still pass unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-08-12 15:04:49 +02:00
parent 01acbfed61
commit 2bfb1ab056
4 changed files with 381 additions and 201 deletions
@@ -94,3 +94,83 @@ def test_wgan_knobs_split_per_stage(monkeypatch, tmp_path):
    assert cfg["stage1_model"]["wgan"]["gp_weight"] == 10.0
    assert cfg["stage2_model"]["wgan"]["n_critic"] == 5
    assert cfg["stage2_model"]["wgan"]["gp_weight"] == 2.5
+
+
+def test_batch_size_invalid_string_errors(monkeypatch, tmp_path):
+    monkeypatch.setattr(cli, "run_train_job", lambda *a, **kw: None)
+    result = runner.invoke(
+        cli.app,
+        ["train", "dummy.parquet", "--out", str(tmp_path / "run"), "--batch-size", "not-a-number"],
+    )
+    assert result.exit_code == 1
+    assert "--batch-size must be an integer or 'auto'" in result.output
+
+
+def test_out_dir_resolution_prefers_explicit_out_over_resume(monkeypatch, tmp_path):
+    captured: dict = {}
+
+    def _fake_run_train_job(*, data, cfg, out_dir, **kwargs):
+        captured["out_dir"] = out_dir
+
+    monkeypatch.setattr(cli, "run_train_job", _fake_run_train_job)
+
+    resume_dir = tmp_path / "resumed_run"
+    resume_dir.mkdir()
+    (resume_dir / "last.pt").touch()
+    explicit_out = tmp_path / "explicit_run"
+
+    result = runner.invoke(
+        cli.app,
+        ["train", "dummy.parquet", "--out", str(explicit_out), "--resume", str(resume_dir / "last.pt")],
+    )
+    assert result.exit_code == 0, result.output
+    assert captured["out_dir"] == explicit_out
+
+
+def test_out_dir_resolution_falls_back_to_resume_parent(monkeypatch, tmp_path):
+    captured: dict = {}
+
+    def _fake_run_train_job(*, data, cfg, out_dir, **kwargs):
+        captured["out_dir"] = out_dir
+
+    monkeypatch.setattr(cli, "run_train_job", _fake_run_train_job)
+
+    resume_dir = tmp_path / "resumed_run"
+    resume_dir.mkdir()
+    (resume_dir / "last.pt").touch()
+
+    result = runner.invoke(cli.app, ["train", "dummy.parquet", "--resume", str(resume_dir / "last.pt")])
+    assert result.exit_code == 0, result.output
+    assert captured["out_dir"] == resume_dir
+
+
+def test_out_dir_resolution_defaults_when_neither_out_nor_resume_given(monkeypatch, tmp_path):
+    captured: dict = {}
+
+    def _fake_run_train_job(*, data, cfg, out_dir, **kwargs):
+        captured["out_dir"] = out_dir
+
+    monkeypatch.setattr(cli, "run_train_job", _fake_run_train_job)
+    monkeypatch.chdir(tmp_path)
+
+    result = runner.invoke(cli.app, ["train", "dummy.parquet"])
+    assert result.exit_code == 0, result.output
+    assert captured["out_dir"] == Path("checkpoints") / cli.gconfig.default_out_dir_name(cli.gconfig.DEFAULT_CONFIG)
+
+
+def test_batch_size_auto_estimates_and_echoes(monkeypatch, tmp_path):
+    captured: dict = {}
+
+    def _fake_run_train_job(*, data, cfg, out_dir, num_workers, **kwargs):
+        captured["batch_size"] = cfg["train"]["batch_size"]
+
+    monkeypatch.setattr(cli, "run_train_job", _fake_run_train_job)
+    monkeypatch.setattr(cli.gconfig, "estimate_batch_size", lambda hidden_dim, n_blocks, device: 123)
+
+    result = runner.invoke(
+        cli.app,
+        ["train", "dummy.parquet", "--out", str(tmp_path / "run"), "--batch-size", "auto"],
+    )
+    assert result.exit_code == 0, result.output
+    assert captured["batch_size"] == 123
+    assert "batch_size: 123 (auto-estimated from free GPU memory)" in result.output
@@ -889,6 +889,125 @@ def test_merge_cli_overrides_real_config_fixtures_pass_key_validation(fixture_na
    gconfig.merge_cli_overrides(gconfig.DEFAULT_CONFIG, _CONFIGS_DIR / fixture_name, {})  # must not raise


+# ---------------------------------------------------------------------------
+# overrides_from_flags (issues.md Issue 3): the flag -> config-path table
+# shared by `giant train`/`giant new-run`. Each test below pins one
+# precedence rule directly, without CliRunner — see also
+# tests/test_cli_train_overrides.py for the thin end-to-end smoke coverage.
+# ---------------------------------------------------------------------------
+
+
+def test_overrides_from_flags_empty_values_yield_empty_overrides():
+    assert gconfig.overrides_from_flags({}) == {}
+    assert gconfig.overrides_from_flags({"epochs": None, "hidden_dim": None}) == {}
+
+
+def test_overrides_from_flags_train_block_passthrough():
+    overrides = gconfig.overrides_from_flags({"epochs": 5, "lr": 1e-3, "hidden_dim": None})
+    assert overrides == {"train": {"epochs": 5, "lr": 1e-3}}
+
+
+@pytest.mark.parametrize(
+    ("shorthand", "explicit", "path_key"),
+    [
+        ("hidden_dim", "stage1_hidden_dim", "hidden_dim"),
+        ("n_blocks", "stage1_n_res_blocks", "n_res_blocks"),
+        ("dropout", "stage1_dropout", "dropout"),
+    ],
+)
+def test_overrides_from_flags_stage1_explicit_overrides_shorthand(shorthand, explicit, path_key):
+    overrides = gconfig.overrides_from_flags({shorthand: 1, explicit: 2})
+    assert overrides["stage1_model"][path_key] == 2
+
+
+@pytest.mark.parametrize(
+    ("shorthand", "path_key"),
+    [("hidden_dim", "hidden_dim"), ("n_blocks", "n_res_blocks"), ("dropout", "dropout")],
+)
+def test_overrides_from_flags_stage1_shorthand_alone(shorthand, path_key):
+    overrides = gconfig.overrides_from_flags({shorthand: 7})
+    assert overrides["stage1_model"][path_key] == 7
+
+
+def test_overrides_from_flags_stage2_only_knobs():
+    overrides = gconfig.overrides_from_flags(
+        {
+            "stage2_hidden_dim": 32,
+            "stage2_n_res_blocks": 4,
+            "stage2_dropout": 0.1,
+            "stage2_decoder": "one_shot",
+            "stage2_k_max": 8,
+            "stage2_context_dim": 16,
+            "stage2_stage1_context": "sampled",
+        }
+    )
+    assert overrides["stage2_model"] == {
+        "hidden_dim": 32,
+        "n_res_blocks": 4,
+        "dropout": 0.1,
+        "decoder": "one_shot",
+        "k_max": 8,
+        "context_dim": 16,
+        "stage1_context": "sampled",
+    }
+    assert "stage1_model" not in overrides
+
+
+def test_overrides_from_flags_mode_fans_to_both_stages():
+    overrides = gconfig.overrides_from_flags({"mode": "wgan"})
+    assert overrides["stage1_model"]["generator"] == "wgan"
+    assert overrides["stage2_model"]["generator"] == "wgan"
+
+
+def test_overrides_from_flags_stage1_generator_overrides_mode_for_stage1_only():
+    overrides = gconfig.overrides_from_flags({"mode": "wgan", "stage1_generator": "flow"})
+    assert overrides["stage1_model"]["generator"] == "flow"
+    assert overrides["stage2_model"]["generator"] == "wgan"
+
+
+def test_overrides_from_flags_stage2_generator_overrides_mode_for_stage2_only():
+    overrides = gconfig.overrides_from_flags({"mode": "wgan", "stage2_generator": "flow"})
+    assert overrides["stage1_model"]["generator"] == "wgan"
+    assert overrides["stage2_model"]["generator"] == "flow"
+
+
+def test_overrides_from_flags_emb_dim_sets_both_conditioning_axes():
+    overrides = gconfig.overrides_from_flags({"emb_dim": 24})
+    assert overrides["conditioning"]["particle"]["emb_dim"] == 24
+    assert overrides["conditioning"]["material"]["emb_dim"] == 24
+
+
+def test_overrides_from_flags_conditioning_sets_both_axes_type():
+    overrides = gconfig.overrides_from_flags({"conditioning": "onehot"})
+    assert overrides["conditioning"]["particle"]["type"] == "onehot"
+    assert overrides["conditioning"]["material"]["type"] == "onehot"
+
+
+def test_overrides_from_flags_router_config_only_touches_stage1():
+    overrides = gconfig.overrides_from_flags({"router_config": {"enabled": True, "type": "energy"}})
+    assert overrides["stage1_model"]["router"] == {"enabled": True, "type": "energy"}
+    assert "stage2_model" not in overrides
+
+
+@pytest.mark.parametrize(
+    ("shared", "stage1_specific", "stage2_specific", "path_key"),
+    [
+        ("n_critic", "stage1_n_critic", "stage2_n_critic", "n_critic"),
+        ("gp_weight", "stage1_gp_weight", "stage2_gp_weight", "gp_weight"),
+        ("noise_dim", "stage1_noise_dim", "stage2_noise_dim", "noise_dim"),
+        ("critic_lr", "stage1_critic_lr", "stage2_critic_lr", "critic_lr"),
+    ],
+)
+def test_overrides_from_flags_wgan_knobs_split_per_stage(shared, stage1_specific, stage2_specific, path_key):
+    overrides = gconfig.overrides_from_flags({shared: 5.0, stage1_specific: 3.0})
+    assert overrides["stage1_model"]["wgan"][path_key] == 3.0
+    assert overrides["stage2_model"]["wgan"][path_key] == 5.0
+
+    overrides = gconfig.overrides_from_flags({shared: 5.0, stage2_specific: 2.5})
+    assert overrides["stage1_model"]["wgan"][path_key] == 5.0
+    assert overrides["stage2_model"]["wgan"][path_key] == 2.5
+
+
 # ---------------------------------------------------------------------------
 # checkpoint config-mismatch warnings (unchanged surface, still exercised)
 # ---------------------------------------------------------------------------