fix: tolerate f32 reassociation in mllama ragged test, add nightly gate by inureyes · Pull Request #953 · lablup/mlxcel

inureyes · 2026-07-28T13:01:21Z

Summary

Two fixes for one root cause. models::mllama::text::tests::ragged_real_tile_rows_match_reference_masked_full_rows has failed deterministically on main since #622 landed on 2026-07-02, because it asserts exact f32 equality across a reduction whose lane assignment differs between the two sides. That failure survived 26 days because no workflow runs the general unit suite, which is the second and more consequential half.

Half 1: the assertion

The failure value is 3.7252903e-9, which is bit-exactly 2^-28, against an output whose max absolute element is 1.3224149e-1 (1 ULP there is 1.4901161e-8). The difference is 0.25 ULP of the largest element. A genuine masking or row-selection error would surface at order 1e-1, eight orders larger, because the padding rows carry content of the same magnitude as the real rows.

The issue records six probes that localize the cause. Rather than re-run them, I checked their conclusion holds:

A concatenate-built 6-row input versus a directly allocated one is exactly 0.0, so the concatenate construction is not the trigger.
Interior-masked 8 rows versus contiguous 6 rows reproduces 3.7252903e-9 with no concatenate involved, so the interior mask position alone is the trigger.
Trailing-mask variants at kv_len 6, 5 and 4 are all exactly 0.0. That independently confirms the exp(logit - 1e9) == 0.0 premise perf(vision/mllama): build cross-attention states from real tiles only #622 relies on, and it is the reason the two sibling tests legitimately pass on exact equality. Both of those siblings still pass on exact equality in this branch, which is the same premise holding under test.

The failing case is the only one where surviving rows move lane position: they sit at {0,1,4,5,6,7} in the 8-wide reduction and at {0..5} in the 6-wide one. That reassociates both the softmax denominator and the value accumulation, and f32 addition is not associative. The equivalence #622 proves holds in exact arithmetic, and exact arithmetic does not imply bitwise equality once lanes move inside a parallel reduction.

So this is a tolerance question, not a production bug. Only the sliced path runs in production, since the port threads no text-side mask at all; the masked branch exists solely inside this test. No production code changed.

ragged_real_tile_rows_match_reference_masked_full_rows now asserts diff <= 1e-6 against a named RAGGED_REASSOCIATION_TOL constant, and carries a comment recording why it tolerates a last-bit difference while its siblings do not, plus an explicit instruction not to tighten it back. 1e-6 is about 5 orders of magnitude below the 1e-1 output scale and about 2.5 orders above the observed artifact, so it stays loud on a real error, and it matches the max_abs_diff(..) < 1e-6 bound already used elsewhere in src/models/. real_tile_rows_match_reference_masked_full_rows and full_tile_mask_is_the_unmasked_computation keep their exact assert_eq!(..., 0.0) and still pass.

The test was red on arrival, not a later regression. The issue reports building at #622's merge commit 0ad300c3f and getting the identical value, and the supporting history checks out on this branch: git log 0ad300c3f..origin/main -- src/models/mllama/ is empty, attention_from_ptr in src/lib/mlxcel-core/src/layers.rs last changed in #149, and MLX_EXPECTED_COMMIT last moved in #270 on 2026-06-14, both before the test landed.

Half 2: the absent gate, and the decision

The accurate statement is that no job runs the general unit suite, not that no workflow contains cargo test. pipeline-parallel-ci.yml runs cargo clippy -p mlxcel --lib --tests -- -D warnings directly and reaches cargo test through scripts/ci/run-pp-heterogeneous-memory.sh and run-pp-two-host-logical.sh, but with distributed::-prefixed selectors that could never have matched models::mllama or main_tests, behind a path filter that never fires on a model-port PR. The ci.yml header comment claiming clippy and cargo-test "do NOT run in any workflow" is therefore itself inaccurate, and is corrected here.

This is systemic rather than a one-off: family_order_is_exhaustive in src/main_tests.rs was also failing on main, missing MiniMax VLM and Step VLM from FAMILY_ORDER after #800 and #781 each added a family without registering it, and was repaired in #946. Two independent deterministic, weight-free failures reached main through the same gap.

Decision: a scheduled nightly on the self-hosted runner, not a PR-time gate

New .github/workflows/nightly-verify.yml runs make verify (fmt + clippy + test) once a day at 18:00 UTC (03:00 KST) on self-hosted-macos-26-arm64, plus workflow_dispatch for on-demand runs. A failed scheduled run files a GitHub issue titled [nightly-verify] main is red, reusing that issue on subsequent nights rather than filing duplicates, so the signal is not another Actions tab nobody opens. The three verify steps run independently so a fmt violation cannot mask a red suite.

Weighed against the cost rationale that motivated the removal in #23:

It does not reinstate what chore(ci): drop clippy+test from release pipeline; keep gate local-only #23 took out. chore(ci): drop clippy+test from release pipeline; keep gate local-only #23's rationale, verbatim, was that release builds should not "block on a slow lint/test pass that make verify would have caught locally", and chore(ci): move clippy + test from PR-time CI to release pipeline #21 before it argued the runner round-trip "wasn't adding signal that wasn't reachable locally". Both objections are about blocking. This blocks nothing, and consumes that runner once per day at a quiet hour instead of once per PR.
It supplies the one signal that is genuinely not reachable locally. make verify tells a developer whether the tree is green. It cannot tell anyone whether the tree was green when nobody ran it, which is the exact failure mode both of these tests hit. That is what a scheduled run adds, and it adds it without taking on the cost that chore(ci): move clippy + test from PR-time CI to release pipeline #21 and chore(ci): drop clippy+test from release pipeline; keep gate local-only #23 rejected.
A cheap targeted PR-time job is not actually cheap. Any Rust test in this repository first builds MLX C++ through mlxcel-core, so narrowing the selector to main_tests and the detection/metadata registration tests does not narrow the build. The existing two-host-logical job budgets 45 minutes on ubuntu-latest for exactly that reason.
A GitHub-hosted job would also be a weaker signal. ubuntu-latest builds without metal and accelerate, leaving the gated regions of mlxcel-core that the Makefile's own verify comment calls out untypechecked. It would have caught family_order_is_exhaustive, but it could not have reproduced the mllama failure at all, since that is a Metal SDPA reduction-order artifact. A green ubuntu job would have reported "fine" while main was red.
Exposure drops from 26 days to about one. If a day ever proves too long, the documented next step is adding push: [main], which costs one run per merge rather than one per PR.

The runner carries no model weights, which is fine: the real-checkpoint tests under tests/ self-skip on a missing models/<name> dir or are #[ignore]d, so what the nightly actually gates is the weight-free unit and contract suite, which is where both known failures lived.

A third instance, which this PR's own CI ran into

Worth recording because it is the same defect class and it happened during this PR. The cargo-fmt check here failed, and not on formatting: it died at rustup toolchain install 1.100.0 with a 404, because #146 bumped dtolnay/rust-toolchain to a ref naming a Rust release that does not exist. cargo fmt is the only PR-time Rust gate this repository has, and it had not run since 2026-07-27. It hid for the same structural reason as the other two: the job is gated on needs.changes.outputs.rust == 'true', so a docs-only or packaging-only merge skips it and the workflow still reports success.

That is now fixed on main by #954, which landed while this PR was open and reached the same conclusion independently. This branch is rebased onto it and carries none of that change; the duplicate commit was dropped rather than merged. Three independent instances in one issue's lifetime is the argument for the nightly, not against it.

One thing that is deliberately not fixed here

main currently emits two -D warnings-fatal warnings that predate this PR, both visible in the --bin mlxcel build:

warning: unused import ...                                  src/multimodal/host_preprocessor.rs
warning: function `export_qwen2_vl_prefill` is never used   src/multimodal/host_preprocessor_export.rs:154

They are not fixed here, deliberately. export_qwen2_vl_prefill is not actually dead: src/multimodal/host_preprocessor.rs:428 and the module's tests call it, so the warning is a feature-gated artifact of the binary's build configuration, and the right fix is a cfg decision belonging to the in-flight OpenXLA vision work rather than a deletion from this PR. Both arrived with #915 two days ago and nothing caught them, which is a third, milder instance of the same gap this issue is about.

The consequence is that unless they are resolved first, the first nightly run will fail on its clippy step and file its tracking issue. That is the mechanism working, and it is why the three verify steps are split: a red clippy still leaves make verify-test running and reported separately, so it cannot hide the state of the suite.

What changed

src/models/mllama/text.rs: ragged_real_tile_rows_match_reference_masked_full_rows moves from assert_eq!(max_abs_diff(..), 0.0) to assert!(diff <= RAGGED_REASSOCIATION_TOL) with the reassociation rationale recorded inline. The module comment above the three tests now says two of them pin byte-level equality while one tolerates the artifact. No production code touched.
.github/workflows/nightly-verify.yml (new): scheduled make verify on the self-hosted Apple Silicon runner, with issue-filing on failure, a fork guard, a nightly-verify concurrency group, and the persistent CARGO_TARGET_DIR pattern release.yml already uses.
.github/workflows/ci.yml: header comment corrected. It no longer claims clippy and cargo-test run in no workflow, and now names both the path-filtered pipeline-parallel-ci.yml jobs and the new nightly.
CONTRIBUTING.md lines 44 to 49: the two commands are marked NOT gated at PR time; yours to run instead of "enforced by CI on self-hosted macOS runner", and the paragraph now describes the gate that exists. The old claim came from ci: add clippy and cargo test gates via self-hosted macOS runner #14 and has been false since chore(ci): move clippy + test from PR-time CI to release pipeline #21 moved the gate and chore(ci): drop clippy+test from release pipeline; keep gate local-only #23 deleted it on 2026-05-18, with neither commit updating the doc.
Makefile: the verify* header claimed to match ".github/workflows/ci.yml exactly" and to reproduce a clippy + test (macOS ARM64) job that no longer exists. It now points at nightly-verify.yml, which genuinely invokes these same targets, and states that the local run is the real gate.

Five files, one commit. The toolchain-ref fix described above is not part of this diff; it is #954's, and this branch is rebased on top of it.

Test plan

DEVELOPER_DIR=... cargo test --release --lib --features metal,accelerate models::mllama on an Apple M1 Ultra: 8 passed, 0 failed, including both siblings still on exact equality
Deliberate-failure demonstration on the same command the nightly runs. Temporarily setting RAGGED_REASSOCIATION_TOL to 0.0 fails with ragged per-image selection must match the reference-masked full attention to within 0e0, got 3.7252903e-9, which is bit-exactly the 2^-28 the issue reports. That confirms both the issue's measurement and that the new assertion is not vacuous. Restored to 1e-6 and re-run green.
DEVELOPER_DIR=... cargo test --release --features metal,accelerate --bin mlxcel family_order: 2 passed, 0 failed, confirming the other known instance is green after feat(models): add Ant Group Ling / Bailing MoE (bailing_moe) text model support #946
cargo fmt --all -- --check
actionlint on both workflows: only the expected unknown-custom-runner-label note for self-hosted-macos-26-arm64, which release.yml already carries
BASE_REF=origin/main python3 scripts/ci/check_cross_repo_refs.py
cargo-fmt and cargo-deny green on this PR once rebased onto ci: restore the rust-toolchain pin and stop dependabot from bumping it #954 (cargo-fmt was red before that, for the toolchain-ref reason above, not for formatting)

One acceptance criterion is discharged by the mechanism rather than by hand. The entire cargo test --release --features metal,accelerate suite was not run here; the two known failures were verified individually with narrow selectors. The nightly's first run establishes the whole-suite state on the real runner and files anything still red, which is the "or each remaining failure is filed as its own issue" branch of that criterion.

Closes #939

The ragged cross-attention test has failed deterministically on `main` since #622 landed on 2026-07-02, asserting exact f32 equality between an interior-masked 8-row attention and a contiguous 6-row one. Those two differ by 2^-28, which is 0.25 ULP of the largest output element, because the surviving keys occupy lanes {0,1,4,5,6,7} in the 8-wide reduction and {0..5} in the 6-wide one, reassociating both the softmax denominator and the value accumulation. The equivalence #622 proves holds in exact arithmetic, and exact arithmetic does not imply bitwise equality once lanes move inside a parallel reduction. The two sibling tests mask a trailing run instead, so every surviving key keeps its lane position, and they keep their exact `assert_eq!(..., 0.0)`. No production code changes: only the sliced path runs in production, and the masked branch exists solely inside this test. That failure survived 26 days because no job runs the general unit suite. pipeline-parallel-ci.yml does run clippy and `cargo test`, but with `distributed::`-prefixed selectors behind a path filter that never fires on a model-port PR, so the ci.yml header comment claiming neither runs in any workflow was itself inaccurate. The same gap let `family_order_is_exhaustive` sit red on `main` until #946, which makes this systemic rather than a one-off. nightly-verify.yml runs `make verify` once a day on `self-hosted-macos-26-arm64` and files a GitHub issue when it fails, reusing that issue on subsequent nights. It deliberately does not reinstate the PR-time gate #23 removed: it blocks nothing and consumes the shared runner once per day rather than once per PR, while running the same feature set and hardware class in which the mllama failure is observable at all. A GitHub-hosted job would not have been cheaper, since every Rust test here first builds MLX C++ through mlxcel-core, and it would build without metal or accelerate, so it could not have reproduced this failure. CONTRIBUTING.md has claimed since #14 that CI enforces clippy and `cargo test` on the self-hosted macOS runner for every Rust PR. #21 moved that gate and #23 deleted it on 2026-05-18, neither updating the doc, so the claim has been false since then. The Makefile `verify*` header made the same stale claim about a `clippy + test (macOS ARM64)` job and now points at nightly-verify.yml, which genuinely invokes those same targets. Validated on an Apple M1 Ultra: `cargo test --release --lib --features metal,accelerate models::mllama` passes 8/8, and temporarily setting the new tolerance to 0.0 reproduces the original failure at the reported value, so the assertion is not vacuous. Closes #939

… calls it (#961) ## What breaks `Pipeline Parallel CI / 2-host logical` fails on every PR that touches the pipeline-parallel paths. The pipeline-parallel step itself passes; only the trailing `Clippy on PP modules` step fails, and it fails on `src/multimodal/`, which such a PR typically does not touch at all. `export_qwen2_vl_prefill` is imported at `src/multimodal/host_preprocessor.rs` and defined at `src/multimodal/host_preprocessor_export.rs` unconditionally, but its only non-test call site is inside a `#[cfg(feature = "xla-iree")]` block. The workflow runs `cargo clippy -p mlxcel --lib --tests -- -D warnings` with no features, so that call site compiles out, the import becomes unused, the function becomes dead, and `-D warnings` promotes both to errors. It went unnoticed because the workflow is path-filtered to the pipeline-parallel modules and last ran on 2026-07-20, while the unconditional import arrived on 2026-07-24 in `edc0ebbb6`. The first PR to touch those paths since is where it surfaced. ## The fix Gate the import and the definition on `any(feature = "xla-iree", test)`. The feature arm matches the single production call site; the `test` arm is needed because the module tests exercise the function irrespective of features, and dropping them would trade a lint failure for lost coverage. ## Validation Reproduced the exact CI invocation locally. Before: two errors, `unused import: export_qwen2_vl_prefill` and `function export_qwen2_vl_prefill is never used`, and `could not compile mlxcel (lib)`. After: `Finished dev profile`, clean. The normal development configuration is unaffected: `cargo clippy --release --lib --tests --features metal,accelerate` is clean, and the Qwen2-VL export tests still compile and pass. One unrelated test in that module, `xla_loader_keeps_text_and_unqualified_vlm_image_capability_false`, fails both before and after this change and also fails on `main`, so it is out of scope here. ## Why this is filed as its own PR It is a repository-wide unblock rather than part of any feature branch, and it is the second breakage of this shape found this week after the `dtolnay/rust-toolchain` pin in #954: pre-existing on `main`, invisible because a path filter or a feature gate kept any run from exercising the failing combination. The nightly verification job added in #953 targets exactly that blind spot.

…l three outcomes (#987) ## What was broken `multimodal::host_preprocessor::tests::xla_loader_keeps_text_and_unqualified_vlm_image_capability_false` has failed deterministically on `main` since `edc0ebbb6` (#915). It loops over `["llama", "qwen2_vl"]` asserting `load_xla_image_preprocessor(...)` returns `Ok(None)`, but #915 made `qwen2_vl` return `Err(InvalidConfig("Qwen2-VL XLA image execution requires the xla-iree feature"))` when the feature is absent, so the `.unwrap()` panics. That commit edited the same test file, adding an import and two export tests, and left this loop untouched, so the contract and the test that pins it diverged inside a single change to a single file. This is the third deterministically-failing test found on `main` in one week, after the two repaired in #946 and #953. It is the first one the nightly verification job added in #953 should catch on its own. ## Which side is right The production behavior stays; the test changes. The reasoning, recorded here so it is not relitigated: Qwen2-VL has no MLX vision fallback. The arm immediately above already rejects `MLXCEL_XLA_VISION_BACKEND=host` for it with exactly that reason, so in a build without `xla-iree` there is no code path that can embed its images. `Ok(None)` for such a checkpoint is indistinguishable from a text-only checkpoint, so a session would start with every image silently ignored, which is a worse outcome than a named failure at load. Failing at load also takes away nothing that worked. `xla-iree = ["xla-backend", "mlxcel-xla/iree"]`, and without `mlxcel-xla/iree` the session's `prefill` and `decode_step` return `mlxcel_xla::NOT_WIRED`, whose text is "the OpenXLA inference session was built without the `iree` feature; rebuild ...". An `xla-backend`-only build cannot generate from any checkpoint at all, so there is no working configuration being broken here. Feature absence already errors elsewhere in the same function: an explicit `MLXCEL_XLA_VISION_BACKEND=iree` returns `Err` for the same reason. ## The counter-argument, addressed rather than dropped Without `xla-iree`, LLaVA does **not** error. Unless the policy is explicitly `iree` it falls through to the host preprocessor and returns `Ok(Some(host))`. Qwen2-VL is the only family where a missing compile-time feature is fatal, and that asymmetry deserved an answer rather than silence. It is deliberate and now documented where a reader will find it: LLaVA has a complete MLX host vision tower and projector to fall back to, Qwen2-VL has none. The doc comment on `load_xla_image_preprocessor` previously described only two of the three outcomes the function implements, which is precisely how the contract drifted from its test. It now covers all three (`Ok(None)`, `Ok(Some)`, `Err`), states which families reach each and why, and records the `NOT_WIRED` reasoning. ## Changes The capability-false loop drops `qwen2_vl` and gains `mllama`, so it still covers an unqualified VLM family rather than degrading to a text-only-only test, with an inline comment explaining why `qwen2_vl` is deliberately absent and where its contract is pinned instead. A new `xla_loader_rejects_qwen2_vl_without_the_iree_feature`, gated `#[cfg(not(feature = "xla-iree"))]` because with the feature the same config takes the IREE load path, asserts the error variant and that its message both names the missing feature and carries the `--features xla-iree` rebuild instruction. Both callers surface this string verbatim into a startup failure, so the instruction is the only actionable part an operator receives; the error text was extended to carry it. ## Validation `cargo test --release --lib --features metal,accelerate multimodal::host_preprocessor` goes from `11 passed; 1 failed` to `13 passed; 0 failed`. `cargo fmt --all -- --check` and the cross-repo-ref guard are clean. No production behavior changes: the only non-test edits are the doc comment and the error string. Closes #966

inureyes force-pushed the fix/issue-939-mllama-ragged-tolerance-ci-gate branch from ed72cc3 to dc6c6fb Compare July 28, 2026 13:09

inureyes merged commit 755f113 into main Jul 28, 2026
6 checks passed

inureyes mentioned this pull request Jul 28, 2026

fix(multimodal): gate the Qwen2-VL prefill export on the feature that calls it #961

Merged

This was referenced Jul 30, 2026

fix: Reconcile the Qwen2-VL XLA loader contract with its failing capability test #966

Closed

fix(multimodal): pin the Qwen2-VL XLA loader contract and document all three outcomes #987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: tolerate f32 reassociation in mllama ragged test, add nightly gate - #953

fix: tolerate f32 reassociation in mllama ragged test, add nightly gate#953
inureyes merged 1 commit into
mainfrom
fix/issue-939-mllama-ragged-tolerance-ci-gate

inureyes commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

inureyes commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Half 1: the assertion

Half 2: the absent gate, and the decision

Decision: a scheduled nightly on the self-hosted runner, not a PR-time gate

A third instance, which this PR's own CI ran into

One thing that is deliberately not fixed here

What changed

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

inureyes commented Jul 28, 2026 •

edited

Loading