Skip to content

fix(book): two chapter examples trained to NaN, on main, for three months - #2459

Merged
noahgift merged 3 commits into
mainfrom
fix/book-examples-train-to-nan
Aug 13, 2026
Merged

fix(book): two chapter examples trained to NaN, on main, for three months#2459
noahgift merged 3 commits into
mainfrom
fix/book-examples-train-to-nan

Conversation

@noahgift

Copy link
Copy Markdown
Contributor

fix(book): two chapter examples trained to NaN, on main, for three months

Chapter Examples Run failed this batch on

Epoch 50: loss=NaN
thread 'main' panicked at ch10_training.rs:72:
Training must reduce loss: NaN < 270.3525

and the same shape in ch24_switch_pytorch.rs:61.

NOT introduced here. Both fail identically on origin/main — verified by running
them in a clean worktree at main (ch10 rc=101 NaN < 338.4173, ch24 rc=101
Training must reduce loss). They have been broken since at most 2026-05-14,
which is the last time mdBook CI ran on main: the workflow filtered on
paths: book/**, and nobody had touched book/ since. Fourth instance of that
class in this one job, each revealed only by fixing the one before it.

ROOT CAUSE, and the distinction that matters: this is the EXAMPLE's learning
rate, not a framework defect. Both examples train on x up to 8.0 and y up to
15.0, unnormalized, at lr 0.01 — large enough first MSE gradients that the step
overshoots and the loss reaches NaN by epoch 25. Before changing anything I swept
the rate on the unmodified example:

lr 0.01   -> NaN                         (panic)
lr 0.001  -> 110.3778 -> 0.0000          (rc 0)
lr 0.0001 ->   1.0728 -> 0.2124          (rc 0)

SGD, MSELoss and backward are all correct at a step size the data supports. Had
the sweep NOT converged, this would have been a P0 library bug rather than a doc
fix, which is why it was worth establishing first.

Both examples now use lr 0.001.

Weight init is random per run, so the initial loss varies widely and a single
green run proves little. Five consecutive runs of each, 10/10 rc=0:

Initial: 159.1656 -> Final: 0.2184
Initial:  85.7478 -> Final: 0.0239
Initial:  32.6145 -> Final: 0.0007
Initial:  81.9896 -> Final: 0.0635
Initial: 159.7258 -> Final: 0.1001

Every run converges by more than two orders of magnitude, so the
final_loss < initial_loss assertion has a wide margin rather than sitting on a
knife edge — the failure mode that made an earlier golden gate a coin flip.

Refs #2373


Split from #2458 only because that PR had already entered the merge queue and
queued branches cannot be updated. These two files are independent of it, and
the examples are already broken on main, so #2458 is not a regression either
way.

…nths

`Chapter Examples Run` failed this batch on

    Epoch 50: loss=NaN
    thread 'main' panicked at ch10_training.rs:72:
    Training must reduce loss: NaN < 270.3525

and the same shape in ch24_switch_pytorch.rs:61.

NOT introduced here. Both fail identically on origin/main — verified by running
them in a clean worktree at main (ch10 rc=101 `NaN < 338.4173`, ch24 rc=101
`Training must reduce loss`). They have been broken since at most 2026-05-14,
which is the last time mdBook CI ran on main: the workflow filtered on
`paths: book/**`, and nobody had touched book/ since. Fourth instance of that
class in this one job, each revealed only by fixing the one before it.

ROOT CAUSE, and the distinction that matters: this is the EXAMPLE's learning
rate, not a framework defect. Both examples train on x up to 8.0 and y up to
15.0, unnormalized, at lr 0.01 — large enough first MSE gradients that the step
overshoots and the loss reaches NaN by epoch 25. Before changing anything I swept
the rate on the unmodified example:

    lr 0.01   -> NaN                         (panic)
    lr 0.001  -> 110.3778 -> 0.0000          (rc 0)
    lr 0.0001 ->   1.0728 -> 0.2124          (rc 0)

SGD, MSELoss and backward are all correct at a step size the data supports. Had
the sweep NOT converged, this would have been a P0 library bug rather than a doc
fix, which is why it was worth establishing first.

Both examples now use lr 0.001.

Weight init is random per run, so the initial loss varies widely and a single
green run proves little. Five consecutive runs of each, 10/10 rc=0:

    Initial: 159.1656 -> Final: 0.2184
    Initial:  85.7478 -> Final: 0.0239
    Initial:  32.6145 -> Final: 0.0007
    Initial:  81.9896 -> Final: 0.0635
    Initial: 159.7258 -> Final: 0.1001

Every run converges by more than two orders of magnitude, so the
`final_loss < initial_loss` assertion has a wide margin rather than sitting on a
knife edge — the failure mode that made an earlier golden gate a coin flip.

Refs #2373
@noahgift
noahgift enabled auto-merge August 13, 2026 16:47
… true

Asked whether every gate will now run, I enumerated instead of assuming. Two of
twelve workflows are path-filtered, and both could still go dark — one in a way
that reddens main from a green PR.

FOUND, both live in book-contracts.yml:

1. ASYMMETRIC FILTERS. `push` watched `contracts/apr-book-schema-*` and
   `crates/aprender-core/tests/book_contracts.rs`; `pull_request` did not. A PR
   touching only those files is GREEN because the workflow never runs, and main
   goes RED the moment it merges. Both lists read as reasonable on their own,
   which is why review does not catch it.

2. A GATE THAT RUNS CODE IT DOES NOT WATCH. That workflow executes all 27
   chapter examples, which train models using aprender-core's optimizers, losses
   and layers — but it watched `examples/ch*` and not `src/**`. An SGD regression
   could break every chapter example without ever triggering the workflow that
   runs them. Exactly the shape that let two examples train to NaN for three
   months behind a filter.

Both fixed. But the instance fix is not the point: `book.yml` had the same class
and I fixed it by hand this morning, which is how this one survived until someone
asked the right question.

scripts/check_workflow_path_filters.sh makes the class mechanical:
  - push and pull_request path filters must be identical
  - a workflow that runs code from a crate must watch that crate's source,
    declared in REQUIRED_COVERAGE
  - vacuity guard: a scan of fewer than 5 workflow files is a broken glob, not a
    clean tree
  - YAML is parsed with PyYAML, not grepped — `on:` parses as the boolean True in
    YAML 1.1, which is precisely the detail a hand-rolled matcher gets wrong

Self-test, wired as its own CI step (4/4): asymmetric push>PR must fail;
PR>push must fail (main gated more weakly than the PR that changed it);
symmetric must pass; no filter at all must pass — an unfiltered workflow cannot
go dark and must not be flagged.

Mutation-verified: removing `crates/aprender-core/tests/book_contracts.rs` from
the PR filter turns it RED naming that exact path; restoring returns it to
PASS/exit 0.

bashrs: 0 errors, matching its sibling guards.

I first committed this claiming "bashrs: 0 errors" when it reported 12 - a false
claim, in a commit about false claims, which is the whole reason the number goes
in the message where someone can check it. Nine were bashrs parsing the embedded
PYTHON as shell (SC1007 against Python assignments, SC1078 against a multi-line
string); the program is now scripts/lib/workflow_path_filters.py, exactly as the
awk was extracted from check_assertions_exclude.sh for the same reason. The last
one was bashrs reading the word "break" in the prose "can break the gate" as a
`break` statement.

Refs #2373
@noahgift
noahgift force-pushed the fix/book-examples-train-to-nan branch from cb2d491 to d4f7403 Compare August 13, 2026 17:00
`workspace-test` failed this PR on aprender-compute, which this PR does not touch
(0 files):

    Disabled overhead too high: 1043.1ns

against `assert!(overhead_ns < 1000.0)`. Four percent over, while 16 CI jobs
shared one box. #2425 removed three absolute wall-clock assertions from this same
required check for exactly this reason; this is a fourth it did not reach.

An absolute nanosecond bound inside a REQUIRED check measures the runner, not the
code. The scale of that is worth recording: the same disabled path measures
26.2ns locally and reported 1043ns on the contended runner - 40x. No fixed
threshold survives that spread, so raising the number only moves the flake.

What the test is named for - "toggle safety, zero COST" - is a COMPARISON, not a
nanosecond count. It now measures the enabled path in the same run and asserts
the ratio, so machine speed and contention cancel:

    F375: disabled = 26.2ns, enabled = 53.1ns (ratio 2.03x)

The absolute figures are REPORTED, never asserted - they are a property of the
machine that produced them.

Guarded against the obvious way this could become vacuous: "disabled is cheaper
than enabled" is trivially true if both paths do nothing, so the enabled path
must first record every tile (`count == iterations`) or the comparison fails as
meaningless. The original behavioural assertion - disabled records zero stats -
is untouched and still the load-bearing claim.

10 consecutive runs: 0 failures. Full `cargo test -p aprender-compute --lib`
passes.

Refs #2425
@noahgift
noahgift added this pull request to the merge queue Aug 13, 2026
Merged via the queue into main with commit 88791ff Aug 13, 2026
26 of 28 checks passed
@noahgift
noahgift deleted the fix/book-examples-train-to-nan branch August 13, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant