[DSV4] DeepSeekV4 Pro#2858
Conversation
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
coderabbitai
Bot
added
deepseek
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new example script, deepseek_v4_pro_example.py, for quantizing the DeepSeek-V4 model using llmcompressor with FP8 and NVFP4 schemes. Feedback on the changes highlights several critical improvements to make the script reusable and functional: avoiding hardcoded absolute local paths for the model and offload directories, loading the tokenizer dynamically from the model ID, removing commented-out debug code, and uncommenting the recipe parameter in the oneshot function so that the quantization scheme is actually applied.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
examples/quantizing_moe/deepseek_v4_pro_example.py (3)
81-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a comment to explain
add_special_tokens=False.For educational clarity, it would help users to understand why
add_special_tokens=Falseis used here (since special tokens like BOS/EOS were already added in thepreprocessfunction).📝 Suggested improvement
def tokenize(sample): return tokenizer( sample["text"], padding=False, max_length=MAX_SEQUENCE_LENGTH, truncation=True, - add_special_tokens=False, + add_special_tokens=False, # Special tokens already added in preprocess() )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/quantizing_moe/deepseek_v4_pro_example.py` around lines 81 - 88, In the tokenize function, add a comment above or next to the add_special_tokens=False parameter to explain why special tokens are not being added here. The comment should clarify that special tokens like BOS/EOS were already added during preprocessing in the preprocess function, so they should not be added again during tokenization to avoid duplication.Source: Path instructions
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the empty
max_memoryparameter or add explanation.The
max_memory={}parameter is empty and appears to have no effect. For educational clarity, either:
- Remove it if not needed
- Provide an example value with a comment explaining how users can limit per-device memory allocation
Example with explanation:
max_memory={0: "40GB", 1: "40GB", "cpu": "100GB"}, # Optional: limit per-device memory🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/quantizing_moe/deepseek_v4_pro_example.py` at line 30, The `max_memory={}` parameter being passed in the function call is empty and has no practical effect. Either remove this parameter entirely since an empty dictionary serves no purpose, or replace it with a meaningful example that demonstrates how users can limit per-device memory allocation (such as specifying device IDs and memory limits like {0: "40GB", 1: "40GB", "cpu": "100GB"}) and add a comment explaining the parameter's purpose for educational clarity.Source: Path instructions
104-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider minor clarifications to the recipe configuration.
The quantization recipe is well-structured, but could benefit from small improvements:
Line 109: The target
r"re:.*attn\.compressor\.indexer\.q_b_proj$"is not mentioned in the comments above (lines 96-102). Adding it to the comments would help users understand the full scope.Line 120: The explicit
ignore=[]parameter is unnecessary when empty and can be removed for cleaner code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/quantizing_moe/deepseek_v4_pro_example.py` around lines 104 - 121, Add a clarifying comment above the QuantizationModifier to document the compressor indexer target pattern `r"re:.*attn\.compressor\.indexer\.q_b_proj$"` that appears in the attention config_groups, explaining its purpose alongside the existing attention projection targets. Additionally, remove the empty `ignore=[]` parameter from the QuantizationModifier initialization as it is unnecessary when not explicitly needed.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/quantizing_moe/deepseek_v4_pro_example.py`:
- Around line 16-19: The comment for the
DeepseekV4PreTrainedModel._keep_in_fp32_modules_strict workaround lacks
sufficient context for educational purposes. Enhance the existing comment to
include a comprehensive explanation of the upstream bug being worked around,
link to relevant GitHub issues or pull requests that document this bug, and add
a TODO comment specifying the conditions or library version when this workaround
can be removed. This will help users understand why the workaround is necessary
and when it becomes obsolete.
- Around line 34-38: Remove the commented-out "kluge" workaround code block and
its explanatory comment from the example file. Delete the entire section
starting with the comment "# kluge for the way I saved the decompressed
checkpoint" and all subsequent commented-out lines that follow it which
manipulate the mds weights_map dataset index. This developer-specific workaround
code is not relevant to users learning from the example and should be removed to
keep the example clean and production-ready.
- Around line 126-139: The recipe parameter in the oneshot() function call is
commented out, preventing the quantization recipe from being applied during the
calibration process. Uncomment the recipe=recipe, parameter in the oneshot()
function call to ensure the quantization recipe defined earlier is actually
used. Additionally, improve the comments above and around the
propagate_error=False parameter to clearly explain what the workaround addresses
and when users might encounter similar issues with transformers cache behavior.
- Around line 21-32: Replace the hardcoded local file system paths with publicly
accessible alternatives in the model loading section. Change the MODEL_ID
variable from the local path to use the publicly available Hugging Face model
identifier that is already commented out, and replace the hardcoded
offload_folder path in the AutoModelForCausalLM.from_pretrained call with either
a generic temporary directory path or make it configurable through a variable
that users can easily modify. This will make the example script runnable for end
users without requiring access to specific developer machine paths.
- Around line 59-75: In the preprocess function, add a comment above or within
the assistant role handling block (the elif role == "assistant": section) to
explain that the </think> token is part of DeepSeek-V4's chain-of-thought
reasoning format for educational clarity. Additionally, add error handling to
the function to validate that each message in example["messages"] contains both
"role" and "content" fields before accessing them, either by wrapping the
message processing loop in a try-except block or by adding explicit validation
checks to handle missing or malformed message fields gracefully.
- Line 40: The AutoTokenizer.from_pretrained call hardcodes a model path
"RedHatAI/DeepSeek-V4-Flash-BF16" that differs from the model being used in the
example. Either replace the hardcoded string with the MODEL_ID variable to
ensure consistency throughout the example, or if the Flash and Pro model
tokenizers are intentionally the same, add a clear comment explaining why
different model identifiers are used for the model and tokenizer respectively.
---
Nitpick comments:
In `@examples/quantizing_moe/deepseek_v4_pro_example.py`:
- Around line 81-88: In the tokenize function, add a comment above or next to
the add_special_tokens=False parameter to explain why special tokens are not
being added here. The comment should clarify that special tokens like BOS/EOS
were already added during preprocessing in the preprocess function, so they
should not be added again during tokenization to avoid duplication.
- Line 30: The `max_memory={}` parameter being passed in the function call is
empty and has no practical effect. Either remove this parameter entirely since
an empty dictionary serves no purpose, or replace it with a meaningful example
that demonstrates how users can limit per-device memory allocation (such as
specifying device IDs and memory limits like {0: "40GB", 1: "40GB", "cpu":
"100GB"}) and add a comment explaining the parameter's purpose for educational
clarity.
- Around line 104-121: Add a clarifying comment above the QuantizationModifier
to document the compressor indexer target pattern
`r"re:.*attn\.compressor\.indexer\.q_b_proj$"` that appears in the attention
config_groups, explaining its purpose alongside the existing attention
projection targets. Additionally, remove the empty `ignore=[]` parameter from
the QuantizationModifier initialization as it is unnecessary when not explicitly
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f787e13-b61c-4a72-bac3-e71a8952c37c
📒 Files selected for processing (1)
examples/quantizing_moe/deepseek_v4_pro_example.py
|
The quality checks have failed. Please run |
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
kylesayrs
force-pushed
the
kylesayrs/dsv4-pro
branch
from
0e3c77c to
a54b31a
Compare
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The quality checks have failed. Please run |
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
|
The quality checks have failed. Please run |
Signed-off-by: Kyle Sayers <kylesayrs@gmail.com>
|
The quality checks have failed. Please run |
|
The quality checks have failed. Please run |
|
This pull request has merge conflicts that must be resolved before it can be |
1 participant
Prerequisites
Accelerate/ Transformers
use_cache=Falsefor DeepSeek V4 huggingface/transformers#46965Compressed Tensors
apply_quantization_configcompressed-tensors#730load_offloaded_modelcompressed-tensors#728to_accelerateby only instantiatingOffloadedWeightsLoaderonce compressed-tensors#752LLM Compressor
vLLM