backends/mlx: runtime MoE expert-sort for decode (issue #20554)#20685
backends/mlx: runtime MoE expert-sort for decode (issue #20554)#20685AxelNoun wants to merge 2 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20685
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @AxelNoun! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
d709ef4 to
86ee91c
Compare
|
|
||
| one_const = emit_lifted_constant(P, 1, torch.int32) | ||
|
|
||
| P.emit(IdCopyNode(x=P.slot_to_tid(x_input_slot), out=P.slot_to_tid(out_slots[0]))) |
There was a problem hiding this comment.
Could we directly write to P.slot_to_tid(out_slots[i]) above and avoid these IdCopyNode?
There was a problem hiding this comment.
Done - both branches now write directly into out_slots (the flag via a 0-d FullNode), so the handlers no longer emit any IdCopyNode. The static-shape configs also fold the condition at export time, so no IfNode remains in those graphs either.
|
@claude review this code |
|
Claude finished @metascroy's task in 3m 4s —— View job Code ReviewI reviewed the diff against 🔴 Blocking: PR is built on a stale base (confirms @metascroy's suspicion)The diff deletes code that already exists on
I verified these still exist on The PR's Action: rebase onto current 🟠 Schema backward-compatibility (re: @metascroy's schema.fbs comment)Two concerns on
🟡 Comment style (repo CLAUDE.md: "minimal comments; code self-documenting")Many new comments embed
🟡
|
|
Thanks @AxelNoun! This is a good start, but it looks like it's based on an old base, so it was a little hard to review. Left some comments :) |
|
Hi ! I'll take care of this over the next few days. I still need to run the tests and refine the proposed corrections, after which I will send everything over to you |
86ee91c to
b1f941f
Compare
Let me know when it's ready for another round of review / you've answered my comment and I can take a look :) |
|
@metascroy hello ! Excuse me for the wait! As promised here are my words, thank you for your patience ! Rebased onto current Blocking (rebase) Acceptance criteria
Schema BC approach: went with the review's ask, not the issue's default B meant extending Other review points addressed
RepeatNode vs. BroadcastToNode -- checked, RepeatNode is correct What I actually validated In the process I found and fixed a real bug in the original diff: Verification steps for you (on-device, macOS + MLX)
Everything above the fold is necessary but not sufficient; none of it exercises an actual MLX kernel. |
|
@nil-is-all Hi ! Thank you very much for your guidance and for showing me the steps to get this done. I really appreciate your help! |
|
@claude review this code |
Thanks for the updates @AxelNoun! I had some comments/questions on the code. Can you respond to them (e.g., the test file you added). Thanks! |
|
Claude finished @metascroy's task in 3m 53s —— View job Code ReviewReviewed the current branch ( 🔴
|
13538a8 to
f50f2ec
Compare
|
@metascroy replied to and resolved the remaining threads. Also pushed a follow-up (f50f2ec): fixed the stale forward_raw error string in switch.py, and wired up expected_node_counts in MoeGatherInputsTest so the node-count check actually runs previously the method existed but OpTestCase never read it. Verified all expected counts (not just IdCopyNode) against the MLX lowering for all three configs locally; the full OpTestCase path (serialized .pte + on-device runner) will get its first end-to-end run in CI. |
f50f2ec to
8664e43
Compare
|
@metascroy Pushed a lint fix (8664e43): UFMT/clang-format formatting + a flake8 F811 in test_ops.py (a redundant The MLX workflows are back in |
8664e43 to
547f125
Compare
|
@metascroy Update after amending the mono-commit (new SHA: Handler fixes (real code bugs)In
Fixed by emitting Test fix (contract violation)
VerificationThe two On-device Qwen reshape error is expected to be resolved by the same |
|
@AxelNoun your tests are failing "Failed tests: gather_mm_e4_i64_o128_sorted, gather_mm_e4_i64_o128_bfloat16_sorted". Can you run the tests locally on your machine so we can be sure they pass before starting CI again? |
547f125 to
7131a13
Compare
Request: please approve / re-run CI workflows on this fork PRThis PR is from a fork ( PR is intentionally left in Draft until What this tip is (
|
| Hypothesis | Result |
|---|---|
| Writer / reader schema drift on this test | Fresh export each run; PTE wiring OK (sorted_flag const=1 → ItemInt → vid 0; rhs_indices = input tid; input order x then indices) |
Non-contiguous transpose(B) layout |
Pure-MLX Mac repro: contiguous vs transpose view both match within ~1e-6 |
sorted=True reorders output (test missing inv_order) |
False: MLX contract + test_gather_mm_sorted compare sorted vs A @ B[rhs] with no unscatter; MoE inv_order undoes input permutation from moe_gather_inputs, not a kernel output reorder |
| ET lowering / input binding bug | Unlikely: unsorted path (same graph family) passes; dump matches expected tids |
Remaining strongest explanation:
- Pre-rebase CI built vendored MLX
ce45c525(v0.31.1). - Standalone Metal repro on MLX 0.32.0 (pip + Mac CI throwaway) showed sorted ≈ unsorted ≈
A @ B[idx]for the failing shapes (including dups / transpose view). - CI signature: unsorted ≈ eager (pass), sorted ≠ eager (fail) → points at the
gather_mm_rhs/ sorted path, not at ET compare order. - Rebased tip now builds MLX v0.32.0 (same pin as
main). Iftest-mlxgoes green, the old failure was the outdated submodule pin, not this PR’s lowering.
What we need from maintainers
- Approve and run workflows for this PR (at least MLX).
- Optionally glance at
test-mlxonce it finishes:- Green → we will mark ready for review.
- Still red on
*_sortedonly → we will follow up with targeted runtime diagnostics (not schema churn).
Thanks — happy to answer questions or point at specific dumps/repros if useful.
cc @metascroy
|
@AxelNoun I approved and ran the CI tests and they're failing. The linter is also failing. Did you run locally? |
Replace the compile-time sort_experts flag with a runtime decision (sort when M > sort_cutoff) via mlx::moe_gather_inputs / mlx::moe_scatter_outputs. The FlatBuffer schema keeps the existing sorted_indices bool and appends an optional sorted_indices_flag (IntOrVid) last on GatherMmNode/GatherQmmNode, so existing .pte files stay wire-compatible; the interpreter falls back to the bool when the flag is absent. Handlers write directly into their output slots (no IdCopyNode), the flag's ItemIntNode is memoized on the builder, and static-shape exports fold both branches (no IfNode). Fixes pytorch#20554
7131a13 to
d34d58b
Compare
|
@pytorchbot label "release notes: none" |
@metascroy, I'm really sorry for all the mistakes. Please disregard my latest modifications and messages. I will pick this up tomorrow and spend the entire day resolving the PR completely. Just a heads-up: I won't be able to run the tests on Mac locally because I don't have the hardware with me right now. Thanks for your patience! |
fbcd888 to
d34d58b
Compare
No worries, @AxelNoun. Do ping us once the PR is ready for another review. |
The getitem handler copies tuple outputs into fresh slots (IdCopyNode), so a memo keyed on the producer's out_slots[2] never matched the slot consumers actually receive. Seed it under the getitem users' slots as well, so gather_mm/gather_qmm/moe_scatter_outputs resolve the flag without emitting an ItemIntNode, and static-shape exports fold the IfNode away as the test configs assert. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi @metascroy - the rework is done, and the new tests surfaced a pre-existing Where the PR standsReworked from scratch on top of current
The feature itself is fully green. What isn't: two pre-existing-path Pre-existing issue:
|
| # | Hypothesis | Result |
|---|---|---|
| 1 | b non-contiguous (transpose w/o .contiguous()) |
ruled out - run |
| 2 | index dtype (int64 misread as uint32) | ruled out, full dtype sweep - run |
| 3 | MLX version (submodule vs PyPI) | same SHA: 7a1d4f5c is v0.32.0; pip install . of it passes - run |
| 4 | lazy transpose / eval order | ruled out - run, run |
| 5 | this PR's ItemIntNode→Vid→resolve_int chain |
literal flag fails identically - run |
| 6 | input stream race | same stream; forced eval(A,B,idx) changes nothing - run |
| 7 | MLX_METAL_JIT=ON (line 215) |
flipped to OFF: fails identically (33.0089; AOT metallib 130 MB in logs) - run |
| 8 | the two local source patches | applied to pristine source, pip build: passes - run |
| 9 | MLX_BUILD_CPU=OFF (alone and with patches) |
passes - same run |
| 10 | non-default stream (ET runs on its own stream, idx 0 vs default 1) | gather_mm(sorted, stream=new_stream) passes, whole-graph-on-stream too - run |
The isolation that closes it
(run):
a 60-line C++ repro linked against the exact libmlx.a from cmake-out
(same build whose op_test_runner is red in the same job) passes at ~2e-5. A
standalone rebuild with identical CMake args and the patched source also
passes. Instrumentation inside exec_gather_mm shows A/B/rhs_idx arrive
with identical shape/dtype/strides/values and sorted resolves to 1 - yet
gather_mm(sorted=true) vs false on those same inputs, in that process,
differ by 33.
So: the artifact is healthy; the failure only manifests inside the
op_test_runner process. That leaves link/process-level causes - duplicate
symbols / ODR with something else linked into the runner, or an interaction
with process-wide Metal state - which is beyond what I can observe from
outside the backend, and squarely your territory. Repro sources and all
workflows are on my fork; happy to run anything you want there. If useful as a
first probe: an nm -gU | c++filt scan for mlx:: / MTL:: / NS:: symbols
defined outside libmlx.a among the runner's linked libs.
Severity note: the unquantized MoE prefill path (SwitchLinear →
gather_mm with sorting) is exposed to this at runtime; quantized flows
(gather_qmm) are unaffected. This predates the PR - the old compile-time
sort_experts=True had the same exposure, untested.
For this PR, your call: I can mark the two gather_mm sorted configs
xfail with a link to a tracking issue (I'm happy to file it with this full
bisection), keeping the coverage while the runtime issue is investigated - or
leave them red if you'd rather look first. Everything the PR adds is green.
|
@pytorchbot label "release notes: none" |
|
@nil-is-all It seems to be the case. After a few tries, I managed to come up with a suitable solution. Let me know what you think. Thanks for your patience ! |
Summary
Replace the compile-time
sort_experts: boolflag inSwitchMLPwith a runtime decision inside two new custom ops (moe_gather_inputs,moe_scatter_outputs). A single exported.ptenow handles both prefill (sorted, coalescedgather_mm) and decode (unsorted, no argsort overhead) without separate exports.Wire-compatible schema:
sorted_indices: boolis retained onGatherMmNode/GatherQmmNode; a new optionalsorted_indices_flag: IntOrVidfield (appended last) carries runtime 0/1 values.MLXInterpreter.hprefers the flag when present, otherwise falls back to the static bool.Serialization fix:
TakeNode.indexexpectsIntOrVidOrTid; MoE handlers now passIntOrVidOrTid.from_tid(...)instead of a rawTid(fixes export-time FlatBuffer serialization failure).MLXLoader.{h,cpp}and FlatBuffer bindings are regenerated automatically bygenerate.py+flatcduring the CMake build on Mac CI — not included in this commit, per repo convention.Test plan
python backends/mlx/test/validate_moe_20554.py(all passed)test-mlxjob onmacos-14-xlarge(run_all_tests)Fixes #20554
PR authored with Claude.
cc @metascroy