Repository navigation
Initial EV3 Support - #925
Open
Caio Rocha (caiocbr) wants to merge 5 commits into
Open
Caio Rocha (caiocbr) wants to merge 5 commits into
Caio Rocha (caiocbr) wants to merge 5 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Tiny accepted scales can overflow the precomputed reciprocal and incorrectly dequantize finite nonzero FP8 values to infinity.
3 open findings
What changed in this PR
Adds latency RANK_MAJOR static-scale FP8 support and replay-safe CUDA graph synchronization.
Changes:
- Propagates static FP8 scales through C++, bindings, and Python APIs.
- Adds FP8 dispatch/combine kernels and graph-replay rendezvous.
- Expands validation, documentation, and tests.
| File | Description |
|---|---|
src/ext/ep/latency.cc |
Validates and propagates quantization scales. |
src/ext/ep/include/moe_runtime_context.hpp |
Retains scale in dispatch handles. |
src/ext/ep/include/kernels.hpp |
Adds scale to kernel workloads. |
src/ext/ep/dispatch/rank_major_dispatch.cu |
Supports FP8 payloads and replay invalidation. |
src/ext/ep/dispatch/common.cuh |
Selects rank-major FP8 kernels. |
src/ext/ep/common/quantization.cuh |
Adds scalar FP8 dequantization. |
src/ext/ep/combine/rank_local_reduce_combine.cu |
Enables FP8 rank-local combine. |
src/ext/ep/combine/direct_send_combine.cu |
Enables FP8 direct-send combine. |
src/ext/ep/combine/common.cuh |
Implements FP8 reduction and replay synchronization. |
src/ext/ep/bindings.cpp |
Exposes quantization scale to Python. |
python/test/test_ep.py |
Tests FP8 validation, correctness, and replay. |
python/mscclpp/ep/utils.py |
Validates and resolves scalar scales. |
python/mscclpp/ep/types.py |
Extends the public quantization model. |
python/mscclpp/ep/request_builder.py |
Builds FP8 dispatch/combine requests. |
python/mscclpp/ep/README.md |
Documents FP8 and graph replay contracts. |
python/mscclpp/ep/communicator.py |
Updates combine API documentation. |
include/mscclpp/ext/ep/types.hpp |
Extends the public C++ request API. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| packed.data[0] = value; | ||
| packed.data[1] = value; | ||
| const float converted = mscclpp::to<mscclpp::f32x2>(packed).data[0]; | ||
| return converted == 0.0f ? converted : converted * dequantScale; |
Comment on lines
+152
to
+156
| /// Static FP8 quantization multiplier for latency RANK_MAJOR. | ||
| /// | ||
| /// The input is already quantized as E4M3(real * quantScale). Combine | ||
| /// dequantizes matching expert output with 1 / quantScale. | ||
| float quantScale = 1.0f; |
Comment on lines
+28
to
+30
| Latency RANK_MAJOR instead accepts externally quantized FP8 and uses the | ||
| scalar multiplier ``scale``: values are stored as E4M3 after multiplication | ||
| by ``scale`` and dequantized during combine with ``1 / scale``. |
Comment on lines
+73
to
+77
| mscclpp::f8_e4m3x2 packed; | ||
| packed.data[0] = value; | ||
| packed.data[1] = value; | ||
| const float converted = mscclpp::to<mscclpp::f32x2>(packed).data[0]; | ||
| return converted == 0.0f ? converted : converted * dequantScale; |
Contributor
There was a problem hiding this comment.
Why not use Vectorization instruction to handle more fp8 data at once
| } | ||
| } | ||
|
|
||
| MSCCLPP_DEVICE_INLINE void invalidateCountsForReplay(const TransportView& transport, int nExperts, int nRanks, |
Contributor
There was a problem hiding this comment.
Do we need this function?
Comment on lines
+168
to
+169
| def combine_fn(dout): | ||
| moe.combine(expert_output, dout[1], out=out) |
Comment on lines
+86
to
+87
| if args.cuda_graph and latency and args.iters_per_graph < 2: | ||
| raise ValueError("MSCCL++ latency graph replay requires --iters-per-graph >= 2 to avoid reusing one epoch") |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Benchmark overflow handling can strand MPI peers, and throughput kernel profiling omits required synchronization.
6 open findings
Previously missed (1)
In code that hasn't changed since last review
python/mscclpp_benchmark/ep/ep_bench_mscclpp.py:17
Match synchronizeThroughputPeersKernel in the dispatch profiler filter. Reused preparation launches this kernel before every throughput dispatch (src/ext/ep/throughput.cc:229–232), but its name contains neither dispatch nor synchronizepeers. Kineto therefore omits the required peer synchronization from dispatch and total kernel timings, overstating performance. Include synchronizethroughputpeers in the matching substrings.
🧠 Review effort: Balanced
Comment on lines
+109
to
+110
| if x.float().abs().max().item() * args.static_fp8_scale > fp8_max: | ||
| raise ValueError("--static-fp8-scale overflows the generated activations") |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Both rank-major FP8 combine modes can produce incorrect infinities for accepted small quantization scales.
7 open issues
Review effort: Balanced
| output, expertOutput, topkIndices, topkWeights, nTokens, nTopk, nExperts, nRanks, maxTokensPerRank, epoch, | ||
| transport, workspaceView, sharedMemory); | ||
| transport, workspaceView, sharedMemory, | ||
| DispatchType == DispatchDataType::FP8_E4M3 ? 1.0f / workload.quantScale_ : 1.0f); |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.