Skip to content

ggml-opt: graphs built per step start every optimizer period from zero gradients - #45

Merged
joelteply merged 3 commits into
feat/props-weight-residencyfrom
fix/opt-per-step-accumulators
Oct 7, 2026
Merged

joelteply merged 3 commits into
feat/props-weight-residencyfrom
fix/opt-per-step-accumulators

Conversation

@joelteply

Copy link
Copy Markdown

ggml_opt_prepare_alloc, the path llama_context training takes (it builds a graph per ubatch), keeps the gradient accumulators in ctx_static across graphs, and each backward adds into them in place. The only reset runs before the build, against gb_grad, which is null in this mode. So nothing ever zeroed the accumulators, and step k applied the sum of every gradient since the run began. Upstream ggml-org has the same code.

Measured. test-opt-dynamic-accum (new) runs SGD on loss = w·x, so every step's gradient is x. On the current code, the second step moved w by 2·lr·x instead of lr·x. With the fix, both opt_period 1 and 2 pass, and test-opt still passes 4/4 backend × optimizer combinations (static graphs are unchanged).

The fix: after the per-step build, when a period begins, zero the parameter accumulators.

Effect on real training (same data, seed and lr; train/eval loss per epoch):

run before (accumulating) after
Qwen2.5-Coder 1.5B, walk, 3 epochs 0.680/0.621 · 0.422/0.455 · 0.325/0.483 0.697/0.680 · 0.513/0.522 · 0.376/0.417
Qwen3.5 0.8B hybrid, 2 epochs 0.702/0.496 · 0.341/0.290 0.720/0.533 · 0.393/0.362

The bug behaved like unbounded momentum: it learned faster early, then overshot, and the 1.5B's eval loss rises in epoch 3. With the fix, eval loss is still falling at the end. On the 1.5B it ends lower; on the short 0.8B run it is slower, because the learning rate the core tuned while the bug was present is now too small. The lr default should be re-measured after this lands (a continuum-side follow-up).

This is also a prerequisite for the walk's exact gradient (#40 follow-up), which accumulates gradients across chunks and must start each window from zero.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q4NU4VNiELPQfBpCacDZGc

…o gradients

ggml_opt_prepare_alloc (the path llama_context's training takes: a graph per ubatch) keeps the
gradient accumulators in ctx_static across graphs, and each backward adds into them in place.
The only reset sat before the build, against gb_grad, which is null in this mode, so nothing
ever zeroed them: step k applied the SUM of every gradient since the run began. Measured
(test-opt-dynamic-accum, SGD on loss = w*x): the second step moved w by 2*lr*x, not lr*x.
Upstream has the same code.

The fix zeroes the parameter accumulators after the per-step build when a period begins.
Static graphs are unchanged (test-opt: 4/4 backend x optimizer pass).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q4NU4VNiELPQfBpCacDZGc
@joelteply
joelteply changed the base branch from master to feat/props-weight-residency October 6, 2026 22:38
@joelteply

Copy link
Copy Markdown
Author

Approve at e2278fb (Fable).

Confirmed by reading ggml-opt.cpp on the base. In per-step mode accumulate is true for every period, including opt_period 1, and grad_accs live in ctx_static, created once (grad_accs.empty()). The only reset (ggml_graph_reset(gb_grad) in ggml_opt_alloc) runs against a graph that ggml_opt_eval nulls after every step, and for opt_period 1 it never runs at all. So every backward added into the same accumulators. The fix zeroes them at the period start, after the build, and leaves the LOSS accumulator (the seed of 1) alone, which is right.

Verified the test is a real gate on the M5 (CPU build of this head): with the fix, test-opt-dynamic-accum: OK. With the ggml-opt.cpp hunk reverted, it fails with opt_period 1, step 2: w moved to 0.250000, expected 0.500000 (from 0.750000), i.e. 2·lr·x, your measurement.

One hardening note, not blocking: the loop runs i over grad_accs.size() (the FIRST graph's node count) and reads gf->nodes[i] of the current graph. That's only safe while every step's graph has the same node count, which ggml_build_backward_expand's use of the same array already assumes. Bounding it with i < (size_t) opt_ctx->gf->n_nodes, or asserting the counts are equal, turns a silent out-of-range read into a named failure if a graph ever changes shape.

Worth an upstream PR too: ggml-org has the same code.

…nodes (Fable on #45)

grad_accs is indexed by the first graph's nodes; a later per-step graph may hold fewer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q4NU4VNiELPQfBpCacDZGc
…kend-DL build links no ggml_backend_cpu_init)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q4NU4VNiELPQfBpCacDZGc
@joelteply
joelteply merged commit fac6962 into feat/props-weight-residency Oct 7, 2026
10 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant