fix(memit): accumulate FFN covariance in f64; escalate Cholesky ridge before failing - #339
Open
Ai-Eli-ML wants to merge 1 commit into
Open
fix(memit): accumulate FFN covariance in f64; escalate Cholesky ridge before failing#339Ai-Eli-ML wants to merge 1 commit into
Ai-Eli-ML wants to merge 1 commit into
Conversation
… before failing
estimate_ffn_covariance accumulated K^T K in f32. On Gemma 4 26B-A4B (ffn_dim=2112, activations ~1e3) the entries reach ~1e6*N and the 24-bit mantissa drops the low-order contributions, so the Gram matrix stops being numerically PSD and the Cholesky in memit.rs hits a negative pivot ('Cholesky failed'). Converting to f64 after accumulation (cov_f64) cannot recover what was already lost.
- trace.rs: accumulate and scale in f64, downcast to f32 once at the boundary. Signature unchanged.
- memit.rs: if Cholesky still fails, escalate ridge x10 up to 1000x before returning the error, logging each step. Defensive; the f64 accumulation is the fix.
Originally fixed locally 2026-04-21/22; ported onto current main 2026-08-30.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
estimate_ffn_covariance(crates/larql-inference/src/forward/trace.rs) accumulatesKᵀKinf32. The comment above it says "Float64 would be safer but Array2 suffices at our scales" — that stops being true on larger models.Observed on Gemma 4 26B-A4B (
ffn_dim = 2112, activation magnitudes ~1e3) while running MEMIT locally: each accumulator entry grows to ~1e6 × N over thousands of sampled token positions; with a 24-bit mantissa the low-order contributions are dropped, and the accumulated Gram matrix is no longer numerically PSD. The Cholesky inmemit.rsthen hits a negative pivot and MEMIT fails withCholesky failed.memit.rsalready converts the covariance tof64(cov_f64) before Cholesky, but that cannot recover precision lost during accumulation — the damage is upstream of that cast.Change
trace.rs— accumulateKᵀKinf64, scale inf64, downcast tof32once at the boundary when returning. Public signature unchanged (Option<(Array2<f32>, usize)>). Memory: one temporaryffn_dim²matrix at 8 bytes instead of 4 (≈36 MB vs 18 MB at 2112).memit.rs— if Cholesky still fails on an architecture nobody has profiled, escalateridge×10 up to 1000× before returning the error, logging each escalation viaeprintln!. Defensive only; (1) is the fix. Behaviour at the original ridge is unchanged when the first attempt succeeds.Not included: the ridge value. My local fork hard-coded
ridge = 10.0for Gemma 4 in April;LARQL_MEMIT_RIDGE(now incompile/into_model.rs/into_vindex.rs) is the right design, so nothing to add there.Verification
cargo build -p larql-inference -p larql-lqlon the pinned 1.98.0 toolchain: clean.cargo clippy -p larql-inference -- -D warnings: clean.rustfmt --checkon both files: clean.cargo test -p larql-inference: exit 0 — but note the crate's tests are#[ignore]model-gated (0 ran, 4 ignored on Linux/CPU), so this is a compile/clippy-level check, not behavioural coverage of the change.(N, ffn_dim)matrix with ~1e3 magnitudes and asserts the f32 vs f64 Gram matrices differ beyond tolerance.Context
This was fixed in a local fork on 2026-04-21/22 and sat there; a repo cleanup surfaced it and a check of upstream
trace.rs:303showed thef32accumulator still present, so here it is. First PR to this repo — happy to reshape it to whatever conventions you prefer.