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