order was documented as single-valued ("energy_desc" only, placeholder for a
future alternative ordering) but validate_config only checked its siblings
history/teacher_forcing, so e.g. order = "energy_asc" was silently accepted
and trained as if it were energy_desc. Add the missing check alongside the
other two, gated the same way (only meaningful under
stage2_model.decoder = "autoregressive"). Also updates the stale reason
string on the pre-existing _KNOWN_UNUSED allow-list entry for this key in
tests/test_config_consumed_keys.py, since half of it ("validate_config ...
never [checks] order") is no longer true after this fix — the key stays
allow-listed because validate_config itself isn't in that test's
build/train/rollout consumer whitelist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
build_critics always sized a WGAN critic off the generator's own
hidden_dim/n_res_blocks, silently discarding the documented 0=inherit
sentinel on stage{1,2}_model.wgan.critic_hidden_dim/critic_n_res_blocks
(the same convention critic_lr already honoured). Now both keys are read
with the 0 -> inherit fallback, and stage-scoped-only CLI flags
(--stage{1,2}-critic-hidden-dim/--stage{1,2}-critic-n-res-blocks) are
added -- no shared alias, since critic sizing is an architectural
per-stage knob like --hidden-dim/--n-res-blocks, not a shared training
hyperparameter like --n-critic/--gp-weight/--noise-dim/--critic-lr.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
validate_config_keys only checks that a config key is declared in
DEFAULT_CONFIG, never that anything reads it — the gap that let Issues 1, 2
and 4's dead keys (stage1_context, wgan.critic_hidden_dim/critic_n_res_blocks,
autoregressive.order) slip through silently. tests/test_config_consumed_keys.py
walks every DEFAULT_CONFIG leaf path and asserts each is either found (via AST
scan for attribute access, dict-key-shaped string constants, or constructor/
function parameter names — the last needed because Router subclasses receive
their config via **kwargs filtered by signature) in a fixed whitelist of
build/train/rollout consumer files, or explicitly recorded in _KNOWN_UNUSED
with a reason. A second test asserts the allow-list has no stale entries, so
fixing Issue 1/2/4 will force removal of the corresponding allow-list line
rather than let it silently outlive the bug.
The whitelist is intentionally narrower than "anywhere in giant/": scanning
the whole package produces false negatives from unrelated identifier
collisions (e.g. router_gating.py's unrelated `order` parameter would make
autoregressive.order read as consumed).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lars