Repository navigation
Fix hf_ptq.py discarding completed PTQ run on sanity-generate() failure - #2480
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/Model-Optimizer/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between efbd51878aa50717aaac58b57c44136b3fd1f40f and fb389ab. 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe post-quantization generation sanity check now catches failures, emits a warning, and allows checkpoint export to continue. A regression test verifies this behavior, and the changelog records the fix. ChangesPTQ export recovery
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The best-effort generation check warns on failure and still exports the checkpoint, with regression coverage for that behavior. The change is ready to merge. 🚥 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}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2480 +/- ##
==========================================
+ Coverage 68.65% 70.71% +2.06%
==========================================
Files 601 601
Lines 66394 66394
==========================================
+ Hits 45581 46952 +1371
+ Misses 20813 19442 -1371
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:
|
cjluo-nv
approved these changes
Sep 19, 2026
cjluo-nv
left a comment
cjluo-nv
left a comment
Collaborator
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Approve — the guard around the optional post-PTQ sanity generate() is correct (generated_ids_after_ptq stays None, the decode block is already None-gated, export still runs), and the new regression test drives post_quantize() end-to-end and asserts export_quantized() fires.
No action needed.
The post-quantization sanity-check generate() call in post_quantize() ran unguarded before export_quantized(), so any exception there (e.g. device_map="auto" offloading part of the model to CPU, which NVFP4's dynamic block quantization does not support on CPU) aborted the whole run and discarded a completed calibration without exporting a checkpoint. Wrap the call so a failure there only skips the sanity check and export still proceeds. Fixes NVBug 6752977. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
Edwardf0t1
force-pushed
the
fix/nvbug-6752977-hf-ptq-sanity-generate
branch
from
September 19, 2026 00:22
efbd518 to
fb389ab
Compare
Edwardf0t1
enabled auto-merge (squash)
September 19, 2026 00:35
Contributor
|
chadvoegele
added
the
cherry-pick-done
chadvoegele
pushed a commit
that referenced
this pull request
Sep 22, 2026
…re (#2480) Type of change: Bug fix `post_quantize()` in `examples/hf_ptq/hf_ptq.py` ran the optional post-quantization sanity-check `full_model.generate()` unguarded, directly before `export_quantized()`. Any exception raised there aborted the whole run and discarded a completed calibration without exporting a checkpoint. Root cause (traced from [NVBug 6752977](https://nvbugspro.nvidia.com/bug/6752977), DGX Spark GB10 / DeepSeek-R1-Distill-Llama-8B / NVFP4): `get_model()` loads with `device_map="auto"`, relying on `accelerate`'s `infer_auto_device_map`/`get_max_memory()` to decide GPU vs. CPU placement. On DGX Spark's unified-memory single-GPU host, that memory probe under-reports GPU capacity, so part of even an 8B model can land on CPU — and the existing fallback shrinks the GPU budget further (`* gpu_mem_percentage`), compounding it. Calibration survives this because it never invokes the real fake-quant kernel, but the post-PTQ sanity `generate()` does, and NVFP4's dynamic-block-quantize op (`modelopt/torch/quantization/tensor_quant.py`) hard-asserts `amax.is_cuda` with no CPU fallback, so any CPU-offloaded layer crashes there — after ~5.8 hours of calibration, before export. This PR does not attempt to fix the underlying `device_map`/memory-probing behavior (unverified without the actual hardware/logs, which weren't reachable from this environment). Instead it makes the failure mode safe: a failure in the optional sanity check now only skips that check and warns, and export always proceeds, regardless of why `generate()` failed. No new API. Behavior change only: `examples/hf_ptq/hf_ptq.py` now completes export even if the post-quantization sanity `generate()` call raises. - Added `tests/examples/hf_ptq/test_hf_ptq_args.py::test_post_quantize_export_survives_a_failed_sanity_generate`, which drives `post_quantize()` with a `full_model.generate()` that raises and asserts `export_quantized()` still runs. - Ran `pytest tests/examples/hf_ptq/test_hf_ptq_args.py` (48 passed). - Ran `pre-commit` on the changed files (`ruff-format` reformatted line wrapping only). - Is this change backward compatible?: ✅ - If you copied code from any other sources or added a new PIP dependency, did you follow guidance in `CONTRIBUTING.md`: N/A - Did you write any new necessary tests?: ✅ - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: ✅ - Did you get Claude approval on this PR?: ❌ <!-- pending: run `/claude review` --> Fixes NVBug 6752977. Linked JIRA: OMNIML-5932. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **Bug Fixes** - Quantized checkpoint export now continues when the optional post-quantization generation check fails. - A warning is shown when the generation check cannot complete. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
chadvoegele
removed
cherry-pick-done
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
3 participants
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.
What does this PR do?
Type of change: Bug fix
post_quantize()inexamples/hf_ptq/hf_ptq.pyran the optional post-quantization sanity-checkfull_model.generate()unguarded, directly beforeexport_quantized(). Any exception raised there aborted the whole run and discarded a completed calibration without exporting a checkpoint.Root cause (traced from NVBug 6752977, DGX Spark GB10 / DeepSeek-R1-Distill-Llama-8B / NVFP4):
get_model()loads withdevice_map="auto", relying onaccelerate'sinfer_auto_device_map/get_max_memory()to decide GPU vs. CPU placement. On DGX Spark's unified-memory single-GPU host, that memory probe under-reports GPU capacity, so part of even an 8B model can land on CPU — and the existing fallback shrinks the GPU budget further (* gpu_mem_percentage), compounding it. Calibration survives this because it never invokes the real fake-quant kernel, but the post-PTQ sanitygenerate()does, and NVFP4's dynamic-block-quantize op (modelopt/torch/quantization/tensor_quant.py) hard-assertsamax.is_cudawith no CPU fallback, so any CPU-offloaded layer crashes there — after ~5.8 hours of calibration, before export.This PR does not attempt to fix the underlying
device_map/memory-probing behavior (unverified without the actual hardware/logs, which weren't reachable from this environment). Instead it makes the failure mode safe: a failure in the optional sanity check now only skips that check and warns, and export always proceeds, regardless of whygenerate()failed.Usage
No new API. Behavior change only:
examples/hf_ptq/hf_ptq.pynow completes export even if the post-quantization sanitygenerate()call raises.Testing
tests/examples/hf_ptq/test_hf_ptq_args.py::test_post_quantize_export_survives_a_failed_sanity_generate, which drivespost_quantize()with afull_model.generate()that raises and assertsexport_quantized()still runs.pytest tests/examples/hf_ptq/test_hf_ptq_args.py(48 passed).pre-commiton the changed files (ruff-formatreformatted line wrapping only).Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Fixes NVBug 6752977. Linked JIRA: OMNIML-5932.
🤖 Generated with Claude Code
Summary by CodeRabbit