Repository navigation
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> # Conflicts: # python/sglang/kernels/ops/moe/moe_sorting_small.py
This comment has been minimized.
This comment has been minimized.
rkarhila-amd
left a comment
There was a problem hiding this comment.
Please address the comments. Patching things gets perf quickly, but overriding the aiter behaviour is bad in a long run. Division of general and code-specific code is also not typical for this project.
In addition, please put the perf and accuracy numbers in tables so that human eyes have an easier time inspecting them quickly.
Lint seems to fail, but you need to update the code to latest anyway.
Also, Kimi-K3.1 is just out, so check if these changes are relevant for 3.1
|
|
||
|
|
||
| @dataclass | ||
| class K3SortPlan: |
There was a problem hiding this comment.
Please reconsider the ownership of the K3 plan and dispatch.
The k3_* Triton kernels belong here in kernels/ops/moe, and the K3 model should orchestrate them.
moe_sorting_small.py is shared, yet here it contains K3SortPlan, a K3-specific context variable, route-state handoff, and the shared-expert GEMM.
Could the generic sorter expose a model-agnostic sort-extension hook, with the K3 plan and implementation kept in a k3_* module? This preserves the generic sorter’s model independence while still allowing the Aiter-internal sort boundary to invoke the K3 fast path.
There was a problem hiding this comment.
Done in b56a003. moe_sorting_small.py is model-agnostic again: it exposes a SortExtension hook, scoped with sort_extension(...), at the sort boundary AITER calls inside fused_moe. With no extension set, behavior is unchanged. K3SortPlan, the route-state handoff and the deferred shared-expert GEMM moved to kernels/ops/moe/k3_sort_plan.py. kimi_k3.py routes with k3_route_sort_mid.route_quant and runs the experts under k3_sort_plan(plan).
|
|
||
| @functools.cache | ||
| def apply_k3_moe_decode_cfg() -> None: | ||
| try: |
There was a problem hiding this comment.
Avoid try-excepts, they can make debugging more difficult (if anyone still makes that by hand).
The sglang images are built with specific releases of aiter, trust the correct version to be there. If the current version of aiter is not yet in sglang docker builds, wait for aiter release to be updated before pushing this PR to be merged.
There was a problem hiding this comment.
Done in 97c4f52: the AITER imports are direct, with no try/except fallbacks. The PR relies on the AITER that ships in the image.
| @@ -0,0 +1,113 @@ | |||
| """K3SortPlan: the model-owned inputs of the K3 sort kernels inside aiter fused_moe.""" | |||
There was a problem hiding this comment.
My agent advices to reconsider the test plan:
The new behavior crosses K3 routing, the patched AITER sorter, MXFP8 quantization, shared-expert execution, and the MoE runner. The listed tests are predominantly unit-level.
There was a problem hiding this comment.
Added test/registered/amd/test_k3_moe_sort_plan_e2e.py (c0e75fb). It runs K3 routing, the K3 sort plan, MXFP8 quant, the shared expert and AiterRunnerCore end to end, at the K3/K3.1 TP8 MoE shapes (896 experts, top-k 16, M=1..32), against an fp32 reference. It passes on MI355X: 2 tests, 10 subtests. With the rest of the K3 MoE GPU tests: 23 pass, 211 subtests.
There was a problem hiding this comment.
With end-to-end test I kindly ask removing some of the overlapping tests:
| Test | Verdict | Reason |
|---|---|---|
| test_k3_sort_plan_keeps_small_sort_at_513_experts | Remove | Same gate the M=8/12/16 cases already require: with a plan active, _small_sort_supported stays true past the expert-count cutoff. Those cases also require the kernel to take the GEMM. This one only reads the boolean, at E=513. |
| test_exit_runs_untaken_gemm in test_k3_sort_plan.py | Remove | Mocks shared_gemm_preact. test_untaken_shared_gemm_matches_standalone runs that same exit on a GPU and checks it against the direct kernel and against torch. |
There was a problem hiding this comment.
Removed both in 56c66e8: test_k3_sort_plan_keeps_small_sort_at_513_experts and test_exit_runs_untaken_gemm. The e2e test and the M=8/12/16 plan cases keep that coverage.
| (M=16: ~139 vs 224), and at that shape the tuned M=16 stage1 (``_xcd4`` swizzle) is ~11 us | ||
| slower than the plain variant. These rows were re-tuned on real K3 routing: | ||
| same activation/weight quantization, same layouts, only the | ||
| tile schedule changes, so outputs match the default kernels to fp32-reference noise. |
There was a problem hiding this comment.
I appreciate the effort and I can easily believe the statistics from dspark are different from the (probably) random sampling data data that is used in a lot of the tunings.
However patching the tuning table to me seems a very bad idea and I would recommend solving this in Aiter side, maybe having alternative speculative decoding tuning tables there.
There was a problem hiding this comment.
Agreed. The runtime table patch is removed (k3_moe_cfg.py is deleted in d90bd7c). The retuned token 8/16 rows are in ROCm/aiter#7481, with a before/after table on DFlash-shaped and uniform routing (M=16 DFlash: 77.4 to 68.7 us; uniform: 100.9 to 98.6 us). Until that lands in the image, K3 uses AITER's current rows and stays correct.
…K3 plan to k3_sort_plan Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… with try/except Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> # Conflicts: # python/sglang/kernels/ops/moe/__init__.py
…ve in AITER Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
rkarhila-amd
left a comment
There was a problem hiding this comment.
Looks much better, thank you. A few tests seem to be overlapping, please consider removing those.
#44987 seems to be on collision course with this one but is at draft state currently.
|
|
||
| @functools.cache | ||
| def apply_aiter_small_moe_sort_patch() -> bool: | ||
| """Patch a stock aiter so decode-sized MoE sorting (+ stage1 mxfp8 quant) is one launch. |
There was a problem hiding this comment.
Understood, thanks. This PR no longer patches AITER: the runtime decode-table patch is gone (the rows are in ROCm/aiter#7481), and the AITER imports are direct. The remaining hook here is the model-agnostic SortExtension at the sort boundary, which does nothing unless a model sets it.
| @@ -0,0 +1,113 @@ | |||
| """K3SortPlan: the model-owned inputs of the K3 sort kernels inside aiter fused_moe.""" | |||
There was a problem hiding this comment.
With end-to-end test I kindly ask removing some of the overlapping tests:
| Test | Verdict | Reason |
|---|---|---|
| test_k3_sort_plan_keeps_small_sort_at_513_experts | Remove | Same gate the M=8/12/16 cases already require: with a plan active, _small_sort_supported stays true past the expert-count cutoff. Those cases also require the kernel to take the GEMM. This one only reads the boolean, at E=513. |
| test_exit_runs_untaken_gemm in test_k3_sort_plan.py | Remove | Mocks shared_gemm_preact. test_untaken_shared_gemm_matches_standalone runs that same exit on a GPU and checks it against the direct kernel and against torch. |
|
@rkarhila-amd on your Kimi-K3.1 question: I ran it on a Kimi-K3.1 Pro TP8 MI355X deploy (our K3.1 bringup config) with this feature on and off.
So it applies to K3.1 unchanged: about +4.6% at C=1 and +2.1% at C=8 on the fixed-length workload, with no accuracy change and 0 failed requests. The agentic replay numbers are in the description; they are within run-to-run noise. The shape gates are the same for both models, so there's no K3.1-specific code. |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@rkarhila-amd on #44987 (draft, @kevin-mii): it overlaps with this PR in the K3 decode routing in |
rkarhila-amd
left a comment
There was a problem hiding this comment.
Thanks for the modifications. I don't see any blocking problems so I'll pass it on,
| # stable rank: pairs of earlier tokens on the same expert | ||
| rank = tl.zeros((TOPK,), tl.int32) | ||
| n_prev = t * TOPK | ||
| for s in range(0, n_prev, RANK_TILE): |
There was a problem hiding this comment.
My agent tried to convince me that a standard python range would crash the compiler and insisted on tl.arange.
But in practice this compiled well so no issue.
| ids, w = self._ref_topk(logits) | ||
| with k3_sort_plan(): | ||
| out = self._routed(x, ids, w) | ||
| self._assert_rel(out, self._routed(x, ids, w), 1e-2, "routed") |
There was a problem hiding this comment.
Agent raised a concern:
Both sides of the comparison call self._routed(...), which uses the same AITER/K3 implementation. Therefore, if the implementation produces an incorrect result consistently, both calls can produce the same incorrect result and the test still passes.
A stronger test would compare against an independent reference, such as:
a PyTorch fp32 MoE implementation;
AITER with the K3 optimization disabled;
independently computed expected sorting/permutation and quantization outputs.
I'm satisfied that you test the k3_sort_plan wrapper, up to you if you want to follow this.
Motivation
At K3 decode batch sizes, MoE routing, sorting, activation quant and the shared-expert epilogue were separate small launches, and launch-bound.
Modifications
moe_sorting_small.pystays model-agnostic. It exposesSortExtension, a hook for the sort AITER runs insidefused_moe, scoped with thesort_extension(...)context manager. With no extension set, behavior is unchanged.k3_*module.k3_sort_plan.pyholdsK3SortPlan(SortExtension): the route-state handoff and the deferred shared-expert GEMM. It calls the K3 Triton kernels ink3_route_sort,k3_route_sort_midandk3_moe_epi.kimi_k3.pyroutes withk3_route_sort_mid.route_quantwhen the shape gate passes, then runs the experts underk3_sort_plan(plan).Gates are on shapes and gfx950, not on the model name. Kimi-K3.1 has the same MoE shapes at TP8 (896 experts, top-k 16, hidden 3584, per-rank intermediate 384, SiTU beta 4 / linear beta 25), so these paths apply to it too. Validated on a Kimi-K3.1 Pro TP8 deploy below.
Accuracy Tests
test_k3_moe_sort_plan_e2e.py(new, MI355X): routing, sort, MXFP8 quant, shared expert and the AITER MoE runner vs an fp32 reference, K3/K3.1 TP8 shapes, M=1..32test_moe_sorting_small.py,test_k3_sort_plan*.py, K3 MoE kernel tests (MI355X)Full K3 stack on MI355X TP8 (DFlash, prod config) vs B300, McNemar:
Kimi-K3.1 Pro on MI355X TP8 (DFlash, K3.1 bringup config), GSM8K (1319), this feature on vs off:
Speed Tests and Profiling
Throughput change from turning this feature off, full K3 stack, MI355X TP8:
Mean -6.2%.
Kimi-K3.1 Pro, MI355X TP8, output tok/min/GPU, one run per cell (0 failed requests):
The agentic replay does not pin output lengths, and two deploys of identical code differed by up to 23% at C=8 on K3, so the agentic rows are within noise; the fixed-length 90k prefix rows vary by under 1% between runs.
Checklist
CI States
Latest PR Test (Base): ❌ Run #42072711749
Latest PR Test (Extra): ❌ Run #42072711709
Latest PR Test (AMD ROCm 10): ❌ Run #42072711701
Tip
Base CI only covers the core cases, and its workload may not exercise your change. Use
/rerun-testto run the tests your change touches, or copy this prompt for an agent: