From c3fc768b40e8355c483d5bca73b5e04ec2d03028 Mon Sep 17 00:00:00 2001 From: Lars Bogner Date: Thu, 13 Aug 2026 14:57:21 +0200 Subject: [PATCH] Reject stage2_model.stage1_context = 'sampled' as unimplemented (issues.md Issue 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit trainers.py unconditionally trains stage 2 against the ground-truth stage-1 output (stage1_ctx = x1_s1.detach()), but 'sampled' was accepted by validate_config, stored in config.toml and the checkpoint's model_config, and silently trained identically to 'truth' — mislabeling every downstream artifact for a run launched with --stage2-stage1-context sampled. Mirrors the existing stop_token validate_config pattern. User chose the immediate fix (reject loudly) over the proper fix (actually implement sampled context), which is scoped to Issue 16. Also updates the _KNOWN_UNUSED reason for stage2_model.stage1_context (added by Issue 5's consumed-keys audit) to reflect that the value is now rejected rather than silently accepted, and drops the now-invalid --stage2-stage1-context sampled case from test_stage2_only_knobs (a full CLI invocation) — that flag's plumbing is still covered at the overrides-dict level by test_overrides_from_flags_stage2_only_knobs. Co-Authored-By: Claude Opus 5 --- giant/config.py | 8 ++++++++ tests/test_cli_train_overrides.py | 7 ++++--- tests/test_config.py | 9 +++++++++ tests/test_config_consumed_keys.py | 6 ++++-- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/giant/config.py b/giant/config.py index fecfd72..b022796 100644 --- a/giant/config.py +++ b/giant/config.py @@ -1256,6 +1256,14 @@ def validate_config(cfg: dict) -> None: "(standalone stage-2 evaluation only, never for rollout)" ) + if _get_path(cfg, "stage2_model.stage1_context") == "sampled": + raise ValueError( + "stage2_model.stage1_context = 'sampled' is accepted by the schema " + "but not implemented — trainers.py always trains stage 2 against " + "the ground-truth stage-1 output; use 'truth' (default) instead " + "(see issues.md Issue 16 for the planned implementation)" + ) + if ( _get_path(cfg, "stage2_model.n_sec.mode") == "truth" and _get_path(cfg, "stage1_model.active") diff --git a/tests/test_cli_train_overrides.py b/tests/test_cli_train_overrides.py index 7df258f..4008e83 100644 --- a/tests/test_cli_train_overrides.py +++ b/tests/test_cli_train_overrides.py @@ -41,6 +41,10 @@ def test_stage_prefixed_generator_overrides_shared_mode(monkeypatch, tmp_path): def test_stage2_only_knobs(monkeypatch, tmp_path): + # --stage2-stage1-context is exercised separately at the overrides-dict + # level (test_overrides_from_flags_stage2_only_knobs in test_config.py): + # its only non-default value, "sampled", is rejected by validate_config + # (issues.md Issue 1), so it can't appear in a full CLI invocation here. cfg = _invoke_and_capture_cfg( monkeypatch, tmp_path, @@ -53,15 +57,12 @@ def test_stage2_only_knobs(monkeypatch, tmp_path): "32", "--stage2-context-dim", "16", - "--stage2-stage1-context", - "sampled", ], ) assert cfg["stage2_model"]["decoder"] == "one_shot" assert cfg["stage2_model"]["k_max"] == 8 assert cfg["stage2_model"]["hidden_dim"] == 32 assert cfg["stage2_model"]["context_dim"] == 16 - assert cfg["stage2_model"]["stage1_context"] == "sampled" # untouched stage1 defaults assert cfg["stage1_model"]["hidden_dim"] == 256 diff --git a/tests/test_config.py b/tests/test_config.py index 0f1411c..8e7751a 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -687,6 +687,15 @@ def test_validate_config_stop_token_not_implemented(): assert "stop_token" in str(e) +def test_validate_config_stage1_context_sampled_not_implemented(): + cfg = _cfg_with(**{"stage2_model.stage1_context": "sampled"}) + try: + gconfig.validate_config(cfg) + assert False, "expected ValueError" + except ValueError as e: + assert "sampled" in str(e) + + def test_validate_config_n_sec_truth_rejected_for_rollout_capable_checkpoint(): """'n_sec.mode = "truth" is invalid for a rollout-capable checkpoint' — both stages active means giant rollout diff --git a/tests/test_config_consumed_keys.py b/tests/test_config_consumed_keys.py index 0a0f068..05527bf 100644 --- a/tests/test_config_consumed_keys.py +++ b/tests/test_config_consumed_keys.py @@ -55,8 +55,10 @@ _EXCLUDED_FILES = ("giant/model/_legacy.py",) _KNOWN_UNUSED = { "stage2_model.stage1_context": ( "issues.md Issue 1 — trainers.py hardcodes stage1_ctx to the " - "ground-truth stage-1 output; 'sampled' is accepted and stored but " - "never read" + "ground-truth stage-1 output; 'sampled' is now rejected loudly by " + "validate_config (not silently accepted), but the key still isn't " + "read by any build/train consumer file since only 'truth' can pass " + "validation — see Issue 16 for the real implementation" ), "stage1_model.wgan.critic_hidden_dim": ( "issues.md Issue 2 — build_critics always sizes the critic off the generator's own hidden_dim, never this key"