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" ); }