Repository navigation
fix(ci): widen the verify gate to all four workspace members - #1021
Conversation
`make verify-test` and `make verify-clippy` passed no package selector. The workspace root is itself the `mlxcel` package, so both resolved to `-p mlxcel` and neither built mlxcel-core, mlxcel-surgery or mlxcel-xla. The gate was running 76 of the repository's 84 test binaries and calling that the suite. The 8 it skipped carried 1816 tests, 1354 of them mlxcel-core's, which is the crate holding the MLX `cxx` bridge, layers.rs, the KV cache and the quantization loaders. The clippy half of the hole is the subtler one, because `--all-targets` without `--workspace` does not compile a member's test target either. Measured both ways with a `Debug`-bound error planted in an mlxcel-core test, the #973 defect class: `cargo clippy --all-targets --features metal,accelerate -- -D warnings` exits 0, and the new `make verify-clippy` exits 2 with `error[E0277]` and `could not compile mlxcel-core (lib test)`. `verify-fmt` never had the hole, since `cargo fmt --all` was already workspace-wide. Both targets take `--workspace` rather than the alternative of adding `default-members` to `[workspace]`. `default-members` re-scopes every bare cargo 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. It also moves the gate's scope into a manifest field the gate's own command line does not mention, and a scope you cannot read off the command is the defect being fixed. The Cargo.toml comment records that so the next person does not add one. `verify-test` also takes `--no-fail-fast`, which only matters 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, and compile errors stop the run either way. Feature resolution needed checking and turned out not to be a complication. `cargo tree --workspace --features metal,accelerate -e features -i mlxcel-core` shows mlxcel-core resolving 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, and mlxcel-xla's `iree` feature staying off is what keeps the gate free of an IREE distribution: its build script returns early on a missing `CARGO_FEATURE_IREE`. The code behind `iree`, `diagnostics` and `micro-oracle` is still outside the gate for that reason. Widening the scope does not walk into #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. Sampled every 0.5s across a full workspace run, all 217 samples showed exactly one test binary running, over 75 distinct binaries, with mlxcel-core's observed alone. The `no_other_mlxcel_core_test_binary_is_sharing_the_gpu` guard would not have caught the root-suite case on its own, since it only matches a second mlxcel-core binary, so the Makefile and docs record the serialization a parallel test runner would have to re-establish. Clearing the lint debt this exposes is the rest of the diff, all of it in mlxcel-xla's lib-test target and all pre-existing. One `unused_imports` on the `emit_vision_diagnostics` re-export, whose only consumer is gated on `diagnostics` alone; the `test` arm is kept and the import allowed, because that arm is the only thing type-checking `emit_vision_impl(config, true)` in a build without `diagnostics`, and `diagnostics` implies `iree`. Three `clippy::manual_is_multiple_of` on usize divisibility checks. One `clippy::identity_op` from a `48 * 64 + 0` index, replaced by a `(row, column)` accessor so the packed-bias assertions keep saying which cell they mean. One `clippy::items_after_test_module` in emitter/vision.rs, fixed by moving the test module to the end of the file with no change to its contents. mlxcel-core and mlxcel-surgery were already clean. Measured on the development machine (M1 Ultra, warm build), `--profile test-fast --features metal,accelerate --no-fail-fast`: root only is 76 binaries, 5284 passed, 1m54s; `--workspace` is 84 binaries, 7100 passed, 2m21s. Deleting exactly the 8 member test executables and re-running `--no-run` rebuilt only the three members in 17.3s with the root untouched, which is the added build cost in isolation. Treat all of these as development-machine numbers; the runner is slower and shared, and the runner measurement is a separate nightly dispatch. CONTRIBUTING.md, docs/installation.md, the PR template and the pre-push checklists quoted in ci.yml and release.yml are updated for the new scope, and nightly-verify.yml's header gains a section on which members it covers and what to do if the budget stops fitting. Closes #1007
Runner measurementRun 30881430828,
Against the original These two columns are not a controlled A/B and should not be read as one. The before column is run 30871982545 on Runner poolThis run is also the first successful one on One caveat, tracked separatelyA local full-workspace run hit |
Summary
make verify-testandmake verify-clippypassed no package selector. The workspace root is itself themlxcelpackage, so both resolved to-p mlxceland neither builtmlxcel-core,mlxcel-surgeryormlxcel-xla. Both targets now pass--workspace,verify-testalso passes--no-fail-fast, and the lint debt that surfaces behind the wider scope is cleared.Scope of the hole, measured
--workspace(new gate)The 8 skipped binaries are
mlxcel-core's lib test,mlxcel-xla's lib test, andmlxcel-surgery's lib test plus its fivetests/*_integration.rs.mlxcel-corealone carried 1354 of the missing tests, and it is the crate holding the MLXcxxbridge,layers.rs, the KV cache and the quantization loaders.--workspacerather thandefault-membersdefault-memberswould have worked, and is rejected for two reasons.It re-scopes every bare cargo invocation in the repository at once.
release.ymlrunscargo build --release --target aarch64-apple-darwin --lockedwith no selector, so adefault-memberscovering all four would start compiling the default-offmlxcel-xlainto every release build.scripts/run_quality_gate.shhas three more bare invocations.It also moves the gate's scope into a manifest field that the gate's own command line does not mention. A scope you cannot read off the command is the defect this issue is about, so hiding it in
Cargo.tomlwould be repeating the mistake somewhere new. The[workspace]stanza carries a comment recording the decision so the next person does not add one.Feature resolution
cargo tree --workspace --features metal,accelerate -e features -i mlxcel-coreresolves as follows, so this is not a complication:mlxcel(root)default(surgery) +metal+acceleratemlxcel-coremetal+accelerate, through the root's forwarding, one shared buildmlxcel-surgerydefault(empty)mlxcel-xladefault(empty);ireestays offmlxcel-xla'sireestaying off is what keeps the gate free of an IREE distribution:src/lib/mlxcel-xla/build.rsreturns early on a missingCARGO_FEATURE_IREErather than compiling the native shim. The code behindiree,diagnosticsandmicro-oracletherefore remains outside the gate, which the docs now say.Concurrency, the #1008 question
--workspaceputs themlxcel-coresuite and the root suite in one invocation, which is the arrangement that corrupts results in #1008. It does not reproduce, because cargo builds every test binary and then runs them one at a time.Sampled every 0.5s across a full workspace run: 217 of 217 samples showed exactly one test binary running, across 75 distinct binaries, with
mlxcel_core-cdfd4c46d62ff555observed alone. Doctests are not a second path to it either:mlxcel-coreexecutes 2 doctests and both are pure-CPUhardware::kv_cache_bytes*, and rustdoc merges them into one binary.The
no_other_mlxcel_core_test_binary_is_sharing_the_gpuguard would not have caught the root-suite case by itself, since it matches only a secondmlxcel-corebinary. The Makefile,docs/installation.mdandCONTRIBUTING.mdtherefore record the serialization that a parallel test runner such ascargo nextestwould have to re-establish.--no-fail-fastOnly matters 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, which for a nightly that costs the better part of an hour is the wrong trade. Cargo still exits non-zero, so the gate is no weaker, and compile errors stop the run either way.
Lint debt cleared
Six clippy errors, all pre-existing, all in
mlxcel-xla's lib-test target.mlxcel-coreandmlxcel-surgerywere already clean. The "7" in the issue thread reconciles as these six plus cargo's ownerror: could not compile mlxcel-xla (lib test) due to 6 previous errorssummary line.unused_importsemitter/mod.rs:157, theemit_vision_diagnosticsre-exportclippy::manual_is_multiple_ofemitter/phi4_audio.rs:1003,emitter/qwen2_vl.rs:227,emitter/qwen2_vl.rs:458clippy::identity_opemitter/qwen2_vl.rs:827, a48 * 64 + 0index in a testclippy::items_after_test_moduleemitter/vision.rs:35The unused import is allowed rather than deleted, and the
#[cfg(any(test, feature = "diagnostics"))]gate kept, because thattestarm is the only thing type-checkingemit_vision_impl(config, true)in a build withoutdiagnostics, anddiagnosticsimpliesiree, which needs an IREE dist and is therefore never on in CI. The four sibling re-export blocks above it already carry the same attribute for the same reason. Theidentity_opis fixed with a(row, column)accessor so the packed-bias assertions keep saying which cell they mean instead of degrading to a bare48 * 64. Theitems_after_test_modulefix moves the test module to the end ofemitter/vision.rswith no change to its contents.Timings, development machine
M1 Ultra, warm build,
--profile test-fast --features metal,accelerate --no-fail-fast. These are development-machine numbers; the runner is slower and shared, and its measurement is a separate nightly dispatch that this PR does not trigger.cargo test --profile test-fast --features metal,accelerate --no-fail-fastcargo test --profile test-fast --workspace --features metal,accelerate --no-fail-fastAdded build cost, isolated: deleting exactly the 8 member test executables and re-running
--no-runrebuilt only the three members, root untouched, in 17.3s. A no-op--no-runis 0 to 1s at either scope.So on a warm tree the whole change costs about 44s, split roughly 17s of build and 27s of execution. The runner's 48m54s is dominated by the root build, which is unchanged; the new members add 8 link steps out of 84.
nightly-verify.yml's header records what to do if the budget ever stops fitting: splitverify-testinto per-member steps before dropping members from it.Test plan
make verify-fmt: pass (4s)make verify-clippy: pass (22s), workspace clean at-D warningsmake verify-test: pass (217s), 84 binaries + 4 doctest targets, 7100 passed, 0 failed, 290 ignoredmlxcel_core-…,mlxcel_surgery-…,mlxcel_xla-…and the fivemlxcel-surgeryintegration binaries, plusDoc-testsfor all four membersmlxcel-core's observed aloneDebug-bound error planted in anmlxcel-coretest, the fix(core): bound the declared quantization mode string, not just group_size and bits #973 defect class, then reverted:cargo clippy --all-targets --features metal,accelerate -- -D warnings: exit 0, passes with the error presentmake verify-clippy: exit 2,error[E0277]: NotDebug doesn't implement std::fmt::Debugatsrc/lib/mlxcel-core/src/streams.rs:289,could not compile mlxcel-core (lib test)python3 scripts/ci/check_cross_repo_refs.pyclean; every#Nin the diff verified againstlablup/mlxcelmodels/path written, moved or read for writeNot applicable: no inference code changes, so no real-checkpoint validation. The 84-binary suite includes the model parity tests, which self-skip without weights.
Notes for reviewers
CLAUDE.mdis a symlink toAGENTS.mdand both are gitignored, so the equivalent update to the local agent contract is not in this diff.The known-flaky
granite4_vision_parityandernie4_5_moe_vl_paritytests (#997) passed in all three full runs here.Closes #1007