[MSCCLPP-EP] Add C++ latency expert-parallel runtime - #895
Binyang Li (Binyang2014) wants to merge 13 commits into
Conversation
Add the public MoERuntime request API, latency dispatch/combine host runtime, and expert-major and rank-major CUDA kernels as a standalone mscclpp_ep library that does not depend on Python bindings. Preserve the unified throughput-facing API surface while rejecting throughput mode until its follow-up implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Callers can derive the topology distinction from isAvailable(), numRanks(), and numNvlRanks(); keep the initial C++ latency surface focused on the capability check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove throughput-only prepare and notify methods and strip throughput request fields while retaining empty request types as placeholders for the follow-up implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the duplicate EP CUDA_CHECK macro with MSCCLPP_CUDATHROW and remove the unused throughput-only extended launch helper from the latency implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unused device helpers, align helper naming and inline annotations with MSCCL++ conventions, cap latency routing at top-8, and keep rank-major combine on its TMA path without the unreachable fallback state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cover latency runtime initialization, unsupported throughput requests, expert-major and rank-major dispatch/combine correctness across BF16 and FP8, and CUDA-graph D+C performance for 32 tokens per rank. Remove dead and redundant runtime state found during the kernel review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use 100 graph replays to reduce variance in the expert-major and rank-major 32-token D+C measurements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename the mode-owned host context types to LatencyRuntimeContext and ThroughputRuntimeContext, and clarify the combine input buffer documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Rename the leader-only destination variable from completionRank to dstRank so the rank-major token-store path is easier to follow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run mscclpp-ut |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Keep dispatch metadata opaque, derive fixed request configuration from the runtime, and use EP-local exceptions with caller-managed GPU lifetimes. Update EP tests for randomized top-8 routing with 16 experts per GPU and weighted rank-major benchmark inputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| MoEMode mode_; | ||
| int rank_; | ||
| int numRanks_; | ||
| int numNvlRanks_; |
There was a problem hiding this comment.
Could we rename numNvlRanks_ to numRanksPerNode_? It is derived from getNranksPerNode(), so the current name implies an NVLink topology property that is not actually established here. We could potentially remove this field instead, since the current latency implementation uses numRanksPerIpcDomain_ for its communication-domain and availability decisions.
| DispatchHandle launchLatencyDispatch(const LatencyDispatchRequest& request); | ||
| void launchLatencyCombine(const LatencyCombineRequest& request); | ||
|
|
||
| std::shared_ptr<mscclpp::Bootstrap> bootstrap_; |
There was a problem hiding this comment.
Since we pass the communicator through the constructor, wouldn’t it make sense to retain it directly? I realize that most of the time we essentially use only the bootstrap, but in other codebases we generally keep the communicator rather than retaining only the bootstrap.
| /// | ||
| /// The caller must keep all buffers referenced by this struct (inputs and | ||
| /// outputs) valid until the corresponding combine operation has finished. | ||
| struct LatencyDispatchRequest { |
There was a problem hiding this comment.
Would it be clearer and safer to split this into layout-specific request types (for example, ExpertMajorDispatchRequest and RankMajorDispatchRequest) and make LatencyDispatchRequest a variant or shared/common portion? The current struct permits invalid combinations and relies on runtime assertions even though several fields are mutually exclusive:
- Expert-major only:
outputScales(FP8 only),outputSrcInfo, andoutputLayoutRange. - Rank-major only:
outputTopkIdx,outputTopkWeights, andinvalidTokenExpertId. - Both layouts:
output,outputCount,input,topkIdx,topkWeights,numTokens,maxTokensPerRank,dispatchDataType,numBlocks, andstream; however,outputCounthas layout-dependent indexing, and FP8dispatchDataTypeis currently supported only by expert-major.
Separate request types would make these requirements explicit in the type system, document the different output shapes, and avoid exposing fields that callers must set to nullptr for the selected layout.
| /// Requested dispatch payload format. | ||
| DispatchDataType dispatchDataType; | ||
| /// Dispatch grid block count. | ||
| int numBlocks; |
There was a problem hiding this comment.
Does numBlocks need to be part of every dispatch request? This looks like an implementation-specific tuning parameter whose valid and optimal value depends on the selected algorithm, device occupancy, rank count, and shared-memory use. Could the runtime/algorithm choose a standard value by default (for example, treating 0 as automatic), while retaining an optional override for benchmarking or advanced tuning? That would keep ordinary callers from needing to understand the kernel's control-block and all-block-residency constraints.
| /// Handle returned by the matching dispatch. | ||
| DispatchHandle handle; | ||
| /// Combine grid block count. | ||
| int numBlocks; |
There was a problem hiding this comment.
Similarly, should combine block count default to an algorithm-selected value rather than being mandatory in every request? A default such as 0 meaning automatic would let the selected combine implementation derive a safe value from occupancy, layout, and device properties, while still allowing an explicit override for performance tuning.
| namespace mscclpp { | ||
| namespace ep { | ||
|
|
||
| constexpr int DispatchNWarps = 16; |
There was a problem hiding this comment.
Could you clarify why dispatch uses 16 warps (512 threads) rather than the maximum 32 warps (1024 threads) per block? Is this an intentional performance choice based on benchmarking—for example, to balance register/shared-memory pressure, block residency, or TMA throughput? Using fewer threads can certainly perform better here, but it would be helpful to document the rationale and which workload or GPU configurations were used to select 16.
| #define EP_STATIC_ASSERT(cond, reason) static_assert(cond, reason) | ||
| #endif | ||
|
|
||
| class EPException : public std::exception { |
There was a problem hiding this comment.
Do we need an EP-specific exception type and this separate exception header? MSCCL++ already has the public mscclpp::Error hierarchy with typed ErrorCode values, along with MSCCLPP_CUDATHROW, MSCCLPP_CUTHROW, and MSCCLPP_ASSERT_DEVICE. It may be more consistent to use mscclpp::Error(..., ErrorCode::InvalidUsage) for invalid requests and ErrorCode::InternalError for failed internal invariants, keeping only minimal EP helper macros if needed. This would also avoid the current public-API mismatch where public methods document EPException, but that type is defined only in this private src header and therefore cannot be caught by users through the installed public headers.
| EP_HOST_ASSERT(outputTopkWeights == allocationLayout.rankMajorTopkWeightsBuffer_); | ||
| } | ||
|
|
||
| const Workload workload{.epoch_ = context.epoch_ + 1, |
There was a problem hiding this comment.
Should we update the context epoch here—or perhaps use an atomic add—to prevent two kernels from being called with the same epoch if one don't update before the other arrives?
Summary
MoERuntimeAPI and latency dispatch/combine request typesmscclpp_epshared library with no Python dependencyScope
ThroughputDispatchRequestandThroughputCombineRequesttypes reserve names for the follow-up implementationprepare,notify, host runtime, and CUDA kernels are intentionally excludedC++ test coverage
Validation
./tools/lint.shmscclpp_epbuilds forMSCCLPP_GPU_ARCHS=90andnativemscclpp_epandmscclpp_pylibmscclpp_ep.soand the public EP headersmpirun -np 8 ./build-ep-tests/bin/mp_unit_tests --filter=MoERuntimeTest: 4/4 passedFeature/EP regression comparison
feature/epaverages: expert-major 50.58 us, rank-major 41.21 us