Repository navigation
Conversation
…sed form is 7.8e-3 off The upstream fix for the scale rounding (huggingface/candle#4057) divides by the f32 scale; the fix written here while the issue was open multiplies by an f64 reciprocal; candle's own slow path divides in f64. All three round the normalised value to the storage dtype before the weight multiply, so they should agree - and they do, on 512 rows of 2560 elements: 0 of 1,310,720 bf16 values differ between any pair of them. The old fused form is a different matter: 58,637 of 1,310,720 elements differ from the fixed forms, with a maximum relative difference of 7.812e-3. That is the same 7.8e-3 the issue reported, so the upstream fix removes the reported defect and not a smaller one. The test is also the evidence for adopting the upstream fix rather than keeping the local one: the two are bit-identical, so the change is a no-op numerically and the only thing it buys is one implementation instead of two.
|
Reviewed this against the issue it fixes, with a measurement of my own. The change is right. It restores the order the reference implementations use: compute the scale in f32, divide in f32, round the normalised value to the storage dtype, then multiply by the weight in the storage dtype. That is what candle's own Three measurements, 512 rows of 2560 elements, 1,310,720 bf16 values. I compared the three f32-scale forms - your fused path, the slow path, and the form I wrote locally while the issue was open (which multiplies by an f64 reciprocal rather than dividing by an f32 scale, so it is a genuinely different arithmetic): All three are bit-identical, so the f32-versus-f64 difference in the scale does not survive the bf16 rounding. And against the old fused form: 7.812e-3 is the same magnitude the issue reported, so this removes the reported defect rather than a smaller one. One question, which is about scope rather than about this patch. This changes only the CPU kernel. The CUDA and Metal paths have their own kernels, and if they round the scale the same way then the issue is not fixed for those, and closing #4052 on the CPU change alone would be premature. I could not check the CUDA kernel from the tree I have (the crate is not vendored there), so I am asking rather than asserting: do the GPU kernels keep the scale in f32, and if so, is the issue's wording - "the two implementations in A note on the reproduction. The issue's 2560-element sinusoidal case matched the slow path on your machine, and you could not reproduce the full-model result. The likely reason is that the old code's output depended on where the f32 scale happened to land when rounded to bf16, which is a coarse quantisation of a value that itself depends on the order of the sum of squares. So the old behaviour was not only wrong, it was sensitive to the accumulation order - and the parallel sum makes that order thread-count dependent. The fix removes the systematic part, which is the important part; the residual sensitivity of the parallel sum to the thread count remains, and is inherent to accumulating in f32 rather than something this patch introduces. If it is useful, a test that runs the same input at two thread counts and asserts the bits agree would pin that down. One thing for the record: this was prepared with Codex assistance, which the description says. Not a comment on the patch - the patch reads as careful, the tests are the right shape for a rounding-order bug (exact bit patterns, both half dtypes, one row and 33 rows so the serial and parallel paths are both covered, nonzero storage offsets on the input and the weight, and a case on the rounding boundary before the weight multiply), and the failure against the original production code is what makes them worth having. |
…s, and it does not close the gap huggingface/candle#4057 keeps the CPU RMSNorm scale in f32 and rounds the normalised value instead, which is the order candle's own slow path and Hugging Face both use, so the two implementations in ops.rs agree with each other. Measured over 1,310,720 bf16 values, the three f32-scale forms - the PR's, the slow path's, and the one written here - are bit-identical, and the old fused form is 7.812e-3 off, the same magnitude the issue reported. So adopting it is a no-op numerically and buys one implementation instead of two: when it merges, the local rms_norm_f32_scale and its 13 call sites can go. It does not close the 3.54e-03 that is left, because that is the width of the activations around the attention and not the norm's scale. The PR's scope is one open question: it changes only the CPU kernel, and the CUDA and Metal paths have their own kernels. If they round the scale the same way then #4052 is not fixed for those. The CUDA crate is not vendored in the tree here, so it is asked on the PR rather than asserted.
|
One addition to my review above, on the scope question, from checking the tree here. There is no arch-specific CPU path for RMSNorm to miss, so the CPU half of this fix is complete. That also explains why the defect was easy to miss for a long time. A generic scalar Rust path that is auto-vectorised by LLVM for the sum of squares but not for the per-element conversion is not obviously "a fused kernel with a rounding bug" - it reads like an implementation detail, and the slow path next to it does the right thing by construction. Worth noting for the record, from the same tree: the per-element conversion is exactly what a hand-written kernel would avoid. If it is useful to the discussion: the difference between "the two implementations in |
The stack is cumulative - each PR takes the previous ones' paths - so the delta is what gets reviewed, and 'small' is a property of the delta rather than of the branch. pr/01 should be dropped rather than opened: it is the RMSNorm f32-scale fix, and huggingface/candle#4057 fixes the same defect upstream, so once that lands the local reimplementation is redundant and slower than candle's. The work since the stack was built needs new PRs on top of it: the per-expert gather, the scoped dequants on both paths, the fp8 dense-weight cache, the expert cache wiring, the kolibri1 GGUF path. One does not belong in the stack at all - the fp8 dequant's f32 form touches only blockwise_fp8/ops.rs and depends on nothing, so it is standalone and off master, pushed as pr/01-fp8-dequant-f32.
The stack is cumulative - each PR takes the previous ones' paths - so the delta is what gets reviewed, and 'small' is a property of the delta rather than of the branch. pr/01 should be dropped rather than opened: it is the RMSNorm f32-scale fix, and huggingface/candle#4057 fixes the same defect upstream, so once that lands the local reimplementation is redundant and slower than candle's. The work since the stack was built needs new PRs on top of it: the per-expert gather, the scoped dequants on both paths, the fp8 dense-weight cache, the expert cache wiring, the kolibri1 GGUF path. One does not belong in the stack at all - the fp8 dequant's f32 form touches only blockwise_fp8/ops.rs and depends on nothing, so it is standalone and off master, pushed as pr/01-fp8-dequant-f32.
…sed form is 7.8e-3 off The upstream fix for the scale rounding (huggingface/candle#4057) divides by the f32 scale; the fix written here while the issue was open multiplies by an f64 reciprocal; candle's own slow path divides in f64. All three round the normalised value to the storage dtype before the weight multiply, so they should agree - and they do, on 512 rows of 2560 elements: 0 of 1,310,720 bf16 values differ between any pair of them. The old fused form is a different matter: 58,637 of 1,310,720 elements differ from the fixed forms, with a maximum relative difference of 7.812e-3. That is the same 7.8e-3 the issue reported, so the upstream fix removes the reported defect and not a smaller one. The test is also the evidence for adopting the upstream fix rather than keeping the local one: the two are bit-identical, so the change is a no-op numerically and the only thing it buys is one implementation instead of two.
…s, and it does not close the gap huggingface/candle#4057 keeps the CPU RMSNorm scale in f32 and rounds the normalised value instead, which is the order candle's own slow path and Hugging Face both use, so the two implementations in ops.rs agree with each other. Measured over 1,310,720 bf16 values, the three f32-scale forms - the PR's, the slow path's, and the one written here - are bit-identical, and the old fused form is 7.812e-3 off, the same magnitude the issue reported. So adopting it is a no-op numerically and buys one implementation instead of two: when it merges, the local rms_norm_f32_scale and its 13 call sites can go. It does not close the 3.54e-03 that is left, because that is the width of the activations around the attention and not the norm's scale. The PR's scope is one open question: it changes only the CPU kernel, and the CUDA and Metal paths have their own kernels. If they round the scale the same way then #4052 is not fixed for those. The CUDA crate is not vendored in the tree here, so it is asked on the PR rather than asserted.
|
Thanks for the independent 512 x 2,560 BF16 measurement. Your three-way agreement and the 58,637 differing values against the old fused result provide useful validation beyond my local reproduction. I traced the GPU implementations at this PR's head:
This is source inspection only; I have not run CUDA or Metal. I changed the closing reference to a related-issue reference so this CPU patch will leave #4052 open for the remaining backend-scope discussion and GPU validation. One correction on thread count: the CPU implementation parallelizes whole rows above 32 rows. Each row's sum of squares uses a serial iterator, so changing Rayon worker count changes row scheduling, not the within-row reduction order. The existing 1-row and 33-row tests exercise both dispatch branches; a worker-count comparison would not exercise a different reduction order here. This source review and response were prepared with Codex assistance. |
The fused CPU RMSNorm kernel rounds the normalization scale to the storage dtype before dividing. For F16/BF16 this can change the normalized values relative to
rms_norm_slow.Keep the scale and division in F32, then round the normalized value back to the storage dtype before multiplying by the weight. This matches the existing slow path's rounding order and preserves the F32 operation order.
Two CPU regressions compare exact F16/BF16 bit patterns and the slow path. They cover one row and 33 rows (serial and parallel execution), nonzero input/weight storage offsets, and the rounding boundary before weight multiplication. Both fail against the original production code.
Validation on Windows x64, Rust 1.99.1, native CPU Release:
cargo test --release -p candle-nn --lib --tests: 67 passed.cargo test --release -p candle-nn --doc: 6 passed, 2 existing ignored examples.cargo clippy --release -p candle-nn --tests --examples --benches -- -D warnings: passed.cargo fmt --all -- --checkandgit diff --check: passed.The small current-main reproduction shows the discrepancy in both half dtypes. The issue's 2560-element sinusoidal case matched the slow path on this machine; I have not reproduced the reported full-model result. CUDA/Metal and full-model inference were not tested, and this patch changes only the CPU kernel.
This implementation, regression coverage, PR text and local code review were prepared with Codex assistance.
Related to #4052 (CPU portion). GPU arithmetic and runtime validation remain follow-up work.