Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
inureyes merged 1 commit into from
Jul 28, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
195 changes: 195 additions & 0 deletions .github/workflows/nightly-verify.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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/<name>` 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"
10 changes: 5 additions & 5 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
14 changes: 9 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
45 changes: 36 additions & 9 deletions src/models/mllama/text.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<MlxArray> {
Expand Down Expand Up @@ -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();
Expand All @@ -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}"
);
}

Expand Down