From 337459f31b47a8f1c7468fbbc37a2af6750f9894 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 7 Aug 2026 15:37:14 +0900 Subject: [PATCH] fix(tests): tolerate f32 reassociation in the mllama tile-selection parity test `sub_max_real_tiles_keep_the_legacy_real_rows_byte_identical` asserted exact f32 equality between the 1-real-tile path and the corresponding rows of the all-tiles path. Selecting 1 tile of 4 makes the vision encoder reduce over a different extent, and f32 addition is not associative, so the equivalence that holds in exact arithmetic never implied bitwise equality. Measured on Apple M5 Max: the difference is 5.9604645e-8 (2^-24) against a largest output element of 1.1521907, where 1 ULP is 1.1920929e-7. That is 0.5 ULP, the smallest nonzero difference representable there. A real row-selection or padding error would surface at the 1e0 output scale, seven orders larger, so the new 1e-6 bound stays loud on an actual defect. This is the defect #953 fixed for the sibling assertion in `src/models/mllama/text.rs`; this file was missed at the time. The three sibling assertions here that do not move lane position keep their exact equality and still pass, which is the same premise holding under test. Also report the measured divergence in the two chunked-SDPA assertions in `layers.rs`. They named only the chunk size, so the magnitude behind their failures had to be recovered by patching the assertion locally while investigating #1065. Their tolerances are deliberately unchanged: those two are catching a real M5 precision problem and loosening them would mask it. --- CHANGELOG.md | 4 ++++ src/lib/mlxcel-core/src/layers.rs | 10 +++++---- tests/mllama_parity.rs | 37 ++++++++++++++++++++++++------- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e0d44de5..483de9891 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- `mllama_parity::sub_max_real_tiles_keep_the_legacy_real_rows_byte_identical` no longer demands exact f32 equality. Selecting 1 real tile of 4 makes the vision encoder reduce over a different extent than the all-tiles path, and f32 addition is not associative, so the equivalence that holds in exact arithmetic never implied bitwise equality. On Apple M5 Max the difference is 5.9604645e-8 (2^-24) against a largest output element of 1.1521907, which is 0.5 ULP; a real row-selection error would surface seven orders larger. The assertion moves to a named 1e-6 bound. This is the same defect #953 fixed for the sibling assertion in `src/models/mllama/text.rs`, which missed this file. The two chunked-SDPA assertions in `layers.rs` now also report the measured divergence instead of only naming the chunk size (#1065). + ### Changed - The pinned Rust toolchain moves from 1.93.1 (2026-02-11) to 1.97.1 (2026-07-14), and the `dtolnay/rust-toolchain` tag in `ci.yml` tracks it as the comment there requires. The workflows that install `@stable` are unaffected and were never building at a different version: that action runs `rustup default` and never exports `RUSTUP_TOOLCHAIN`, so `rust-toolchain.toml` overrode it per directory and every cargo invocation in the tree already resolved to the pin. `cargo fmt` produces no diff at the new version, so the bump reformats nothing, but six new clippy lints fire under `-D warnings` and are fixed here: `question_mark` in `memory_estimate.rs` and `sanitize.rs`, `collapsible_match` in `chat_request.rs`, `for_kv_map` and `unnecessary_cast` in two test modules, and `unneeded_wildcard_pattern` in `pipeline_remote_real_models.rs`. All six are mechanical and behavior-preserving. diff --git a/src/lib/mlxcel-core/src/layers.rs b/src/lib/mlxcel-core/src/layers.rs index e38dc82d0..4c43b02e4 100644 --- a/src/lib/mlxcel-core/src/layers.rs +++ b/src/lib/mlxcel-core/src/layers.rs @@ -6285,9 +6285,10 @@ mod tests { chunk, ); assert_eq!(ffi::array_shape(&chunked), ffi::array_shape(&full)); + let diff = max_abs_diff(&full, &chunked); assert!( - max_abs_diff(&full, &chunked) < 1e-5, - "chunk={chunk} diverged from unchunked SDPA" + diff < 1e-5, + "chunk={chunk} diverged from unchunked SDPA by {diff}" ); } } @@ -6341,9 +6342,10 @@ mod tests { for chunk in [1, 2, 3] { let chunked = chunked_causal_attention(&q, &k, &v, scale, chunk); assert_eq!(ffi::array_shape(&chunked), ffi::array_shape(&native)); + let diff = max_abs_diff(&native, &chunked); assert!( - max_abs_diff(&native, &chunked) < 1e-5, - "q_len={q_len} k_len={k_len} chunk={chunk} diverged from do_causal SDPA" + diff < 1e-5, + "q_len={q_len} k_len={k_len} chunk={chunk} diverged from do_causal SDPA by {diff}" ); } } diff --git a/tests/mllama_parity.rs b/tests/mllama_parity.rs index 64a4dff78..1e73b7638 100644 --- a/tests/mllama_parity.rs +++ b/tests/mllama_parity.rs @@ -251,6 +251,15 @@ fn cross_states() -> UniquePtr { mlxcel_core::from_slice_f32(&fill(n, 42), &[1, KV_LEN, HIDDEN]) } +/// Tolerance for the one comparison in this file whose two sides reassociate a +/// parallel reduction differently, so exact f32 equality is not a property the +/// code can have. See `sub_max_real_tiles_keep_the_legacy_real_rows_byte_identical`. +/// +/// Do not tighten this back to `0.0`. The sibling assertion in +/// `src/models/mllama/text.rs` was moved off exact equality for the same reason +/// in #953, and this file's case was missed at the time. +const TILE_SELECTION_REASSOCIATION_TOL: f32 = 1e-6; + /// Max absolute elementwise difference between two arrays. fn max_abs_diff(a: &MlxArray, b: &MlxArray) -> f32 { let diff = mlxcel_core::subtract(a, b); @@ -521,10 +530,21 @@ fn states_rows(states: &MlxArray, start: i32, end: i32) -> UniquePtr { mlxcel_core::slice(states, &[0, start, 0], &[1, end, HIDDEN]) } -/// (a) Sub-max real tiles: the real-tile states are byte-identical to the -/// corresponding rows of the legacy all-tiles states (slicing before the -/// per-position projector changes nothing), and only the padding-tile rows -/// are dropped. +/// (a) Sub-max real tiles: the real-tile states match the corresponding rows of +/// the legacy all-tiles states (slicing before the per-position projector +/// changes nothing), and only the padding-tile rows are dropped. +/// +/// This one tolerates a last-bit difference where its siblings below assert +/// exact equality, because it is the only case here where the surviving rows +/// change lane position inside the reduction: selecting 1 real tile of 4 makes +/// the vision encoder reduce over a 1-tile extent instead of a 4-tile one, and +/// f32 addition is not associative, so the equivalence that holds in exact +/// arithmetic does not imply bitwise equality. Measured on Apple M5 Max, the +/// difference is 5.9604645e-8 (2^-24) against an output whose largest element +/// is 1.1521907, where 1 ULP is 1.1920929e-7. That is 0.5 ULP, the smallest +/// nonzero difference representable there. A real row-selection or padding +/// error would surface at the 1e0 output scale, seven orders larger, so the +/// 1e-6 bound stays loud on an actual defect. #[test] fn sub_max_real_tiles_keep_the_legacy_real_rows_byte_identical() { let model = tiny_vl_model(); @@ -542,10 +562,11 @@ fn sub_max_real_tiles_keep_the_legacy_real_rows_byte_identical() { assert_eq!(mlxcel_core::array_shape(&sub), vec![1, V_PATCHES, HIDDEN]); let expected = states_rows(&full, 0, V_PATCHES); - assert_eq!( - max_abs_diff(&sub, &expected), - 0.0, - "real-tile states must be byte-identical to the legacy states' real rows" + let diff = max_abs_diff(&sub, &expected); + assert!( + diff <= TILE_SELECTION_REASSOCIATION_TOL, + "real-tile states diverged from the legacy states' real rows by {diff}, \ + over the {TILE_SELECTION_REASSOCIATION_TOL} reassociation tolerance" ); }