-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[Refactor] Rename n_share_experts_fusion
as num_fused_shared_experts
#6735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ch-wan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR primarily focuses on a refactor to improve code clarity by renaming the variable and parameter n_share_experts_fusion
to num_fused_shared_experts
. This change is applied consistently across the codebase, including Python modules, C++ kernels, benchmark scripts, and documentation, wherever this parameter is used to control the number of shared experts fused with normal experts in MoE models, particularly Deepseek V3/R1.
Highlights
- Variable/Parameter Rename: The core change is renaming
n_share_experts_fusion
tonum_fused_shared_experts
for better readability and understanding of its purpose. - Codebase Wide Application: The renaming is applied across various parts of the codebase, including Python source files, C++ kernel code, benchmark scripts, and documentation.
- Benchmark Script Updates: Benchmark scripts (
tuning_fused_moe_triton.py
,benchmark_vllm_vs_sglang_fused_moe_triton.py
) and their README are updated to reflect the new parameter name and in some cases, remove the explicit parameter from command line examples, suggesting it might be automatically determined. - Kernel Code Updates: The C++ kernel code (
moe_fused_gate.cu
,common_extension.cc
,sgl_kernel_ops.h
) and its Python wrapper (moe.py
) are updated to use the new parameter name in function signatures and internal logic. - Test Parameter Reduction: In
sgl-kernel/tests/test_moe_fused_gate.py
, the range of tested values for the renamed parameter is reduced from[0, 1, 8, 16]
to[0, 1]
. This might indicate a change in the test coverage or supported values for this specific test case.
Changelog
Click here to see the changelog
- benchmark/kernels/fused_moe_triton/README.md
- Updated command line examples to use
--num-fused-shared-experts
instead of--n-share-experts-fusion
. - Removed the explicit
--num-fused-shared-experts
parameter from some examples.
- Updated command line examples to use
- benchmark/kernels/fused_moe_triton/benchmark_vllm_vs_sglang_fused_moe_triton.py
- Removed the
n_share_experts_fusion
parameter fromget_model_config
function signature (L21). - Removed the logic using
n_share_experts_fusion
to calculateE
(L46-L50). - Removed the
--n-share-experts-fusion
argument from the argument parser (L297). - Updated the call to
get_model_config
to no longer pass the removed parameter (L327).
- Removed the
- benchmark/kernels/fused_moe_triton/tuning_fused_moe_triton.py
- Removed the logic using
args.n_share_experts_fusion
to calculateE
for Deepseek models (L402-L406). - Removed the logic using
args.n_share_experts_fusion
to calculateE
for Llama4 (L411). - Removed the
--n-share-experts-fusion
argument from the argument parser (L573-L578).
- Removed the logic using
- python/sglang/srt/layers/moe/topk.py
- Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
ingrouped_topk
(L106). - Updated usage of the parameter
n_share_experts_fusion
tonum_fused_shared_experts
withingrouped_topk
(L131, L134, L144). - Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
inbiased_grouped_topk_impl
(L163). - Updated usage of the parameter
n_share_experts_fusion
tonum_fused_shared_experts
withinbiased_grouped_topk_impl
(L195, L198, L208). - Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
inbiased_grouped_topk
(L242). - Updated comment regarding kernel support to use
num_fused_shared_experts
(L250). - Updated usage of the parameter
n_share_experts_fusion
tonum_fused_shared_experts
withinbiased_grouped_topk
(L263, L291). - Updated retrieval from
global_server_args_dict
to use key"num_fused_shared_experts"
(L313). - Updated usage of the parameter
n_share_experts_fusion
tonum_fused_shared_experts
in calls to internal functions (L335, L349).
- Renamed parameter
- python/sglang/srt/managers/schedule_batch.py
- Updated key in
global_server_args_dict
mapping fromServerArgs
to use"num_fused_shared_experts"
(L90).
- Updated key in
- python/sglang/srt/model_executor/model_runner.py
- Updated key in
global_server_args_dict
initialization to use"num_fused_shared_experts"
(L210).
- Updated key in
- python/sglang/srt/models/deepseek_nextn.py
- Renamed method
determine_n_share_experts_fusion
todetermine_num_fused_shared_experts
(L125).
- Renamed method
- python/sglang/srt/models/deepseek_v2.py
- Renamed instance variable
self.n_share_experts_fusion
toself.num_fused_shared_experts
(L225). - Updated usage of the variable in
__init__
(L245, L247, L266). - Updated usage of the variable in
_forward_shared_experts
(L419). - Updated usage of the variable in
op_shared_experts
(L435). - Renamed method
determine_n_share_experts_fusion
todetermine_num_fused_shared_experts
(L1649). - Updated usage of the variable and dictionary key within
determine_num_fused_shared_experts
(L1666, L1667, L1674, L1675, L1682, L1684, L1692, L1693). - Updated usage of the variable in
load_weights
(L1881, L1916, L1923, L1942).
- Renamed instance variable
- python/sglang/srt/server_args.py
- Renamed class attribute
n_share_experts_fusion
tonum_fused_shared_experts
(L207). - Renamed command line argument
--n-share-experts-fusion
to--num-fused-shared-experts
(L1362). - Updated help text for the command line argument to use the new name (L1366).
- Renamed class attribute
- sgl-kernel/csrc/common_extension.cc
- Updated function signature definition for
moe_fused_gate
to usenum_fused_shared_experts
(L150).
- Updated function signature definition for
- sgl-kernel/csrc/moe/moe_fused_gate.cu
- Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
inmoe_fused_gate_impl
(L60). - Updated usage of the parameter within
moe_fused_gate_impl
(L71, L225, L229). - Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
inmoe_fused_gate_kernel
(L276). - Updated usage of the parameter within
moe_fused_gate_kernel
(L287, L308). - Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
inmoe_fused_gate_kernel_dynamic
(L336). - Updated usage of the parameter within
moe_fused_gate_kernel_dynamic
(L354, L467, L480, L493). - Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
inmoe_fused_gate
C++ function (L368).
- Renamed parameter
- sgl-kernel/include/sgl_kernel_ops.h
- Updated function signature declaration for
moe_fused_gate
to usenum_fused_shared_experts
(L209).
- Updated function signature declaration for
- sgl-kernel/python/sgl_kernel/moe.py
- Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
in the Python wrapper functionmoe_fused_gate
(L43). - Updated docstring to use
num_fused_shared_experts
(L52). - Updated call to the C++ kernel to pass
num_fused_shared_experts
(L60).
- Renamed parameter
- sgl-kernel/tests/test_moe_fused_gate.py
- Renamed parameter
n_share_experts_fusion
tonum_fused_shared_experts
intest_moe_fused_gate_combined
function signature (L23). - Reduced the tested values for the parameter from
[0, 1, 8, 16]
to[0, 1]
(L22). - Updated usage of the parameter within the test function (L30, L38, L50, L55, L63).
- Updated assertion messages to use the new parameter name (L90, L94).
- Renamed parameter
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A name was changed, a simple deed,
For clarity, a vital need.
From 'n_share' to 'num_fused' clear,
Making the code less hard to steer.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully renames n_share_experts_fusion
to num_fused_shared_experts
across the codebase, which improves clarity. However, there are several accompanying functional changes, particularly in benchmark scripts and test coverage, that are not explained in the PR and need clarification. The PR title might be an understatement given these changes.
Specifically:
- Benchmark scripts (
benchmark_vllm_vs_sglang_fused_moe_triton.py
andtuning_fused_moe_triton.py
) no longer use the--num-fused-shared-experts
CLI argument. Instead, theE
parameter calculation within theirget_model_config
helper seems to have changed (e.g., using a hardcoded+ 1
for DeepSeekV3 models). This needs justification as it could alter benchmark configurations. - The test coverage for
num_fused_shared_experts
insgl-kernel/tests/test_moe_fused_gate.py
has been reduced from[0, 1, 8, 16]
to[0, 1]
. Given thatnum_fused_shared_experts
can be set totp_size
(which can be > 1) by the model's internal logic, this reduction might miss potential issues for larger values. - There's a TODO comment in
python/sglang/srt/layers/moe/topk.py
stating themoe_fused_gate
kernel doesn't supportnum_fused_shared_experts > 0
, which seems to conflict with the kernel's implementation and previous test coverage. Clarification on this would be helpful.
Overall, the renaming is well-executed, but the functional changes require explanation to ensure their correctness and intent are understood.
Summary of Findings
- Mismatch between PR Title and Actual Changes: The PR title indicates a rename, but there are functional changes in benchmark configurations and test coverage that are not explained.
- Benchmark Configuration Changes: The method for calculating the
E
parameter in benchmark scripts has been altered (hardcoded+1
), and the relevant CLI argument has been removed from these scripts. The impact and rationale for this need clarification. - Reduced Test Coverage: Test parameters for
num_fused_shared_experts
intest_moe_fused_gate.py
have been reduced, potentially leaving cases wherenum_fused_shared_experts > 1
(e.g., equalstp_size
) untested for themoe_fused_gate
kernel. - Potentially Outdated TODO Comment: A TODO comment in
topk.py
regardingmoe_fused_gate
kernel's support fornum_fused_shared_experts > 0
seems to conflict with the kernel's implementation details.
Merge Readiness
The renaming aspect of this PR is well-implemented. However, due to the unexplained functional changes in benchmark configurations and the reduction in test coverage, I recommend addressing these points before merging. Specifically, providing context for these changes and ensuring adequate test coverage for num_fused_shared_experts
when it can take values greater than 1 (like tp_size
) would be crucial. I am unable to approve this PR myself; please ensure other reviewers approve it after these concerns are addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@ch-wan Need rebase main. |
Merge branch 'sgl_20250610_sync_tag047 of git@code.alipay.com:Theta/SGLang.git into main https://code.alipay.com/Theta/SGLang/pull_requests/52 Reviewed-by: 剑川 <jianchuan.gys@antgroup.com> * [Bugfix] Fix slice operation when chunk size mismatch (sgl-project#6697) * [Bugfix] Fix ChatCompletion endpoint of mini_lb when stream is set (sgl-project#6703) * [CI] Fix setup of disaggregation with different tp (sgl-project#6706) * [PD] Remove Unnecessary Exception Handling for FastQueue.get() (sgl-project#6712) * Fuse routed_scaling_factor in DeepSeek (sgl-project#6710) * Overlap two kernels in DeepSeek with communication (sgl-project#6711) * Minor refactor two-batch overlap (sgl-project#6682) * Speed up when having padding tokens two-batch overlap (sgl-project#6668) * [Feature] Support Flashinfer fp8 blockwise GEMM kernel on Blackwell (sgl-project#6479) * Fix LoRA bench (sgl-project#6719) * temp * Fix PP for Qwen3 MoE (sgl-project#6709) * [feat] triton kernel for get_last_loc (sgl-project#6676) * [fix] more mem for draft_extend cuda_graph (sgl-project#6726) * [PD] bug fix: Update status if nixl receiver send a a dummy req. (sgl-project#6720) * Tune memory arguments on B200 (sgl-project#6718) * Add DeepSeek-R1-0528 function call chat template (sgl-project#6725) * refactor(tool call): Fix BaseFormatDetector tool_index issue and refactor `parse_streaming_increment` (sgl-project#6715) * Add draft extend CUDA graph for Triton backend (sgl-project#6705) * refactor apply_w8a8_block_fp8_linear in fp (sgl-project#6545) * [PD] Support completion endpoint (sgl-project#6729) * PD Rust LB (PO2) (sgl-project#6437) * Super tiny enable sole usage of expert distribution metrics and update doc (sgl-project#6680) * Support picking variants of EPLB algorithms (sgl-project#6728) * Support tuning DeepEP configs (sgl-project#6742) * [test] add ut and bm for get_last_loc (sgl-project#6746) * Fix mem_fraction_static for AMD CI (sgl-project#6748) * [fix][RL] Fix DeepSeekV3ForCausalLM.post_load_weights for multiple update weight (sgl-project#6265) * Improve EPLB logical to physical dispatch map (sgl-project#6727) * Update DeepSeek-R1-0528 function call chat template (sgl-project#6765) * [PD] Optimize time out logic and add env var doc for mooncake (sgl-project#6761) * Fix aiohttp 'Chunk too big' in bench_serving (sgl-project#6737) * Support sliding window in triton backend (sgl-project#6509) * Fix shared experts fusion error (sgl-project#6289) * Fix one bug in the grouped-gemm triton kernel (sgl-project#6772) * update llama4 chat template and pythonic parser (sgl-project#6679) * feat(tool call): Enhance Llama32Detector for improved JSON parsing in non-stream (sgl-project#6784) * Support token-level quantization for EP MoE (sgl-project#6782) * Temporarily lower mmlu threshold for triton sliding window backend (sgl-project#6785) * ci: relax test_function_call_required (sgl-project#6786) * Add intel_amx backend for Radix Attention for CPU (sgl-project#6408) * Fix incorrect LoRA weight loading for fused gate_up_proj (sgl-project#6734) * fix(PD-disaggregation): Can not get local ip (sgl-project#6792) * [FIX] mmmu bench serving result display error (sgl-project#6525) (sgl-project#6791) * Bump torch to 2.7.0 (sgl-project#6788) * chore: bump sgl-kernel v0.1.5 (sgl-project#6794) * Improve profiler and integrate profiler in bench_one_batch_server (sgl-project#6787) * chore: upgrade sgl-kernel v0.1.5 (sgl-project#6795) * [Minor] Always append newline after image token when parsing chat message (sgl-project#6797) * Update CI tests for Llama4 models (sgl-project#6421) * [Feat] Enable PDL automatically on Hopper architecture (sgl-project#5981) * chore: update blackwell docker (sgl-project#6800) * misc: cache is_hopper_arch (sgl-project#6799) * Remove contiguous before Flashinfer groupwise fp8 gemm (sgl-project#6804) * Correctly abort the failed grammar requests & Improve the handling of abort (sgl-project#6803) * [EP] Add cuda kernel for moe_ep_pre_reorder (sgl-project#6699) * Add draft extend CUDA graph for flashinfer backend (sgl-project#6805) * Refactor CustomOp to avoid confusing bugs (sgl-project#5382) * Tiny log prefill time (sgl-project#6780) * Tiny fix EPLB assertion about rebalancing period and recorder window size (sgl-project#6813) * Add simple utility to dump tensors for debugging (sgl-project#6815) * Fix profiles do not have consistent names (sgl-project#6811) * Speed up rebalancing when using non-static dispatch algorithms (sgl-project#6812) * [1/2] Add Kernel support for Cutlass based Fused FP4 MoE (sgl-project#6093) * [Router] Fix k8s Service Discovery (sgl-project#6766) * Add CPU optimized kernels for topk and rope fusions (sgl-project#6456) * fix new_page_count_next_decode (sgl-project#6671) * Fix wrong weight reference in dynamic EPLB (sgl-project#6818) * Minor add metrics to expert location updater (sgl-project#6816) * [Refactor] Rename `n_share_experts_fusion` as `num_fused_shared_experts` (sgl-project#6735) * [FEAT] Add transformers backend support (sgl-project#5929) * [fix] recover auto-dispatch for rmsnorm and rope (sgl-project#6745) * fix ep_moe_reorder kernel bugs (sgl-project#6858) * [Refactor] Multimodal data processing for VLM (sgl-project#6659) * Decoder-only Scoring API (sgl-project#6460) * feat: add dp-rank to KV events (sgl-project#6852) * Set `num_fused_shared_experts` as `num_shared_experts` when shared_experts fusion is not disabled (sgl-project#6736) * Fix one missing arg in DeepEP (sgl-project#6878) * Support LoRA in TestOpenAIVisionServer and fix fused kv_proj loading bug. (sgl-project#6861) * support 1 shot allreduce in 1-node and 2-node using mscclpp (sgl-project#6277) * Fix Qwen3MoE missing token padding optimization (sgl-project#6820) * Tiny update error hints (sgl-project#6846) * Support layerwise rebalancing experts (sgl-project#6851) * Tiny allow profiler API to auto create directory (sgl-project#6865) * Support Blackwell DeepEP docker images (sgl-project#6868) * [EP] Add cuda kernel for moe_ep_post_reorder (sgl-project#6837) * [theta]merge 0605 * oai: fix openAI client error with single request via batch api (sgl-project#6170) * [PD] Fix potential perf spike caused by tracker gc and optimize doc (sgl-project#6764) * Use deepgemm instead of triton for fused_qkv_a_proj_with_mqa (sgl-project#6890) * [CUTLASS-FP4-MOE] Introduce CutlassMoEParams class for easy initialization of Cutlass Grouped Gems Metadata (sgl-project#6887) * bugfix(OAI): Fix image_data processing for jinja chat templates (sgl-project#6877) * [CPU] enable CI for PRs, add Dockerfile and auto build task (sgl-project#6458) * AITER backend extension and workload optimizations (sgl-project#6838) * [theta]merge * [theta]merge * [Feature] Support Flashinfer fmha on Blackwell (sgl-project#6930) * Fix a bug in abort & Improve docstrings for abort (sgl-project#6931) * Tiny support customize DeepEP max dispatch tokens per rank (sgl-project#6934) * Sync the changes on cuda graph runners (sgl-project#6932) * [PD] Optimize transfer queue forward logic for dummy rank (sgl-project#6922) * [Refactor] image data process in bench_serving (sgl-project#6879) * [fix] logical_to_all_physical_map index 256 is out of bounds in EP parallel. (sgl-project#6767) * Add triton fused moe kernel config for E=257 on B200 (sgl-project#6939) * [sgl-kernel] update deepgemm (sgl-project#6942) * chore: bump sgl-kernel v0.1.6 (sgl-project#6943) * Minor compile fused topk (sgl-project#6944) * [Bugfix] pipeline parallelism and Eagle Qwen2 (sgl-project#6910) * Tiny re-introduce profile id logging (sgl-project#6912) * Add triton version as a fused_moe_triton config search key to avoid performace decrease in different Triton version (sgl-project#5955) * reduce torch.zeros overhead in moe align block size kernel (sgl-project#6369) * chore: upgrade sgl-kernel v0.1.6 (sgl-project#6945) * add fbgemm moe grouped gemm kernel benchmark (sgl-project#6924) * [Docker] Add docker file for SGL Router (sgl-project#6915) * Disabling mixed chunked prefill when eagle is enabled (sgl-project#6874) * Add canary for EPLB rebalancing (sgl-project#6895) * Refactor global_server_args_dict (sgl-project#6866) * Fuse routed scaling factor in topk_reduce kernel (sgl-project#6220) * Update server timeout time in AMD CI. (sgl-project#6953) * [misc] add is_cpu() (sgl-project#6950) * Add H20 fused MoE kernel tuning configs for DeepSeek-R1/V3 (sgl-project#6885) * Add a CUDA kernel for fusing mapping and weighted sum for MoE. (sgl-project#6916) * chore: bump sgl-kernel v0.1.6.post1 (sgl-project#6955) * chore: upgrade sgl-kernel v0.1.6.post1 (sgl-project#6957) * [DeepseekR1-FP4] Add Support for nvidia/DeepSeekR1-FP4 model (sgl-project#6853) * Revert "Fuse routed scaling factor in topk_reduce kernel (sgl-project#6220)" (sgl-project#6968) * [AMD] Add more tests to per-commit-amd (sgl-project#6926) * chore: bump sgl-kernel v0.1.7 (sgl-project#6963) * Slightly improve the sampler to skip unnecessary steps (sgl-project#6956) * rebase h20 fused_moe config (sgl-project#6966) * Fix CI and triton moe Configs (sgl-project#6974) * Remove unnecessary kernels of num_token_non_padded (sgl-project#6965) * Extend cuda graph capture bs for B200 (sgl-project#6937) * Fuse routed scaling factor in deepseek (sgl-project#6970) * Sync cuda graph runners (sgl-project#6976) * Fix draft extend ut stability with flush cache (sgl-project#6979) * Fix triton sliding window test case (sgl-project#6981) * Fix expert distribution dumping causes OOM (sgl-project#6967) * Minor remove one kernel for DeepSeek (sgl-project#6977) * [perf][sgl-kernel] extend cutlass_mla_decode to support num_head < 128 (sgl-project#6929) * Enable more unit tests for AMD CI. (sgl-project#6983) * Use torch.compile to fuse flash attention decode metadata preparation (sgl-project#6973) * Eliminate stream sync to speed up LoRA batch init (sgl-project#6960) * support qwen3 emebedding (sgl-project#6990) * Fix torch profiler bugs for bench_offline_throughput.py (sgl-project#6557) * chore: upgrade flashinfer v0.2.6.post1 jit (sgl-project#6958) * cleanup tmp dir (sgl-project#7007) * chore: update pr test xeon (sgl-project#7008) * Fix cutlass MLA gets almost zero accuracy (sgl-project#6998) * Update amd nightly models CI. (sgl-project#6992) * feat: add direct routing strategy to DP worker (sgl-project#6884) * Fallback to lower triton version for unfound fused moe configs (sgl-project#7013) * Fix torchvision version for Blackwell (sgl-project#7015) * Simplify prepare_extend_after_decode (sgl-project#6987) * Migrate to assertEqual (sgl-project#6741) * Fix torch version in blackwell dockerfile (sgl-project#7017) * chore: update pr test xeon (sgl-project#7018) * Update default settings for blackwell (sgl-project#7023) * Support both approximate and exact expert distribution collection (sgl-project#6964) * Add decode req pool (sgl-project#6980) * [theta]merge 0610 * [theta]merge 0610 * [CI] Add CI workflow for sgl-router docker build (sgl-project#7027) * Fix fused_moe triton configs (sgl-project#7029) * CPU: map changes from developing branch in sgl-kernel (sgl-project#6833) * chore: bump v0.4.7 (sgl-project#7038) * Update README.md (sgl-project#7040)
Motivation
Modifications
Checklist