-
Notifications
You must be signed in to change notification settings - Fork 240
[5763424][ONNX][Autocast] Fix ConstantOfShape layer output precision #789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5763424][ONNX][Autocast] Fix ConstantOfShape layer output precision #789
Conversation
Signed-off-by: gcunhase <[email protected]>
📝 WalkthroughWalkthroughA single-line conditional branch is added to handle the ConstantOfShape operation type in the type propagation logic of the ONNX precision converter, returning the dtype of its value attribute to enable correct lower-precision type propagation. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelopt/onnx/autocast/precisionconverter.py (1)
343-352: Add guard for optional ConstantOfShape value attribute.The
valueattribute in ConstantOfShape is optional per the ONNX specification. When omitted, the output defaults to scalar 0 with float32 type. Direct accessnode.attrs["value"]will raiseKeyErrorif the attribute is missing, breaking type propagation for valid ONNX models.Use
node.attrs.get("value")with a fallback tonp.float32, and handle bothgs.Constantand ndarray value types:Proposed fix
elif node.op == "ConstantOfShape": - return node.attrs["value"].dtype + value = node.attrs.get("value") + if value is None: + return np.float32 # ONNX spec default when 'value' is omitted + if isinstance(value, gs.Constant): + return value.values.dtype + if hasattr(value, "dtype"): + return value.dtype + return None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modelopt/onnx/autocast/precisionconverter.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #789 +/- ##
=======================================
Coverage 74.23% 74.24%
=======================================
Files 192 192
Lines 19033 19035 +2
=======================================
+ Hits 14129 14132 +3
+ Misses 4904 4903 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…789) ## What does this PR do? **Type of change:** Bug fix **Overview:** Fixed the output precision of ConstantOfShape layers in models with custom ops. ## Usage <!-- You can potentially add a usage example below. --> ```python $ python -m modelopt.onnx.quantization --onnx_path=$MODEL_NAME.onnx ``` ## Testing See bug 5763424. ## Before your PR is "*Ready for review*" <!-- If you haven't finished some of the above items you can still open `Draft` PR. --> - **Make sure you read and follow [Contributor guidelines](https://github.com/NVIDIA/Model-Optimizer/blob/main/CONTRIBUTING.md)** and your commits are signed. - **Is this change backward compatible?**: Yes - **Did you write any new necessary tests?**: No - **Did you add or update any necessary documentation?**: No - **Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?**: No <!--- Only for new features, API changes, critical bug fixes or bw breaking changes. --> ## Additional Information This issue only affects models with custom ops. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved type propagation handling for ConstantOfShape operations in ONNX autocast, ensuring correct precision type conversion across related operations. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: gcunhase <[email protected]>
What does this PR do?
Type of change: Bug fix
Overview: Fixed the output precision of ConstantOfShape layers in models with custom ops.
Usage
Testing
See bug 5763424.
Before your PR is "Ready for review"
Additional Information
This issue only affects models with custom ops.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.