Fix per-tensor FP8 weight dequantization#1962
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1962 +/- ##
==========================================
+ Coverage 76.50% 77.32% +0.81%
==========================================
Files 525 524 -1
Lines 59257 59251 -6
==========================================
+ Hits 45337 45814 +477
+ Misses 13920 13437 -483
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7069c2b to
db3edfb
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds scalar-scale FP8 weight dequantization support, centralizes FP8 conversion paths, adds targeted unit coverage, and introduces a documented, loader-tested Mistral-Medium NVFP4 max-calibration recipe. ChangesFP8 dequantization
Mistral-Medium NVFP4 recipe
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 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 `@modelopt/torch/quantization/plugins/huggingface.py`:
- Around line 1353-1363: CPU scalar-scale dequantization fails because CUDA
device context wrapping occurs unconditionally in forward paths. Move the
torch.cuda.device(self.weight.device) context from forward and unpack_weight
into _dequantize_weight, applying it only around the Triton weight_dequant call;
leave the block_size is None multiplication path context-free so forward and
unpack_weight work with CPU tensors.
🪄 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: Enterprise
Run ID: d0335345-d7f3-4221-b07b-bc382357a35c
📒 Files selected for processing (2)
modelopt/torch/quantization/plugins/huggingface.pytests/unit/torch/quantization/plugins/test_huggingface.py
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-4-8) — DM the bot to share feedback.
Clean, focused bug fix (+34/-12, 2 files) adding per-tensor (scalar weight_scale_inv) FP8 weight dequantization to _QuantFP8Linear.
Correctness verified:
_setupsetsblock_size = Nonefor 0D scale; 2D path preserves the existing block=128 assert/computation. Other ndims still fail thendim == 2assert as before.- New
_dequantize_weighthelper does directweight.to(dtype) * scale_inv.to(dtype)for the scalar path (correct per-tensor broadcast) and delegates to the unchanged Tritonweight_dequantkernel for the block path. Theassert weight_dequant is not Nonecorrectly moved into the block-only branch, so the scalar path no longer requires Triton. forwardandunpack_weightnow share the helper with behavior preserved for the block path (confirmed againstfp8_kernel.py, which is untouched).
Test: test_fp8_linear_per_tensor_dequant is CPU-runnable — it monkeypatches weight_dequant=None, asserts block_size is None, and checks the dequant result equals weight.float() * 2.0. Reasonable focused coverage; PR body documents manual full-model validation (Mistral-Medium-3.5, 616 modules, finite logits, unpack_weight() on q_proj).
No licensing changes (headers/LICENSE untouched), no local imports, no new subsystem/abstraction, small cohesive change. No prompt-injection attempts in the PR body/diff.
Minor (non-blocking, pre-existing): the scalar path in forward/unpack_weight still runs under torch.cuda.device(self.weight.device), and the test exercises _dequantize_weight directly rather than end-to-end forward()/unpack_weight() — both consistent with prior FP8 behavior and covered by the described manual GPU validation.
db3edfb to
9d6fdde
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/recipe/test_loader.py (1)
169-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd semantic coverage for the new recipe.
The current parameterized test only proves that the path parses and contains a
quantizesection. Add a focused assertion for this recipe’s expected max-calibration settings and representative NVFP4, FP8, and KV-cache overrides so incorrect wildcard ordering or missing imports cannot pass unnoticed.As per path instructions, tests should protect the expected behavior with focused pytest coverage.
🤖 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 `@tests/unit/recipe/test_loader.py` at line 169, Extend the parameterized recipe test covering "huggingface/models/nvidia/Mistral-Medium-3.5-128B-NVFP4/ptq/nvfp4-max-calib" with focused assertions for its max-calibration configuration, including representative NVFP4, FP8, and KV-cache overrides. Validate the resolved settings rather than only path parsing and the presence of quantize, ensuring wildcard ordering and required imports are exercised.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.
Nitpick comments:
In `@tests/unit/recipe/test_loader.py`:
- Line 169: Extend the parameterized recipe test covering
"huggingface/models/nvidia/Mistral-Medium-3.5-128B-NVFP4/ptq/nvfp4-max-calib"
with focused assertions for its max-calibration configuration, including
representative NVFP4, FP8, and KV-cache overrides. Validate the resolved
settings rather than only path parsing and the presence of quantize, ensuring
wildcard ordering and required imports are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 103c04e7-2eae-4ba4-9e29-36d9066f7217
📒 Files selected for processing (4)
modelopt/torch/quantization/plugins/huggingface.pymodelopt_recipes/huggingface/models/nvidia/Mistral-Medium-3.5-128B-NVFP4/ptq/nvfp4-max-calib.yamltests/unit/recipe/test_loader.pytests/unit/torch/quantization/plugins/test_huggingface.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/torch/quantization/plugins/test_huggingface.py
- modelopt/torch/quantization/plugins/huggingface.py
Add the published Mistral Medium 3.5 NVFP4 recipe. Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
9d6fdde to
3b1d157
Compare
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
3 participants
What does this PR do?
Type of change: Bug fix
Supports scalar
weight_scale_invvalues when converting Hugging Face FP8 linear layers. The scalar path dequantizes weights directly, while the existing block-scale Triton path remains unchanged. Forward execution andunpack_weight()share the same dequantization helper.Usage
No API changes. Hugging Face FP8 checkpoints using per-tensor weight scales convert without additional configuration.
Testing
test_fp8_linear_per_tensor_dequant— passed.mistralai/Mistral-Medium-3.5-128Bon AWS-PDX and converted all 616 scalar-scale FP8 linear modules.unpack_weight()passed onmodel.language_model.layers.0.self_attn.q_proj.Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
This isolates the per-tensor FP8 dequantization fix from commit
2f913319d2ac2fbd47397ece871f18576f2ad009; unrelated recipe changes are excluded.Summary by CodeRabbit
Summary
block_sizeand using a dedicated dequantization path for more reliable results.