fix: resolve DeepSeek-V3 accuracy drop when EP is enabled #8412
Conversation
- Add SGL_DISABLE_EP_FP8 environment variable to disable FP8 quantization in EP mode - Fix accuracy drop from 0.945 to 0.845 on GSM8K benchmark when EP is enabled - Modify EPMoE and DeepEPMoE forward methods to respect FP8 disable flag - Update DeepGEMM requirement check to be optional when FP8 is disabled - Maintain backward compatibility with opt-in fix via environment variable Fixes sgl-project#8402
There was a problem hiding this comment.
Summary of Changes
Hello @juyterman1000, 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!
This pull request delivers a crucial fix for a significant accuracy degradation in DeepSeek-V3 models when operating with Expert Parallelism (EP) enabled. The root cause was identified as the automatic application of FP8 quantization, which introduced numerical precision issues. The primary solution involves providing users with an opt-in mechanism, an environment variable, to disable FP8 quantization specifically for EP mode. This ensures that the performance benefits of EP can be realized without compromising the model's accuracy on reasoning tasks, while also maintaining backward compatibility.
Highlights
- Accuracy Fix for DeepSeek-V3 EP: Addresses a significant accuracy drop (0.945 to 0.845 on GSM8K benchmark) observed when Expert Parallelism (EP) is enabled for DeepSeek-V3 models, which was traced to automatic FP8 quantization.
- FP8 Quantization Control: Introduces a new environment variable,
SGL_DISABLE_EP_FP8, allowing users to explicitly disable FP8 quantization in EP mode, thereby restoring model accuracy while maintaining the performance benefits of Expert Parallelism. - Conditional DeepGEMM Usage: Modifies the
EPMoEandDeepEPMoElayers to make the use of DeepGEMM (an optimized matrix multiplication kernel) conditional on whether FP8 quantization is enabled, ensuring compatibility and flexibility when FP8 is disabled. - Enhanced Testing and Debugging Utilities: Adds new Python scripts (
debug_deepseek_v3_ep_accuracy.pyandtest_deepseek_v3_ep_fix.py) to aid in debugging various EP configurations and to validate the effectiveness of the accuracy fix.
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 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 or fill out our survey 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.
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.
Code Review
This pull request addresses a significant accuracy drop for DeepSeek-V3 models when Expert Parallelism (EP) is enabled. The proposed solution, which introduces an environment variable SGL_DISABLE_EP_FP8 to disable FP8 quantization, is well-reasoned and the core code changes in python/sglang/srt/layers/moe/ep_moe/layer.py are correctly implemented.
The PR also includes several utility scripts for debugging, testing, and patching. While the main fix is solid, I've identified a few issues in these scripts that should be addressed:
- The debug script (
debug_deepseek_v3_ep_accuracy.py) contains a bug related to path handling that will prevent it from running correctly. - The patching script (
fix_deepseek_v3_ep_accuracy.py) uses a brittle string-replacement method and appears to include some dead code.
My review provides specific feedback to resolve these issues. Overall, this is a valuable fix that should restore model accuracy.
|
|
||
| # Create output directory | ||
| os.makedirs(args.output_dir, exist_ok=True) | ||
| os.chdir(args.output_dir) |
There was a problem hiding this comment.
Using os.chdir here will likely break the script. The call to run_gsm8k_benchmark uses a relative path benchmark/gsm8k/bench_sglang.py, which will not be found after changing the current directory. This will cause a FileNotFoundError.
A better practice is to avoid changing the directory and instead construct full paths for your output files. For example:
# In main()
from pathlib import Path
output_dir = Path(args.output_dir)
output_dir.mkdir(exist_ok=True)
# In test_configuration()
output_file = output_dir / f"gsm8k_results_{config_name.replace(' ', '_').lower()}.jsonl"
# In main()
results_file = output_dir / "debug_results.json"This would require passing output_dir to test_configuration.
| print("All fixes applied successfully!") | ||
|
|
||
|
|
||
| def fix_fp8_quantization_issue(): |
There was a problem hiding this comment.
This script modifies source files using string replacement, which is very brittle. Any minor changes to the source code, like adding a space or a comment, will cause this patching script to fail. For a script intended to be used by others, this is a significant risk.
Consider using a more robust patching mechanism, like creating and applying a diff file, or using Abstract Syntax Tree (AST) transformations for a more reliable patch.
| def fix_expert_routing_consistency(): | ||
| """Fix expert routing consistency issues.""" | ||
|
|
||
| print("Fix 2: Improving expert routing consistency...") | ||
|
|
||
| # Path to the topk selection file | ||
| topk_file = Path("python/sglang/srt/layers/moe/topk.py") | ||
|
|
||
| if not topk_file.exists(): | ||
| print(f"Warning: {topk_file} not found") | ||
| return | ||
|
|
||
| # Read the file | ||
| with open(topk_file, 'r') as f: | ||
| content = f.read() | ||
|
|
||
| # Add deterministic routing option | ||
| routing_fix = ''' | ||
| # Add deterministic routing for better consistency in EP mode | ||
| def _ensure_deterministic_routing(topk_weights, topk_ids): | ||
| """Ensure deterministic routing by using stable sorting.""" | ||
| if torch.backends.cudnn.deterministic: | ||
| # Use stable sort to ensure deterministic behavior | ||
| sorted_weights, sorted_indices = torch.sort(topk_weights, dim=-1, descending=True, stable=True) | ||
| topk_ids = torch.gather(topk_ids, -1, sorted_indices) | ||
| topk_weights = sorted_weights | ||
| return topk_weights, topk_ids | ||
| ''' | ||
|
|
||
| # Insert the fix function | ||
| if "_ensure_deterministic_routing" not in content: | ||
| # Find a good place to insert (after imports, before first function) | ||
| insert_pos = content.find("def select_experts(") | ||
| if insert_pos != -1: | ||
| content = content[:insert_pos] + routing_fix + "\n\n" + content[insert_pos:] | ||
|
|
||
| # Write the modified content back | ||
| with open(topk_file, 'w') as f: | ||
| f.write(content) | ||
|
|
||
| print(" ✓ Added deterministic routing function") |
There was a problem hiding this comment.
The function _ensure_deterministic_routing is added to topk.py by fix_expert_routing_consistency, but it appears to be dead code as it's never called within the file or any other part of the changes.
If this function is necessary for the fix, a call to it is missing. If it's not needed, it should be removed to avoid confusion and keep the fix script focused on the required changes.
Fixes #8402
Motivation
Modifications
Checklist