Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ For inference changes, real-checkpoint validation is required — synthetic-only
-->

- [ ] `cargo fmt --all -- --check` (enforced by CI — violations block merge)
- [ ] `cargo clippy --all-targets -- -D warnings`
- [ ] `cargo test --profile test-fast`
- [ ] `cargo clippy --workspace --all-targets -- -D warnings`
- [ ] `cargo test --workspace --profile test-fast`
- [ ] `cargo deny check`
- [ ] Validated with a real checkpoint (specify which, e.g. `mlx-community/Qwen3.5-0.8B-OptiQ-4bit`): ...

Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
# 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(test-fast)
# make verify # fmt + clippy(workspace, metal,accelerate, -D warnings)
# # + test(workspace, test-fast)
# make verify-clean # same, after `cargo clean` — use when clippy's
# # per-crate cache may be hiding a regression
#
Expand Down
34 changes: 34 additions & 0 deletions .github/workflows/nightly-verify.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,40 @@
# are `#[ignore]`d. What this run actually gates is the weight-free unit and
# contract suite, which is exactly where both known failures lived.
#
# WHICH MEMBERS IT COVERS
#
# All four, as of #1007, because `verify-clippy` and `verify-test` now pass
# `--workspace`. They did not before, and the omission was not a small one: the
# workspace root is itself the `mlxcel` package, so a bare `cargo test` there
# resolves to `-p mlxcel` and never builds mlxcel-core, mlxcel-surgery or
# mlxcel-xla at all. This job was running 5238 of the 6992 tests in the
# repository and calling that the suite. mlxcel-core held 1354 of the missing
# ones and is the crate with the MLX `cxx` bridge, layers.rs, the KV cache and
# the quantization loaders.
#
# The clippy half was the subtler one. `--all-targets` without `--workspace`
# does not compile a member's *test* target either, so test-only lint debt and
# test-only compile errors were equally invisible: six clippy errors were
# sitting in mlxcel-xla's lib-test target while this job passed clean, and five
# `Debug`-bound compile errors in new mlxcel-core tests once passed both this
# job and the local gate. `verify-fmt` never had the hole, because
# `cargo fmt --all` was already workspace-wide.
#
# Feature resolution across the members is not a complication here.
# mlxcel-core resolves to metal + accelerate through the root package's
# forwarding, so one build of it serves every member. mlxcel-surgery and
# mlxcel-xla resolve to their empty defaults, which matters most for
# mlxcel-xla: its `iree` feature stays off, its build script skips the native
# shim, and this job therefore needs no IREE distribution. The code behind
# `iree`, `diagnostics` and `micro-oracle` stays outside the gate for the same
# reason.
#
# On the budget: the switch to `[profile.test-fast]` in #1000 took the
# `cargo test` step from 2h54m34s to 48m54s (run 30871982545), and the scope
# widening spends part of that headroom back. If it ever stops fitting, split
# `verify-test` into per-member steps before dropping members from it; a member
# that is not in the gate is a member nobody is testing.
#
# WHICH PROFILE THE TESTS BUILD UNDER, AND WHAT THAT GIVES UP
#
# `make verify-test` builds under `[profile.test-fast]`, not `[profile.release]`
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ jobs:
# machine in a fraction of the time.
#
# Quality gate lives locally now. Pre-push checklist:
# make verify # fmt + clippy(metal,accelerate, -D warnings) + test(test-fast)
# make verify # fmt + clippy(workspace, metal,accelerate, -D warnings)
# # + test(workspace, test-fast)
# make verify-clean # same, after `cargo clean` — use when clippy's
# # per-crate cache may be hiding a regression
#
Expand Down
12 changes: 8 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,26 @@ Thank you for your interest in contributing to mlxcel! This document covers the
```bash
# macOS (Apple Silicon)
cargo build --release --features metal,accelerate
cargo test --profile test-fast --features metal,accelerate
cargo test --workspace --profile test-fast --features metal,accelerate

# Linux / CUDA
cargo build --release --features cuda
cargo test --profile test-fast --features cuda
cargo test --workspace --profile test-fast --features cuda
```
4. Run the local quality gates:
```bash
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 --profile test-fast --features metal,accelerate # NOT gated at PR time; yours to run
cargo clippy --workspace --all-targets --features metal,accelerate -- -D warnings # NOT gated at PR time; yours to run
cargo test --workspace --profile test-fast --features metal,accelerate --no-fail-fast # NOT gated at PR time; yours to run
cargo deny check # gated at PR time (advisories + licenses + sources)
```
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 or when the run does not finish, 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`.

`make verify` runs exactly the three commands above, and the nightly invokes those same Makefile targets, so the local gate and CI cannot drift apart. Tests build under `[profile.test-fast]` rather than `--release`: `opt-level = 3` is kept so MLX numerics stay representative, while the fat LTO and single codegen unit that make a full `--release` test link take hours are dropped. `make test-fast` / `make test-fast-cuda` are the same profile with `--test-threads=1` and a `FILTER` hook for narrowing the run while you iterate. Reach for `cargo test --release --features metal,accelerate` by hand only when you suspect a defect specific to release codegen. See [`docs/installation.md`](docs/installation.md#fast-iteration-builds) for the measured comparison.

`--workspace` is not optional, and leaving it off is how the gate went blind before (#1007). This repository's workspace root is itself the `mlxcel` package, so a bare `cargo test` or `cargo clippy` resolves to `-p mlxcel` and never builds `mlxcel-core`, `mlxcel-surgery` or `mlxcel-xla` at all, let alone their test targets. That hid 1754 tests, 1354 of them in `mlxcel-core`, which is the crate holding the MLX `cxx` bridge, `layers.rs`, the KV cache and the quantization loaders. It also hid test-only lint debt, since `--all-targets` without `--workspace` does not compile a member's test target either. `cargo fmt --all` was already workspace-wide, which is why the fmt gate never had the hole. Each member builds at the feature set the root selects: `mlxcel-core` gets `metal` and `accelerate` through the root's forwarding, `mlxcel-surgery` and `mlxcel-xla` get their empty defaults. `mlxcel-xla`'s `iree` feature stays off, so the gate needs no IREE distribution and the code behind `iree`, `diagnostics` and `micro-oracle` remains ungated.

`--no-fail-fast` goes with it. Once the run covers four members, the first failing test binary would otherwise end it and hide the other three; cargo still exits non-zero, so the gate is no weaker. Cargo runs the test binaries one at a time, so the `mlxcel-core` suite never shares the Metal device with the root suite, which is the condition that corrupts results in #1008.
5. For inference changes, validate against a real checkpoint. Synthetic or build-only validation is not enough: a shape-compatible change can compile, pass unit tests, and still produce wrong logits on an actual quantized checkpoint. Fetch one with `mlxcel download mlx-community/<model-id>`, and see [`docs/supported-models.md`](docs/supported-models.md) for the families each code path covers. A change to a shared component should be smoke-tested against at least two families.
6. Commit with a conventional prefix (see below) and a clear message.
7. Push to your fork and open a Pull Request. The PR template will prompt for a summary, test plan, and linked issues.
Expand Down
13 changes: 13 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
# There is deliberately no `default-members` here, and adding one is a bigger
# change than it looks. The workspace root is itself the `mlxcel` package, so a
# bare `cargo <cmd>` run here resolves to `-p mlxcel`; `default-members` would
# re-point every such invocation in the repository at once, release.yml's
# `cargo build --release --target aarch64-apple-darwin --locked` included, which
# would start compiling the default-off `mlxcel-xla` into every release build.
#
# The consequence is that a command must say `--workspace` to see all four
# members, and until #1007 the quality gate did not: `make verify-test` and
# `make verify-clippy` covered the root package only, leaving 1754 tests across
# the other three (1354 of them mlxcel-core's) and the whole of their test-target
# lint surface ungated. Both targets now pass `--workspace` explicitly. If you
# add a fifth member, they pick it up with no change here.
[workspace]
members = [".", "src/lib/mlxcel-core", "src/lib/mlxcel-surgery", "src/lib/mlxcel-xla"]
resolver = "2"
Expand Down
61 changes: 53 additions & 8 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,11 @@ test-doc: ## Run documentation tests
# `make test-fast-cuda FILTER=server::chat_request`.
#
# `verify-test` below also builds under [profile.test-fast] as of #1000, so
# these targets and the CI-faithful gate now differ only in feature set and
# `--test-threads=1`, not in codegen.
# these targets and the CI-faithful gate no longer differ in codegen. What they
# do still differ in is scope and reporting: the gate adds `--workspace` and
# `--no-fail-fast` (#1007) and pins the feature set, while these stay on the
# root package with `--test-threads=1` and a FILTER hook, which is what makes
# them fast enough for an edit-test loop.
# ----------------------------------------------------------------------------

.PHONY: test-fast
Expand Down Expand Up @@ -465,6 +468,48 @@ pre-commit: fmt clippy test ## Pre-commit checks
# reproduces only under fat LTO or single-unit codegen; use
# `cargo test --release --features metal,accelerate` by hand when you
# are chasing one.
# 4. `--workspace`: all four members, not just the root package. The
# workspace root IS the `mlxcel` package, so a bare `cargo test` or
# `cargo clippy` resolves to `-p mlxcel` and never builds mlxcel-core,
# mlxcel-surgery or mlxcel-xla, let alone their test targets. 1754 tests
# were invisible on that basis, mlxcel-core's 1354 of them, and so was
# test-only lint debt: six clippy errors sat in mlxcel-xla's lib-test
# target while the root gate passed clean, and during #973 five
# `Debug`-bound compile errors in new mlxcel-core tests passed both of the
# commands above, only `cargo test -p mlxcel-core` finding them (#1007).
# `verify-fmt` never had the hole because `cargo fmt --all` was already
# workspace-wide.
#
# This is a flag here rather than `default-members` in Cargo.toml on
# purpose. `default-members` would silently re-scope every bare cargo
# invocation in the repository, release.yml's
# `cargo build --release --target aarch64-apple-darwin --locked` included,
# which would then compile the default-off `mlxcel-xla` into every release
# build. It would also move the gate's scope into a manifest field that
# the gate's own command line does not mention, and a scope you cannot
# read off the command is the defect this item exists to fix.
#
# What `--workspace` covers is each member at the feature set the root
# selects: mlxcel-core resolves to metal + accelerate through the root's
# forwarding, while mlxcel-surgery and mlxcel-xla resolve to their (empty)
# defaults. mlxcel-xla's `iree` feature stays off, so its build script
# skips the C shim and the gate needs no IREE distribution; the code
# behind `iree`, `diagnostics` and `micro-oracle` is still ungated here.
#
# Cargo builds all the test binaries and then runs them one at a time, so
# the mlxcel-core suite never overlaps the root suite on the Metal device.
# That is what keeps `--workspace` clear of #1008, where two concurrent
# mlxcel-core suites aborted 7 of 12 runs. Anything that starts running
# test binaries in parallel here (cargo-nextest, say) has to re-establish
# that; the `no_other_mlxcel_core_test_binary_is_sharing_the_gpu` guard
# only sees a second mlxcel-core binary, not a root-suite one.
# 5. `--no-fail-fast` on the test target. Without it the first failing test
# binary ends the run, which was harmless when the run was one package and
# is not now: a single red root-suite test would hide all of mlxcel-core,
# mlxcel-surgery and mlxcel-xla behind it. A nightly that costs the better
# part of an hour should report everything that is red in one pass. It
# does not weaken the gate, since cargo still exits non-zero, and it does
# not skip compile errors, which stop the run either way.
#
# Run `make verify` before opening or updating a PR. Run `make verify-clean`
# (which prepends `cargo clean`) when you suspect clippy's per-crate result
Expand All @@ -478,14 +523,14 @@ verify-fmt: ## CI-faithful: cargo fmt --all -- --check
$(CARGO) fmt --all -- --check

.PHONY: verify-clippy
verify-clippy: ## CI-faithful: clippy --all-targets --features metal,accelerate -- -D warnings
@echo "$(CYAN)[verify] clippy (features=metal,accelerate, -D warnings)...$(RESET)"
$(CARGO) clippy --all-targets --features metal,accelerate -- -D warnings
verify-clippy: ## CI-faithful: clippy --workspace --all-targets --features metal,accelerate -- -D warnings
@echo "$(CYAN)[verify] clippy (workspace, features=metal,accelerate, -D warnings)...$(RESET)"
$(CARGO) clippy --workspace --all-targets --features metal,accelerate -- -D warnings

.PHONY: verify-test
verify-test: ## CI-faithful: cargo test --profile test-fast --features metal,accelerate
@echo "$(CYAN)[verify] test (test-fast profile, features=metal,accelerate)...$(RESET)"
$(CARGO) test --profile test-fast --features metal,accelerate
verify-test: ## CI-faithful: cargo test --workspace --profile test-fast --features metal,accelerate --no-fail-fast
@echo "$(CYAN)[verify] test (workspace, test-fast profile, features=metal,accelerate)...$(RESET)"
$(CARGO) test --workspace --profile test-fast --features metal,accelerate --no-fail-fast

.PHONY: verify
verify: verify-fmt verify-clippy verify-test ## Run the full CI-faithful gate locally (recommended before push)
Expand Down
44 changes: 43 additions & 1 deletion docs/installation.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ racing another thread's stream capture) or, with graphs disabled, with
binaries are unaffected; this is a test-parallelism artifact.

```bash
cargo test --profile test-fast --features cuda -- --test-threads=1
cargo test --workspace --profile test-fast --features cuda -- --test-threads=1
```

## Fast iteration builds
Expand Down Expand Up @@ -332,6 +332,48 @@ covered is a defect that reproduces only under fat LTO or `codegen-units = 1`.
Reach for `cargo test --release --features metal,accelerate` by hand when you
are chasing one of those.

## Why the gate says `--workspace`

`make verify-clippy` and `make verify-test` pass `--workspace`, and dropping it
changes what they cover rather than only how fast they run. The workspace root
in this repository is itself the `mlxcel` package, so a bare `cargo test` or
`cargo clippy` here resolves to `-p mlxcel` and never builds `mlxcel-core`,
`mlxcel-surgery` or `mlxcel-xla`. Until #1007 the gate did exactly that, which
left 1754 tests unrun, 1354 of them in `mlxcel-core`, the crate holding the MLX
`cxx` bridge, `layers.rs`, the KV cache and the quantization loaders. The lint
half of the hole is easier to miss: `--all-targets` without `--workspace` does
not compile a member's *test* target either, so test-only lint errors and
test-only compile errors both passed the gate.

There is deliberately no `default-members` in `Cargo.toml` doing this instead.
It would re-scope every bare cargo invocation in the repository at once,
including the `cargo build --release --target aarch64-apple-darwin --locked`
that `release.yml` runs, which would start compiling the default-off
`mlxcel-xla` into every release build.

Each member builds at the feature set the root selects. `mlxcel-core` resolves
to `metal` and `accelerate` through the root package's forwarding, so there is
one build of it shared by every member. `mlxcel-surgery` and `mlxcel-xla`
resolve to their empty defaults; in particular `mlxcel-xla`'s `iree` feature
stays off, so its build script skips the native shim and the gate needs no IREE
distribution. The code behind `iree`, `diagnostics` and `micro-oracle` is still
outside the gate for that reason, and needs a local IREE dist to check (see
`scripts/iree/setup-macos.sh`).

`make verify-test` also passes `--no-fail-fast`, which matters only now that
the run covers four members: without it the first failing test binary ends the
run and hides the other three behind whatever failed first. Cargo still exits
non-zero, so the gate is no weaker for it.

Widening the scope does not put the run into the concurrency hazard of #1008,
where two `mlxcel-core` suites sharing one Metal device aborted 7 of 12 runs.
Cargo builds every test binary and then runs them one at a time, so the
`mlxcel-core` suite never overlaps the root suite on the device. Anything that
changes that, a parallel test runner such as `cargo nextest` for instance, has
to re-establish it: the `no_other_mlxcel_core_test_binary_is_sharing_the_gpu`
guard in `src/lib/mlxcel-core/src/gpu_exclusivity_tests.rs` detects a second
`mlxcel-core` binary, not a root-suite binary competing for the same device.

## Troubleshooting

**Missing Metal toolchain on macOS** — run
Expand Down
8 changes: 8 additions & 0 deletions src/lib/mlxcel-xla/src/emitter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ pub(crate) use model::{
};
#[allow(unused_imports)]
pub(crate) use vision::emit_vision;
// The only consumer is `IreeVisionDiagnosticProjector` in `vision_runtime`, which
// is gated on `diagnostics` alone. The `test` arm is kept deliberately: it is the
// only thing that compiles `emit_vision_impl(config, true)` in a build that does
// not enable `diagnostics`, and `diagnostics` implies `iree`, which needs an IREE
// distribution and so is never on in CI. Keeping the arm therefore means the
// diagnostic emit path is still type-checked by `cargo test`, at the price of the
// re-export having no consumer there. Same reason as the sibling blocks above.
#[allow(unused_imports)]
#[cfg(any(test, feature = "diagnostics"))]
pub(crate) use vision::emit_vision_diagnostics;
#[allow(unused_imports)]
Expand Down
2 changes: 1 addition & 1 deletion src/lib/mlxcel-xla/src/emitter/phi4_audio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ fn emit_phi4_audio_entry(
precision: Precision,
diagnostic: bool,
) -> Result<String, String> {
if config.attention_dim % config.attention_heads != 0 {
if !config.attention_dim.is_multiple_of(config.attention_heads) {
return Err("Phi4MM audio attention_dim must be divisible by heads".to_string());
}
let encoded_len = config.encoded_bucket_len(frame_bucket)?;
Expand Down
Loading
Loading