From dc6c6fbbb6c92a72609d43e051a844c20d919473 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Tue, 28 Jul 2026 21:48:55 +0900 Subject: [PATCH] fix: tolerate f32 reassociation in mllama ragged test, add nightly gate 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 --- .github/workflows/ci.yml | 27 +++- .github/workflows/nightly-verify.yml | 195 +++++++++++++++++++++++++++ CONTRIBUTING.md | 10 +- Makefile | 14 +- src/models/mllama/text.rs | 45 +++++-- 5 files changed, 266 insertions(+), 25 deletions(-) create mode 100644 .github/workflows/nightly-verify.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 51bc6a166..e3a2b9359 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,13 +2,28 @@ # advisory) and cargo-fmt on every touched-Rust change so formatting drift and # license issues are caught at PR time. # -# Clippy and cargo-test do NOT run in any workflow. They used to run on the -# self-hosted Apple Silicon runner — first here at PR time, then briefly in -# release.yml — and in both cases consumed ~30 min per run, blocking either -# PRs or releases on a shared resource for failures that `make verify` -# reliably catches on the developer's machine in a fraction of the time. +# Neither clippy nor the general unit suite is gated at PR time. Both used to +# run on the self-hosted Apple Silicon runner, first here and then briefly in +# release.yml, and in both cases consumed ~30 min per run, blocking either PRs +# or releases on a shared resource for failures that `make verify` reliably +# catches on the developer's machine in a fraction of the time (#21, #23). # -# Quality gate lives locally. Pre-push checklist: +# They are not absent from every workflow, though, so do not read the above as +# "nothing anywhere runs them": +# +# - pipeline-parallel-ci.yml runs `cargo clippy -p mlxcel --lib --tests +# -- -D warnings`, and reaches `cargo test` through scripts/ci/run-pp-*.sh +# with `distributed::`-prefixed selectors. It is debug profile on +# ubuntu-latest and path-filtered to src/distributed/pipeline/** and its +# siblings, so it never runs on a model-port PR and never selects a model +# or CLI test module. +# - nightly-verify.yml runs the full `make verify` (fmt + clippy + test) once +# a day on the self-hosted Apple Silicon runner and files an issue when it +# fails. It is a backstop against a red `main`, not a PR gate; see #939 for +# the two deterministic failures that sat on `main` before it existed, and +# for why it is nightly rather than per-PR. +# +# Quality gate still lives locally at PR time. Pre-push checklist: # make verify # fmt + clippy(metal,accelerate, -D warnings) + test(release) # make verify-clean # same, after `cargo clean` — use when clippy's # # per-crate cache may be hiding a regression diff --git a/.github/workflows/nightly-verify.yml b/.github/workflows/nightly-verify.yml new file mode 100644 index 000000000..e78ac521b --- /dev/null +++ b/.github/workflows/nightly-verify.yml @@ -0,0 +1,195 @@ +# Nightly `make verify` on the self-hosted Apple Silicon runner. +# +# WHY THIS EXISTS +# +# No workflow ran the general unit suite, and two deterministically failing +# tests reached `main` and sat there unnoticed as a result: the mllama +# cross-attention assertion fixed in #939, red for 26 days, and +# `family_order_is_exhaustive` in `src/main_tests.rs`, repaired in #946. Both +# are pure unit tests that need no model weights. The gap is not the policy of +# keeping the gate local (see below); it is that nothing noticed when the local +# gate was already red before a contributor ever ran it. +# +# WHY NIGHTLY AND NOT PR-TIME +# +# `clippy + test` on this target takes ~30 min on the shared self-hosted +# runner. #21 moved that gate from PR time to release time and #23 removed it +# entirely, for a cost reason that still holds: a slow shared resource blocking +# every PR is worse than `make verify` on the developer's machine. This +# workflow does not reinstate the PR-time gate. It consumes the shared runner +# once per day at a quiet hour, blocks nothing, and bounds how long a red +# `main` can hide from 26 days to about one. +# +# WHY NOT A CHEAP GITHUB-HOSTED JOB INSTEAD +# +# Any Rust test in this repository first builds MLX C++ through `mlxcel-core`, +# so narrowing the test selector does not make a GitHub-hosted job cheap; the +# `two-host-logical` job in pipeline-parallel-ci.yml budgets 45 min on +# `ubuntu-latest` for that reason. It would also be a weaker signal: an +# `ubuntu-latest` runner builds without `metal` and `accelerate`, leaving large +# gated regions of mlxcel-core unchecked, and the mllama failure was a Metal +# SDPA reduction-order artifact that such a job could not have reproduced at +# all. Running the real feature set on the real hardware class is the point. +# +# WHAT IT RUNS +# +# `make verify` (fmt + clippy + test), driven through the same Makefile targets +# CONTRIBUTING.md tells contributors to run, so CI and the local gate cannot +# drift apart. The three steps run independently so one failure does not mask +# the other two. +# +# The runner carries no model weights, which is fine: the real-checkpoint +# integration tests under tests/ self-skip on a missing `models/` dir or +# are `#[ignore]`d. What this run actually gates is the weight-free unit and +# contract suite, which is exactly where both known failures lived. +# +# HOW A FAILURE IS REPORTED +# +# A red Actions run that nobody opens is the same blind spot in a new place, so +# a failed scheduled run files (or comments on) a GitHub issue. If a day of +# exposure ever proves too long, the next step is adding `push: [main]` here, +# which costs one run per merge instead of one per day. + +name: Nightly verify + +on: + schedule: + # 18:00 UTC = 03:00 KST, when the shared runner is idle. + - cron: "0 18 * * *" + workflow_dispatch: + +# Default-deny; the job grants only what it needs. +permissions: {} + +concurrency: + # Never stack two runs on the single shared macOS runner. A manual dispatch + # queues behind an in-flight nightly rather than cancelling it. + group: nightly-verify + cancel-in-progress: false + +jobs: + verify: + name: fmt + clippy + test (macOS Apple Silicon) + # Scheduled workflows run from the default branch of whatever repository + # holds them. A fork that enables Actions would otherwise queue forever + # against runner labels it does not own. + if: github.repository == 'lablup/mlxcel' + runs-on: self-hosted-macos-26-arm64 # Self-hosted Apple Silicon runner with macOS 26 SDK + Metal 4 + # Generous because the FIRST run is a cold MLX C++ build plus a release + # link of every integration-test binary. Steady state, with the persistent + # target dir below, is the ~30 min `make verify` takes locally. + timeout-minutes: 180 + permissions: + contents: read + issues: write + + steps: + - name: Checkout code + uses: actions/checkout@v7 + with: + # Don't leave GITHUB_TOKEN in .git/config; this job only fetches. + persist-credentials: false + + # Mirrors release.yml, deliberately including the same path: a persistent + # target dir outside the workspace so the nightly is incremental instead + # of a cold MLX C++ build every time, and so the daily run keeps the + # release job's cache warm. release.yml owns the 7-day prune of this + # directory; cargo's own file lock serializes the rare overlap. + - name: Setup persistent cache paths (self-hosted) + run: | + set -euo pipefail + CARGO_TARGET="$HOME/.cargo-target/mlxcel" + mkdir -p "$CARGO_TARGET" + echo "CARGO_TARGET_DIR=$CARGO_TARGET" >> "$GITHUB_ENV" + + - name: Ensure build tools + run: | + set -euo pipefail + # cmake is required by the mlxcel-core and sentencepiece-sys build scripts. + if ! command -v cmake >/dev/null 2>&1; then + echo "cmake not found, installing via Homebrew..." + brew install cmake + fi + echo "cmake: $(cmake --version | head -1)" + + # rust-toolchain.toml pins the channel (and the rustfmt/clippy + # components) that every cargo invocation below actually resolves to, so + # this step exists to guarantee rustup is present and current, not to + # choose the version. Do not "fix" the mismatch by pinning a channel + # here: the file is the single source, deliberately. + - name: Install Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + components: clippy, rustfmt + + # The three steps below are `make verify` split apart. Each runs even if + # an earlier one failed, so a fmt violation does not hide a red suite. + - name: cargo fmt + id: fmt + if: ${{ !cancelled() }} + run: make verify-fmt + + - name: cargo clippy (metal,accelerate, -D warnings) + id: clippy + if: ${{ !cancelled() }} + run: make verify-clippy + + - name: cargo test (release, metal,accelerate) + id: test + if: ${{ !cancelled() }} + run: make verify-test + + - name: Report a red main + # Only the scheduled run reports. A manual dispatch is someone already + # watching the run, and filing an issue at them is noise. + if: ${{ failure() && github.event_name == 'schedule' }} + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + COMMIT_SHA: ${{ github.sha }} + FMT_OUTCOME: ${{ steps.fmt.outcome }} + CLIPPY_OUTCOME: ${{ steps.clippy.outcome }} + TEST_OUTCOME: ${{ steps.test.outcome }} + run: | + set -euo pipefail + + if ! command -v gh >/dev/null 2>&1; then + echo "gh not found, installing via Homebrew..." + brew install gh + fi + + TITLE='[nightly-verify] main is red' + BODY_FILE="$(mktemp -t nightly-verify-body)" + { + echo "The nightly \`make verify\` run on \`self-hosted-macos-26-arm64\` failed." + echo + echo "| Step | Outcome |" + echo "|---|---|" + echo "| \`make verify-fmt\` | \`${FMT_OUTCOME}\` |" + echo "| \`make verify-clippy\` | \`${CLIPPY_OUTCOME}\` |" + echo "| \`make verify-test\` | \`${TEST_OUTCOME}\` |" + echo + echo "Commit: \`${COMMIT_SHA}\`" + echo "Run: ${RUN_URL}" + echo + echo "Reproduce locally with \`make verify\` (or the single failing target above)." + echo "This issue is reused by subsequent nightly failures while it stays open." + } > "$BODY_FILE" + + # `|| true` on purpose: a transient search failure must not swallow + # the report. An empty result files a fresh issue, and a rare + # duplicate is a better failure mode than silence. + EXISTING="$(gh issue list --repo "$REPO" --state open \ + --search "in:title \"$TITLE\"" --limit 1 \ + --json number -q '.[0].number // empty' || true)" + + if [ -n "$EXISTING" ]; then + gh issue comment "$EXISTING" --repo "$REPO" --body-file "$BODY_FILE" + echo "Commented on existing tracking issue #${EXISTING}." + else + gh issue create --repo "$REPO" --title "$TITLE" --body-file "$BODY_FILE" \ + --label "type:bug,priority:high,status:ready" + fi + + rm -f "$BODY_FILE" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 72132d383..2a123d798 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,12 +41,12 @@ Thank you for your interest in contributing to mlxcel! This document covers the ``` 4. Run the local quality gates: ```bash - cargo fmt --all -- --check # enforced by CI; fmt violations block merge - cargo clippy --all-targets --features metal,accelerate -- -D warnings # enforced by CI on self-hosted macOS runner - cargo test --release --features metal,accelerate # enforced by CI on self-hosted macOS runner - cargo deny check # advisories + licenses + sources + cargo fmt --all -- --check # gated at PR time; fmt violations block merge + cargo clippy --all-targets --features metal,accelerate -- -D warnings # NOT gated at PR time; yours to run + cargo test --release --features metal,accelerate # NOT gated at PR time; yours to run + cargo deny check # gated at PR time (advisories + licenses + sources) ``` - CI enforces `clippy` (with `-D warnings`) and `cargo test` on the `self-hosted-macos-26-arm64` runner on every PR that touches Rust files. CUDA verification is not gated at PR time — that stays exclusive to `release.yml`. + PR-time CI runs only the cheap gates: `cargo fmt` and `cargo deny` in [`ci.yml`](.github/workflows/ci.yml), plus a path-filtered clippy and a `distributed::`-scoped `cargo test` in [`pipeline-parallel-ci.yml`](.github/workflows/pipeline-parallel-ci.yml) when you touch pipeline-parallel code. Clippy and the general unit suite are **not** enforced on your PR. They were moved in #21 and removed in #23 because ~30 min per run on the shared self-hosted Apple Silicon runner blocked PRs and releases for failures that `make verify` catches locally in a fraction of the time. [`nightly-verify.yml`](.github/workflows/nightly-verify.yml) runs the full `make verify` once a day on `self-hosted-macos-26-arm64` and files an issue when `main` goes red, so a broken suite surfaces within a day rather than on the next contributor's `make verify`. Treat that as a backstop, not a substitute: run the two commands above yourself before you push. CUDA verification is not gated at PR time either; that stays exclusive to `release.yml`. While iterating, prefer `[profile.test-fast]` over `--release`: `make test-fast` / `make test-fast-cuda` (or `cargo test --profile test-fast --features <...>`) rebuild in seconds instead of minutes. It is for local/agent edit-test loops only; run the `--release` commands above (or `make verify`) before opening or updating a PR. See [`docs/installation.md`](docs/installation.md#fast-iteration-builds) for the measured comparison. 5. For inference changes, validate against a real checkpoint — synthetic or build-only validation is not enough (see [`AGENTS.md`](AGENTS.md) for why). diff --git a/Makefile b/Makefile index 57d0278be..ed82e5a41 100644 --- a/Makefile +++ b/Makefile @@ -433,11 +433,15 @@ ci: fmt-check check clippy test ## CI workflow: format check, check, lint, test pre-commit: fmt clippy test ## Pre-commit checks # ---------------------------------------------------------------------------- -# CI-faithful local gate (matches .github/workflows/ci.yml exactly) +# CI-faithful local gate (matches .github/workflows/nightly-verify.yml) # -# The `verify*` targets reproduce the GitHub Actions `clippy + test (macOS -# ARM64)` job step-for-step. They differ from the looser `clippy` / `test` -# targets above in three ways that have repeatedly bitten us: +# The `verify*` targets ARE what the nightly workflow runs: nightly-verify.yml +# invokes these same Makefile targets so the local gate and the scheduled +# backstop cannot drift apart. Note that ci.yml gates only fmt and cargo-deny +# at PR time, so running this before you push is the real gate, not a +# formality; the nightly is only a net that catches a red `main` within a day. +# They differ from the looser `clippy` / `test` targets above in three ways +# that have repeatedly bitten us: # # 1. `--features metal,accelerate` — the CI feature set. Without it, large # gated regions of mlxcel-core (parts of cache/turbo/quant, the @@ -473,7 +477,7 @@ verify-test: ## CI-faithful: cargo test --release --features metal,accelerate .PHONY: verify verify: verify-fmt verify-clippy verify-test ## Run the full CI-faithful gate locally (recommended before push) - @echo "$(GREEN)[verify] OK — matches GitHub Actions clippy+test job$(RESET)" + @echo "$(GREEN)[verify] OK: matches the nightly-verify GitHub Actions job$(RESET)" .PHONY: verify-clean verify-clean: ## Run `verify` after a `cargo clean` (use when clippy's cache may be hiding a regression) diff --git a/src/models/mllama/text.rs b/src/models/mllama/text.rs index 0a9fbfa2f..2208b4d72 100644 --- a/src/models/mllama/text.rs +++ b/src/models/mllama/text.rs @@ -589,9 +589,11 @@ mod tests { // from the per-image `num_tiles`). `exp(logit - 1e9)` underflows to // exactly 0.0, so those positions add exact zeros to the softmax numerator // and denominator: attending over the REAL-tile rows alone is the same - // computation. These tests pin that equivalence at the byte level, which - // is what licenses `MllamaVLModel` to drop the padding-tile rows from + // computation. These tests pin that equivalence, which is what licenses + // `MllamaVLModel` to drop the padding-tile rows from // `cross_attention_states` instead of threading a reference-style mask. + // Two of the three pin it at the byte level; the ragged case tolerates a + // last-bit f32 reassociation artifact for the reason spelled out on it. /// `[1, rows, HIDDEN]` cross-states with identifiable per-row content. fn cross_rows(rows: i32, seed: usize) -> UniquePtr { @@ -641,9 +643,17 @@ mod tests { ); } + /// Bound for the ragged case below. The equivalence this file pins holds + /// in exact arithmetic, but the ragged case reassociates an f32 reduction + /// (see the test's comment), so it is bitwise-exact only up to a last-bit + /// artifact. `1e-6` is ~5 orders of magnitude below the ~`1e-1` output + /// scale, so it still fails loudly on a real masking or row-selection + /// error while tolerating the reassociation. + const RAGGED_REASSOCIATION_TOL: f32 = 1e-6; + /// Ragged multi-image (media 0: 1 real tile of 2, media 1: 2 of 2): - /// media-major concatenation of each image's real rows is byte-identical - /// to reference-masked attention over the full row set. + /// media-major concatenation of each image's real rows matches + /// reference-masked attention over the full row set. #[test] fn ragged_real_tile_rows_match_reference_masked_full_rows() { let config = tiny_config(); @@ -661,11 +671,28 @@ mod tests { let sliced = mlxcel_core::concatenate(&media0, &media1, 1); let sliced_out = layer.forward(&h, &sliced, None); - assert_eq!( - max_abs_diff(&masked_out, &sliced_out), - 0.0, - "ragged per-image selection must be byte-identical to the \ - reference-masked full attention" + // This is the one case in this file that is NOT bitwise-exact, and the + // reason is the mask position, not the row selection. Its two siblings + // mask a TRAILING run of keys, so every surviving key keeps its lane + // position and the softmax denominator and value accumulation are + // summed in the same association order on both sides. Those two stay + // on `assert_eq!(..., 0.0)`. Here the masked columns are INTERIOR ({2, 3} + // of 8), so the surviving keys sit at lanes {0,1,4,5,6,7} in the 8-wide + // reduction but at {0..5} in the 6-wide one. f32 addition is not + // associative, so the two orders differ in the last bit: the observed + // difference on an Apple M1 Ultra is 2^-28, which is 0.25 ULP of the + // largest output element. Masking still contributes exact zeros + // (`exp(logit - 1e9)` underflows to 0.0); that premise is what the + // trailing-mask siblings pin, and it is unaffected. Do not tighten + // this back to exact equality; the equality holds in exact arithmetic, + // which does not imply bitwise equality once lanes move inside a + // parallel reduction, and the artifact is expected to vary with + // hardware and kernel tiling. + let diff = max_abs_diff(&masked_out, &sliced_out); + assert!( + diff <= RAGGED_REASSOCIATION_TOL, + "ragged per-image selection must match the reference-masked full \ + attention to within {RAGGED_REASSOCIATION_TOL:e}, got {diff:e}" ); }