Repository navigation
[MoonEP] BF16 PoC integration for Kimi-K3 - #33249
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
cef0c76 to
852af75
Compare
There was a problem hiding this comment.
Thanks for putting this together. I spent some time walking through the SGLang side and comparing the integration boundary against MoonEP's dispatch / plan / prefetch / combine model.
Overall I like the direction. In particular, making MoonEP a distinct dispatch/combine format rather than trying to make it look wire compatible with DeepEP feels like the right abstraction. I also like keeping weight prefetch explicit between dispatch and expert compute; that seems like a useful boundary to preserve when the correctness first runner is eventually replaced by the production kernel.
Since this is explicitly a draft PoC, I'm intentionally not asking for grouped GEMM, CUDA graphs, quantized weights, or production memory efficiency here. The inline comments are mostly about making sure the current narrow contract fails safely instead of accidentally advertising capabilities it doesn't have yet.
One broader design thought for the next iteration: I'd be careful about using is_deepep_class_backend() as a growing capability bucket. Sharing the DeepEPMoE execution skeleton makes sense, but being on that execution branch doesn't necessarily imply MoonEP shares every DeepEP runtime capability. As this grows, capability-oriented checks such as supports_overlap, uses_per_rank_shared_slots, or supports_cuda_graph may keep the backend boundaries easier to reason about.
I also think the replicated global weight layout is reasonable for proving the first end-to-end path, as long as we keep treating it as disposable PoC infrastructure rather than the long term storage contract. The production path should ideally make MoonEP's actual expert ownership / symmetric memory layout canonical instead of allocating global SGLang weights and then building another MoonEP representation.
Finally, it would be useful to record the exact MoonEP revision used for the 8xH100 validation. I would currently interpret that result as good evidence for the synthetic BF16 MoonEP dispatch -> prefetch -> compute -> combine path, but not yet full Kimi K3 integration validation because the script bypasses the real FusedMoE weight loading/layout construction path.
Overall, promising direction. My main suggestion for this stage is to keep the supported surface narrow and explicit. If unsupported combinations fail early, the underlying storage and kernels can evolve later without having to unwind accidental compatibility assumptions.
| try: | ||
| return dist.get_rank(group=self.group) | ||
| except (AssertionError, RuntimeError, TypeError, ValueError): | ||
| return 0 |
There was a problem hiding this comment.
Could we avoid falling back to rank 0 here?
plan.experts_to_copy is rank local, and _expert_ids_from_plan() uses this value to select the current rank's row. If rank lookup fails on a nonzero rank, we would keep running but associate dispatched segments with rank 0's prefetched expert mapping. That looks like a silent correctness failure rather than a safe fallback.
I think it would be safer to let dist.get_rank(group=...) fail here and mock/inject rank explicitly in unit tests if needed.
| @@ -121,6 +121,7 @@ def create_moe_dispatcher(moe_runner_config: MoeRunnerConfig) -> BaseDispatcher: | |||
| return StandardDispatcher(moe_runner_config) | |||
| elif ( | |||
| a2a_backend.is_deepep() | |||
| or a2a_backend.is_moonep() | |||
There was a problem hiding this comment.
One concern with routing MoonEP through MaybeTboDeepEPDispatcher: when TBO is enabled, the wrapper creates two MoonEPDispatcher instances, but both appear to resolve through the same process wide MoonEPBuffer cache.
The buffer key is based on the static MoonEP dimensions + id(group), without dispatcher/in-flight identity, so two otherwise identical inner dispatchers will reuse the same underlying moonep.Buffer. That seems unsafe for overlapping sub-batches, especially since MoonEP's dispatch_a/b and combine_a/b are intentionally not implemented yet.
For this PoC, could we keep MoonEP on the single synchronous path and explicitly reject TBO/SBO at startup? Then overlap support can be added once there is an explicit ownership model for multiple in-flight MoonEP buffers/plans.
| run_moonep_bf16_expert, | ||
| StandardTopKOutput, | ||
| ) | ||
| except ModuleNotFoundError as exc: |
There was a problem hiding this comment.
Could we avoid silently switching to stubbed SGLang modules in this validation path?
My concern is about what a passing result proves: if the real SGLang import breaks, this catches the failure, installs lightweight stand ins for parts of SGLang, and can still complete the MoonEP validation.
I think both tests are useful, but they prove different things. Could we split this into a lightweight MoonEP adapter/component validation that may use stubs, and a real SGLang integration validation where an import failure is a test failure?
Ideally the latter would also exercise the actual FusedMoE weight creation/loading -> MoonEP weight-layout path, since this script currently constructs MoonEPExpertWeightLayout directly.
|
|
||
| pad_tokens = capacity - num_tokens | ||
| hidden_pad = hidden_states.new_zeros(pad_tokens, hidden_states.shape[1]) | ||
| id_pad = topk_ids.new_zeros(pad_tokens, topk_ids.shape[1]) |
There was a problem hiding this comment.
Small planning/perf question on the padding strategy: all padded routes currently get expert id 0.
Their route weights are zero, so the final numerical contribution should be neutral. But _tokens_per_expert() is computed from these padded IDs, which means the padding still contributes to MoonEP's planning statistics.
For a decode-like case such as 1 real token padded to S=128, expert 0 could therefore look artificially hot and affect migration/prefetch decisions. Could we avoid concentrating dummy routes on one expert, or otherwise ensure padding does not skew the planner? At minimum, a num_tokens << capacity regression test would be useful.
| if moonep_global_weight_storage: | ||
| self.w13_weight._sglang_require_global_experts = True | ||
| self.w2_weight._sglang_require_global_experts = True | ||
| if getattr(self, "w13_weight_bias", None) is not None: |
There was a problem hiding this comment.
Should we explicitly reject expert bias for the MoonEP PoC?
We mark the bias tensors as requiring global experts when they exist, but MoonEPExpertWeightLayout only carries gate/up/down weights and run_moonep_bf16_expert() uses the biasless F.linear(...) path. So a bias containing MoE could appear to construct/load successfully but then silently ignore those biases during expert compute.
I don't think bias support needs to be added in this PoC: a fail fast NotImplementedError when with_bias=True would make the supported contract much clearer.
|
Hi @wirybeaver, thanks for putting this up, the backend plumbing here is exactly the right shape and it made the path forward much clearer for us. Three things, all of which relax constraints this PoC had to work around:
One smaller note, following @charliechenye's "keep the supported surface narrow and explicit": that may also argue against folding moonep into Are you still actively working on this? We'd like to help push it toward a production path (MXFP4 weights, the DeepGEMM runner, and real symmetric-memory weight residency instead of per-rank full copies). Happy to contribute commits to your branch, or to open a follow-up that builds on this one — whichever you prefer. |
|
@lyppg Yeah, I am looking for this contribution and ask @charliechenye who is my Uber colleague for co-authoring. Let me touch with @Fridge003 for coordination. The stacking PR approach sounds good to me for review-ability Though I don't know which git development mode we will adopt, I already give you folks the collaborator access to my forked repo for your convenience: https://github.com/wirybeaver/sglang |
Yes I saw it, thanks. It seems you also use slack, just sent you a message, let's discuss it there |
852af75 to
7b0cdf6
Compare
a3e8612 to
7b0cdf6
Compare
7b0cdf6 to
ffdf6f9
Compare
It is opened as a draft for partial task of #32607's Integrate MoonEP, not as a final production-ready merge request.
What is included
moonepMoE A2A backend recognition.scripts/moonep/validate_moonep_bf16_poc.py.Validation
NV18) passed BF16 validation on 8 ranks withglobal_ok=true,max_abs_err=0.0,relative_err=0.0.docker-ncu-benchmarkpreflight was run. Suitable Vast.ai H100 NVSwitch hosts were container instances without Docker/NVIDIA Container Toolkit; host NCU counters were restricted (ERR_NVGPUCTRPERM), so no NCU performance claims are made.Known limitations
Notes
Local agent workflow files, scratch artifacts, and spec notes are intentionally omitted from this upstream PR diff.
Fork PoC PR: wirybeaver#17
CI States
Latest PR Test (Base): ❌ Run #31810458189
Latest PR Test (Extra): ❌ Run #31810457742