From 6a21c3b908cbb86e14e774b154a2d8993718f7a4 Mon Sep 17 00:00:00 2001 From: Lars Bogner Date: Wed, 12 Aug 2026 15:05:33 +0200 Subject: [PATCH] Mark issues.md Issues 3 & 4 as fixed Records what commit 2bfb1ab actually changed and its scope, matching the status-blockquote convention already used for Issues 1 and 2. Co-Authored-By: Claude Sonnet 5 --- issues.md | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/issues.md b/issues.md index abdcd5d..7684edf 100644 --- a/issues.md +++ b/issues.md @@ -42,8 +42,8 @@ architecture matrix grows, not about rot or breakage. |---|---|---|---|---| | 1 | Config defaults are declared twice; `DEFAULT_CONFIG` and consumers already disagree | **High** | Medium | **Fixed** (`9bf5874`) | | 2 | No unknown-key validation — a typo in `config.toml` silently trains the wrong model | **High** | Small | **Fixed** | -| 3 | `cli.py:train()` is a 58-parameter, 510-line fat controller | **High** | Medium | Open | -| 4 | `cli.py` is at 35.8 % coverage and holds untested override-precedence logic | **High** | Medium | Open | +| 3 | `cli.py:train()` is a 58-parameter, 510-line fat controller | **High** | Medium | **Fixed** (`2bfb1ab`) | +| 4 | `cli.py` is at 35.8 % coverage and holds untested override-precedence logic | **High** | Medium | **Fixed** (`2bfb1ab`, partial — see status note) | | 5 | Inference bootstrap is duplicated verbatim between `predict` and `rollout` | **High** | Small | Open | | 6 | Two independent v0.2→v0.3 migration surfaces encode the same knowledge | Medium | Medium | Open | | 7 | Positional tuple contracts (9-tuple, 7-tuple) between data, model and training layers | Medium | Small | Open | @@ -373,6 +373,26 @@ rollout` against every model trained so far. ## Issue 3 — `cli.py:train()` is a 58-parameter, 510-line fat controller +> **Status: Fixed, commit `2bfb1ab` on `v0.3.0-stage2-autoregressive`.** `giant/config.py` +> now declares `FlagSpec` (a frozen dataclass: `name`, `paths` — one or more dotted config +> paths, `precedence`) and a `FLAG_SPECS` table covering every flag `train`/`new-run` map +> into the config tree, plus `overrides_from_flags(values: dict[str, object]) -> dict`, +> which applies specs in ascending precedence order (so a more-specific flag overwrites a +> shared/shorthand one written earlier at the same path) — one mechanism replacing the +> three different ad hoc "more specific wins" patterns identified below +> (`.update()`-call-order, `setdefault(...)[...] =` overwrite-order, and +> `{**shared, **specific}` merge). `train()`'s ~140-line override-building block +> (`cli.py:656-790` pre-fix) is now a single flat `flag_values` dict (mostly enum `.value` +> unwrapping) plus one call to `overrides_from_flags`; `new_run()`'s near-verbatim copy +> (`cli.py:915-983` pre-fix) collapsed the same way, reusing the identical table. Router +> overrides (`_router_cli_overrides`, unchanged, still shared by both commands) feed into +> the table as a single pre-aggregated `router_config` entry mapped only to +> `stage1_model.router` — the stage1-only asymmetry noted below is preserved exactly, with +> a regression test. No flag was added, removed, or renamed, and no precedence semantics +> changed: `giant train --help`/`giant new-run --help` are byte-identical before and after, +> verified by diffing both. Everything below this point describes the pre-fix state and is +> kept for historical context. + **Severity: High. Effort: Medium.** **Location:** `giant/cli.py:329-839`. @@ -488,6 +508,31 @@ separate inline comment blocks to reconstruct. ## Issue 4 — `cli.py` sits at 35.8 % coverage and holds untested override-precedence logic +> **Status: Fixed for the override-precedence logic (Issue 3's scope); the +> `predict`/`rollout` inference-bootstrap portion described below is still open — that is +> Issue 5, deliberately not attempted here.** Commit `2bfb1ab` on +> `v0.3.0-stage2-autoregressive` adds direct, `CliRunner`-free unit tests for +> `overrides_from_flags` in `tests/test_config.py` — one per precedence rule, including +> several with previously **zero** coverage: both legs of `--stage{1,2}-generator` +> overriding `--mode` (only the stage1 leg had a test before), all three stage1 +> shorthand-vs-`--stage1-*` pairs (previously only `hidden_dim`), all four WGAN knobs' +> shared-vs-per-stage precedence (previously only `n_critic`/`gp_weight`), the +> `--emb-dim`/`--conditioning` dual-axis fan-out (previously untested via `train` at all), +> and an explicit regression test that `router_config` only ever writes +> `stage1_model.router`. `tests/test_cli_train_overrides.py` keeps its original 4 +> `CliRunner` smoke tests unmodified, plus 4 new ones covering the `--batch-size auto` +> parse-error and success paths and all three `out_dir` resolution branches +> (`--out`/`--resume`/default) — previously entirely uncovered. The percentage barely moves +> (36 % → 35 %), because the extraction *deleted* more statements from `cli.py` (539→478) +> than the new tests cover elsewhere in the file, but covered statements rose in absolute +> terms (157→169) and every override-precedence line the original issue called out by +> number is now covered. Coverage of `cli.py`'s other listed gap — the `predict`/ +> `rollout` inference bootstrap (`cli.py:1104-1423`/`1528-1702` in the pre-fix numbering) — +> is unchanged, since fixing that requires the `load_for_inference` extraction described in +> Issue 5, which is explicitly out of scope for this fix (done separately, if at all). +> Everything below this point describes the pre-fix state and is kept for historical +> context. + **Severity: High. Effort: Medium.** **Location:** `giant/cli.py` (1865 lines).