Repository navigation
[5924759] Fix fp16 ONNX INT8 entropy calibration on numpy >= 2.0 - #1558
Conversation
|
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)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughCast histogram range endpoints to Python float in two calibration histogram paths to prevent fp16-derived bin-edge collapse with NumPy >= 2.0, add a regression test for FP16 narrow-range inputs, and document the fix in the 0.45 changelog. ChangesNumPy 2.0 histogram compatibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/gpu/onnx/test_ort_patching.py (1)
156-170: ⚡ Quick winExtend regression coverage to the single-node histogram path too.
This test validates
_collect_value, but the same float-cast fix was also applied to_collect_value_histogram_collector_single_node_calibration. Consider parameterizing this test (or adding a sibling test) to assert identical fp16 narrow-range behavior there as well.As per coding guidelines, “Use pytest for new/updated tests; keep tests lean and add coverage that guards the expected behavior/regression.”
🤖 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/gpu/onnx/test_ort_patching.py` around lines 156 - 170, Add the same fp16 narrow-range assertion for the single-node histogram path by running the existing assertions against the alternative collector entry point _collect_value_histogram_collector_single_node_calibration (either by parameterizing test_collect_value_fp16_narrow_range with both _collect_value and _collect_value_histogram_collector_single_node_calibration or by adding a sibling test). Ensure you invoke the same setup (activations, name_to_arr, mock_histogram_collector), call the single-node function, then assert hist.sum() equals activations.size, len(edges) equals mock_histogram_collector.num_bins + 1, and no zero-width bins (not np.any(np.diff(edges) == 0)) to mirror the original test’s checks.
🤖 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/gpu/onnx/test_ort_patching.py`:
- Around line 156-170: Add the same fp16 narrow-range assertion for the
single-node histogram path by running the existing assertions against the
alternative collector entry point
_collect_value_histogram_collector_single_node_calibration (either by
parameterizing test_collect_value_fp16_narrow_range with both _collect_value and
_collect_value_histogram_collector_single_node_calibration or by adding a
sibling test). Ensure you invoke the same setup (activations, name_to_arr,
mock_histogram_collector), call the single-node function, then assert hist.sum()
equals activations.size, len(edges) equals mock_histogram_collector.num_bins +
1, and no zero-width bins (not np.any(np.diff(edges) == 0)) to mirror the
original test’s checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69514b50-b834-40bf-8abf-257dc247b070
📒 Files selected for processing (3)
CHANGELOG.rstmodelopt/onnx/quantization/ort_patching.pytests/gpu/onnx/test_ort_patching.py
|
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Small, well-scoped bug fix for fp16 ONNX INT8 entropy calibration on numpy >= 2.0. The root-cause analysis (NEP-50 strict promotion causing fp16 linspace bin-edge collapse) is correct, the fix (casting threshold to Python float so np.histogram computes edges in float64) is minimal and applied at both relevant call sites in ort_patching.py. The new test_collect_value_fp16_narrow_range reproduces the failure mode and asserts no bin-edge collapse. CHANGELOG is updated. No API change, no licensing impact.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1558 +/- ##
===========================================
+ Coverage 56.40% 76.57% +20.17%
===========================================
Files 506 507 +1
Lines 55486 55556 +70
===========================================
+ Hits 31295 42541 +11246
+ Misses 24191 13015 -11176
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:
|
| range_max = float(threshold) | ||
| hist, hist_edges = np.histogram( | ||
| data_arr, histogram_collector.num_bins, range=(-threshold, threshold) | ||
| data_arr, histogram_collector.num_bins, range=(-range_max, range_max) |
There was a problem hiding this comment.
This code is a duplicate from _collect_value(). Can we merge this into a separate util function?
gcunhase
left a comment
There was a problem hiding this comment.
LGTM after minor comment is addressed.
| activations[0] = np.float16(1e-5) | ||
| name_to_arr = {"narrow_fp16_tensor": [activations]} | ||
|
|
||
| _collect_value(mock_histogram_collector, name_to_arr) |
There was a problem hiding this comment.
Does it requiring test for _collect_value_histogram_collector_single_node_calibration as well?
_collect_value derives threshold from activation min/max. For fp16 activations with a small range, both np.histogram range endpoints inherit fp16 dtype. Under numpy 2.0 NEP-50 promotion the resulting fp16 linspace collapses 128 bin edges and numpy raises "Too many bins for data range". Cast range to Python float so bin edges are computed in float64. Same fix applied to the single-node calibration variant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
c4ceddc to
8651d82
Compare
#1558 #1670 #1662 #1677 #1327 #1673 #1676 #1687 #1678 #1691 #1697 #1702 #1704 #1726 #1729 (#1734) ## Cherry-picked PRs - #1648 - #1650 - #1594 - #1269 - #1326 - #1652 - #1651 - #1601 - #1653 - #1558 - #1670 - #1662 - #1677 - #1327 - #1673 - #1676 - #1687 - #1678 - #1691 - #1697 - #1702 - #1704 - #1726 - #1729 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added Alpamayo quantization example with FP8/NVFP4 export support. * Introduced FastGen DMD2 distillation library for Qwen-Image text-to-image optimization. * Added lossless MXFP4-to-NVFP4 weight casting for DeepSeek models. * Expanded PTQ recipes with new NVFP4 variants (MLP-only, experts-only, weight-only). * Enhanced sparse attention calibration and export capabilities. * **Documentation** * Added end-to-end Nemotron-3 optimization tutorial and comprehensive PTQ recipe guide. * Updated example READMEs and CHANGELOG with latest optimization capabilities. * **Bug Fixes** * Fixed sparse attention configuration export schema. * Improved KV cache reuse settings for context logits generation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com> Signed-off-by: Rohan Joshi <rohjoshi@nvidia.com> Signed-off-by: Jingyu Xin <jingyux@nvidia.com> Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com> Signed-off-by: Chenhan Yu <chenhany@nvidia.com> Signed-off-by: Fridah-nv <201670829+Fridah-nv@users.noreply.github.com> Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com> Signed-off-by: weimingc <17592131+meenchen@users.noreply.github.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com> Signed-off-by: Gwenaelle Cunha Sergio <gcunhasergio@nvidia.com> Signed-off-by: Sabari07 <sabursd18@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Rohan Joshi <rohjoshi@nvidia.com> Co-authored-by: jingyu-ml <108295447+jingyu-ml@users.noreply.github.com> Co-authored-by: Chenjie Luo <108829653+cjluo-nv@users.noreply.github.com> Co-authored-by: Ajinkya Rasane <131806219+ajrasane@users.noreply.github.com> Co-authored-by: Chenhan D. Yu <5185878+ChenhanYu@users.noreply.github.com> Co-authored-by: Frida Hou <201670829+Fridah-nv@users.noreply.github.com> Co-authored-by: kinjalpatel27 <31936134+kinjalpatel27@users.noreply.github.com> Co-authored-by: Wei-Ming Chen <17592131+meenchen@users.noreply.github.com> Co-authored-by: Zhiyu <zhiyuc@nvidia.com> Co-authored-by: Gwena Cunha <4861122+gcunhase@users.noreply.github.com> Co-authored-by: Sabari07 <sabursd18@gmail.com> Co-authored-by: Sepehr Sameni <ssameni@nvidia.com>
### What does this PR do? Type of change: Bug fix FP16 entropy calibration can fail for sufficiently narrow activation ranges because NumPy may construct the histogram bin edges at FP16 precision. NumPy 2.2 and later reject the resulting collapsed bin spacing, while earlier versions can silently return invalid, non-monotonic edges. The same precision issue can recur when ONNX Runtime merges later calibration batches. Losslessly widen FP16 activation values to FP32 while calculating and merging histograms in both ONNX entropy calibration paths, then restore the source dtype at the calibration-to-quantization boundary. This keeps the histogram bins stable without changing FP16 Q/DQ scale or graph dtype semantics. FP32 inputs and public APIs are unchanged. For full-range FP16 activations, ONNX Runtime can overflow while subtracting FP16 calibration endpoints before it widens the result. Retry only a non-finite FP16 scale calculation with FP32 endpoints, then cast the finite scale back to FP16. Existing finite FP16 calculations and all non-FP16 calculations continue to use ONNX Runtime's original result. The fallback intentionally patches only the `qdq_quantizer` binding used for calibrated activation ranges. Initializer and weight quantization continue to use ONNX Runtime's existing `quant_utils` path unchanged; full-range FP16 weight scaling is outside this calibration fix. The regression tests exercise both collectors across initial collection, an equal-range merge, and an expanding-range merge. They also verify the internal FP32 histogram and external FP16 calibration-range contract, including finite saturation when restoring sanitized values. A real entropy calibration test covers full-range FP16 values and verifies finite FP16 Q/DQ scales and a loadable ONNX Runtime graph. The AutoCast integration verifies the same FP16 scale-type contract through the public quantization path. ### Usage N/A — no API or usage change. ### Testing All tests ran with CUDA hidden. - NumPy 1.26.4: affected histogram, AutoCast, and ORT-patching modules: `39 passed`. - NumPy 2.2.3: affected histogram, AutoCast, and ORT-patching modules: `39 passed`. - NumPy 2.3.5: affected histogram, AutoCast, and ORT-patching modules: `39 passed`. - Public INT8 entropy quantization with full-range FP16 calibration data: finite FP16 Q/DQ scales, full ONNX check passed, and the CPU ONNX Runtime session loaded. - Changed-file pre-commit hooks: passed. ### Before your PR is "*Ready for review*" Make sure you read and follow [Contributor guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md) and your commits are signed (`git commit -s -S`). Make sure you read and follow the [Security Best Practices](https://github.com/NVIDIA/Model-Optimizer/blob/main/SECURITY.md#security-coding-practices-for-contributors) (e.g. avoiding hardcoded `trust_remote_code=True`, `torch.load(..., weights_only=False)`, `pickle`, etc.). - 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?: ❌ ### Additional Information Follow-up to #1558. > 🤖 _Generated by Codex (AI agent)._ --------- Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com> Co-authored-by: Codex <codex@openai.com>
What does this PR do?
Type of change: Bug fix
INT8 entropy calibration of fp16 ONNX models (e.g. ConvNext / EfficientViT / YOLOv8 backbones quantized via
python -m modelopt.onnx.quantization --quantize_mode=int8) used to fail during histogram collection with:_collect_valueinmodelopt/onnx/quantization/ort_patching.pyderivesthreshold = max(abs(min), abs(max))from the activation tensor and passesrange=(-threshold, threshold)tonp.histogram(...). When the model is fp16 and a calibrated activation has a small range (≲ 1e-5), both endpoints inherit fp16 dtype. Under numpy 2.0's NEP-50 strict promotion, the resulting fp16linspacecollapses consecutive 128-bin edges to the same value and numpy refuses to build the histogram. numpy 1.x silently used higher-precision intermediate dtype, masking the issue.The fix casts the range endpoints to Python
floatso numpy computes bin edges in float64 regardless of input dtype. Applied at both call sites:_collect_valueand the single-node variant_collect_value_histogram_collector_single_node_calibration.Usage
# The affected workflow — INT8 entropy calibration of any fp16 ONNX model: python -m modelopt.onnx.quantization \ --quantize_mode=int8 \ --onnx_path=model.fp16.onnx \ --calibration_data_path=calib.npyNo API change.
Testing
test_collect_value_fp16_narrow_rangeintests/gpu/onnx/test_ort_patching.pythat calls_collect_valuewith a fp16 tensor (mostly zeros + one ~1e-5 value) and asserts the histogram is built without raising and all bin edges are distinct. Fails on the buggy code, passes after the fix.tests/gpu/onnx/test_ort_patching.pysuite (31 tests) passes.Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/ASummary by CodeRabbit
Bug Fixes
Tests