Mark issues.md Issues 3 & 4 as fixed
CI / Format (ruff format) (push) Successful in 28s
CI / Lint (ruff check) (push) Successful in 29s
CI / Sync project version with tag (push) Has been skipped
CI / Lint (ruff check) (pull_request) Successful in 41s
CI / Type check (ty) (push) Successful in 44s
CI / Format (ruff format) (pull_request) Successful in 31s
CI / Sync project version with tag (pull_request) Has been skipped
CI / Type check (ty) (pull_request) Successful in 34s
CI / Tests (push) Successful in 3m24s
CI / Tests (pull_request) Successful in 3m18s
CI / Format (ruff format) (push) Successful in 28s
CI / Lint (ruff check) (push) Successful in 29s
CI / Sync project version with tag (push) Has been skipped
CI / Lint (ruff check) (pull_request) Successful in 41s
CI / Type check (ty) (push) Successful in 44s
CI / Format (ruff format) (pull_request) Successful in 31s
CI / Sync project version with tag (pull_request) Has been skipped
CI / Type check (ty) (pull_request) Successful in 34s
CI / Tests (push) Successful in 3m24s
CI / Tests (pull_request) Successful in 3m18s
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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).
|
||||
|
||||
Reference in New Issue
Block a user