[Apple Silicon] [CI] Move the MLX lane to the check-changes + pr-gate composite - #30121
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dfb833c69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8dfb833 to
dfe0c49
Compare
|
/tag-run-ci-label |
| # signature contracts, mocked Metal capture, dummy ServerArgs patching, | ||
| # and quant-config dicts. | ||
| uv run python -m pytest -v \ | ||
| test/registered/unit/hardware_backend/mlx/test_runner_init_contract.py \ |
There was a problem hiding this comment.
Since you're here, could we revisit the design of the multi-stage CI tests? It would be great to align them with the approach used by the other backends.
Also, could we incorporate the smoke tests from #29440 into the multi-stage CI as well?
There was a problem hiding this comment.
Staged. Stage A: the existing macos-26 unit tests, unchanged. Stage B: wired the #29440 smoke tests, dispatch gated: they load 8 to 17 GB models while the hosted runners only have 7 GB. It activates once a self hosted runner exists and #29440 merges, which also fixes the stage A red on main (#29217).
Dispatch surfaced two MUSA bugs: input: for inputs:, so target_stage never registers, and missing base: main plus !cancelled().
check-changes → pr-gate → { stage-a, stage-b } → finish
|
/tag-run-ci-label |
| - "test/**/!(*.md)" | ||
| - ".github/workflows/pr-test-mlx.yml" | ||
| sgl_kernel: | ||
| - "sgl-kernel/**/!(*.md|THIRDPARTYNOTICES.txt|LICENSE)" |
There was a problem hiding this comment.
Since this workflow now has a separate sgl_kernel filter output, I think .github/workflows/pr-test-mlx.yml should be listed under sgl_kernel as well, matching the MUSA/AMD split-filter pattern.
There was a problem hiding this comment.
Added .github/workflows/pr-test-mlx.yml under the sgl_kernel filter.
| from sglang.test.ci.ci_register import register_cpu_ci, register_mlx_ci | ||
|
|
||
| register_cpu_ci(est_time=1, suite="base-a-test-cpu") | ||
| register_mlx_ci(est_time=1, suite="stage-a-unit-test-mlx") |
There was a problem hiding this comment.
Since this PR moves the MLX unit lane to run_suite.py, could we also tag the remaining model-free MLX unit tests with register_mlx_ci(..., suite="stage-a-unit-test-mlx")?
test/registered/unit/hardware_backend/mlx/test_mlx_runner_pool_contract.pytest/registered/unit/hardware_backend/mlx/test_scheduler_mixin.py
| test/registered/unit/hardware_backend/mlx/test_metal_profiler.py \ | ||
| test/registered/unit/hardware_backend/mlx/test_attention_patching.py \ | ||
| "test/registered/unit/hardware_backend/mlx/test_quantization.py::TestMlxQuantizationOverride" | ||
| test/registered/mlx/models_e2e/test_qwen2_moe_mlx_correctness.py \ |
There was a problem hiding this comment.
PR #29440 hasn't been merged yet, and this still uses the list-based mechanism. Could we switch it to the run-suite style as well?
… composite Replaces the inline mlx-label gate with the composite convention the other hardware lanes use: a dorny/paths-filter check-changes job carrying the sibling lanes' broad filter (python/sglang, test, sgl-kernel, the platform pyproject, and the workflow itself — the suite's contract tests import core srt, so narrow MLX-tree globs would miss the refactors they exist to catch), a pr-gate job calling the reusable run-ci gate, and mlx-unit-test hanging off both. The lane is now path triggered; 'labeled' leaves the trigger types since pr-gate live-fetches labels at runtime, so adding run-ci and re-running failed jobs un-skips the suite (the old frozen-payload gate never could). The workflow-level permissions block goes with the gate: an explicit block zeroes pull-requests and actions scopes, which the paths filter and the gate's API reads require; the vendor lanes running this same chain declare no block. Job names, env, steps, and the finish job are unchanged.
…ree tests into stage-a, register model-requiring tests under gated stage-b run-suite
2bf234a to
0efacae
Compare
| import sys | ||
| from typing import Dict, List, Optional | ||
|
|
||
| import tabulate |
There was a problem hiding this comment.
This workflow now calls test/run_suite.py, but the install step only installs python[srt_mps] and pytest. run_suite.py imports tabulate at module import time, and in python/pyproject_other.toml tabulate is only part of the test extra, not srt_mps.
There was a problem hiding this comment.
Fixed, the install now covers the test extra (python[srt_mps,test]), which provides tabulate and subsumes the standalone pytest line. Applied to both stages since they share the install block.
|
|
||
| stage-b-e2e-test-mlx: | ||
| needs: [check-changes, pr-gate] | ||
| # Inert until a self hosted Apple Silicon runner registers and #29440 |
There was a problem hiding this comment.
Could you refresh this comment? #29440 is merged now, and this PR registers those smoke tests under stage-b-e2e-mlx. If stage B should remain manual-only because the self-hosted Apple Silicon runner is not generally available / hosted runners are too small, the comment should say that directly instead of saying it is waiting on #29440.
There was a problem hiding this comment.
Fixed, it now states no available self-hosted Apple Silicon runner and that hosted runners too small for e2e.
…ting comment run_suite.py imports tabulate at module level, but the install step only covered python[srt_mps] plus a bare pytest; tabulate lives in the test extra of pyproject_other.toml. Extend both stage-a and stage-b install steps to python[srt_mps,test] (superset of the standalone pytest install). sgl-project#29440 merged, so the stage-b comment no longer describes the real blocker; restate it as manual-only pending a self hosted Apple Silicon runner.
…elong; constrain stage dispatch input to enum test_mlx_pool_dtype.py and test_tp_worker_routing.py were CPU-registered with MLX skip guards, so they never ran on Linux (skipped) or on the MLX lane (unregistered) -- verified green under the lane's exact env, now registered into stage-a-unit-test-mlx. test_quantization.py's TestMlxQuantization loads real HF models via MlxModelRunner; stage-a forbids downloads (HF_HUB_OFFLINE=1) and this was confirmed to fail with LocalEntryNotFoundError without a pre-warmed cache. Registered into stage-b-e2e-mlx instead, matching the models_e2e correctness tests' convention. target_stage moves from free-text to a choice enum (mirroring pr-test-amd.yml) so a typo can no longer silently no-op stage-b.
|
/tag-and-rerun-ci |
…suites pr-test-mlx.yml selects stage A tests via run_suite.py suite registration since sgl-project#30121. Register test_sliding_window_attention under stage-a-unit-test-mlx and test_gpt_oss_mlx_correctness under stage-b-e2e-mlx, mirroring the existing MLX test registrations.
pr-test-mlx.yml selects stage A tests via run_suite.py suite registration since sgl-project#30121.
pr-test-mlx.yml selects stage A tests via run_suite.py suite registration since sgl-project#30121.
pr-test-mlx.yml selects stage A tests via run_suite.py suite registration since sgl-project#30121.
pr-test-mlx.yml selects stage A tests via run_suite.py suite registration since sgl-project#30121. The macOS stage-a runner exercises the real MPS branch; elsewhere the test runs against the stub.
pr-test-mlx.yml selects stage A tests via run_suite.py suite registration since sgl-project#30121.
Motivation
#29691 landed the MLX lane (
pr-test-mlx.yml) with standalone gating. This PR began as a migration to the shared check-changes plus pr-gate composite used by the other hardware lanes. Implementations include: a separatesgl_kernelfilter output following_pr-test-check-changes.yml, and a staged layout aligned with the other backends, wired for the smoke tests from #29440. All three are in this PR, so it is a gate migration plus a staged restructure. Per @yeahdongcn's review, the scope is inclusive of moving stage A ontorun_suite.pysuite registration, matching how every other hardware lane selects its tests instead of an explicit file list.Modifications
Gate migration
check-changesjob plus the reusablepr-gateworkflow replace the inline label gate.pr-gatelive fetches labels at runtime, so addingrun-ciand rerunning failed jobs restores the suite. The old frozenpayload gate never could.
pull-requestsandactionsscopes the paths filter and the gate API reads require. The vendor lanes running this same chain declare no block.Filter split
main_packageandsgl_kernelare separate filter outputs, following_pr-test-check-changes.yml; thesgl_kernelglob is copied verbatim from that file.pr-gatekeys off achanges_existaggregate, matching the MUSA and NPU lanes.main_packageorsgl_kernel, matching the base stages ofpr-test.yml, so the trigger surface is unchanged from the single key filter.Staged layout and #29440 wiring
mlx-unit-testbecomesstage-a-unit-test-mlx. Invocation, env, and runner are unchanged: model free unit tests,HF_HUB_OFFLINE=1, GitHub hostedmacos-26.stage-b-e2e-test-mlxjob is defined for the [MLX] Add correctness tests for qwen2_moe and qwen3_moe #29440 smoke tests: served correctness for qwen2_moe and qwen3_moe plus the reference equivalence test. Those tests load 8 GB to 17 GB models and are tuned for 24 GB of unified memory; GitHub hostedmacos-26runners have 7 GB. Stage B therefore activates only through aworkflow_dispatchtarget_stageinput until a self hosted Apple Silicon runner registers. The mechanism is copied from the MUSA lane, with two corrections to make dispatch functional. Stage B currently has no scheduled execution; it fires only via the constrained dispatch input, pending the self hosted runner.workflow_dispatchwith atarget_stageinput is added for per stage dispatch; the input is a constrainedtype: choicefollowing the AMD lane (a free text field, per the MUSA original, silently no-ops on a typo).needs, fail on failure or cancellation, tolerate skipped.Test registration
python3 run_suite.py --hw mlx --suite stage-a-unit-test-mlxinstead of an explicit pytest file list, matching how the XPU lane invokesrun_suite.py. This addsHWBackend.MLXandregister_mlx_ci()to the sharedci_register.py, plus astage-a-unit-test-mlxentry inrun_suite.py's per-commit suites.test_quantization.pymixed a model-free class (TestMlxQuantizationOverride, previously cherry-picked viapytest ::ClassName) with a model-downloading class (TestMlxQuantization) in one file.run_suite.pyonly runs whole files, soTestMlxQuantizationOverridemoves to its own file,test_mlx_quantization_override.py, registered for bothbase-a-test-cpuand the new MLX suite.test_quantization.pykeeps only the model-dependent class, now registered understage-b-e2e-mlx(it performs real HF loads, so it belongs with the gated model requiring suite rather than stage-a).register_mlx_ci(...), the same pattern every other backend uses, instead of a line in the workflow's explicit list.test_mlx_pool_dtype.pyandtest_tp_worker_routing.pynow run in stage-a (verified green under the lane's exact env locally);test_quantization.pyreclassified to stage B as above.Dependencies and deferrals
register_mlx_cimarker with [Apple Silicon] [MLX] Fuse MoE combine multiply-reduce into one Metal kernel #29804 (byte identicalci_register.pyaddition; merges cleanly in either order).Accuracy Tests
CI only change; no model or kernel behavior affected. The
FakeOverlapSchedulerstub desync from #29217 (AttributeError onforward_ct) is fixed on main via #30125, and stage A is expected green against the merge commit.Speed Tests and Profiling
N/A.
CI States
Latest PR Test (Base): ❌ Run #28996751813
Latest PR Test (Extra): ❌ Run #28996751681