Added workaround to onnx.helper function depreciated in onnx 1.21#1204
Merged
hthadicherla merged 1 commit intomainfrom Apr 8, 2026
Merged
Added workaround to onnx.helper function depreciated in onnx 1.21#1204hthadicherla merged 1 commit intomainfrom
hthadicherla merged 1 commit intomainfrom
Conversation
…this is depreciated in onnx 1.21 Signed-off-by: Hrishith Thadicherla <hthadicherla@nvidia.com>
Contributor
📝 WalkthroughWalkthroughTwo requirements files have unpinned ONNX dependencies from version 1.19.0 to accept any version. A compatibility shim was added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
hthadicherla
changed the title
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
examples/windows/onnx_ptq/whisper/requirements.txt (1)
7-7: Consider adding a lower version bound foronnx.Completely unpinning the dependency could lead to installing incompatible older versions or future breaking versions. Consider specifying at minimum a lower bound (e.g.,
onnx>=1.19.0) to ensure compatibility with the codebase while still allowing 1.21+.💡 Suggested constraint
-onnx +onnx>=1.19.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/windows/onnx_ptq/whisper/requirements.txt` at line 7, The requirements file currently lists an unpinned dependency "onnx"; update examples/windows/onnx_ptq/whisper/requirements.txt to add a sensible lower bound (e.g., change "onnx" to "onnx>=1.19.0" or "onnx>=1.21.0" depending on tested compatibility) so older incompatible versions are not installed; ensure the new constraint is the only change and matches the tested runtime for the whisper/onnx_ptq example.modelopt/onnx/__init__.py (1)
21-31: Add a comment explaining this workaround.The monkey-patch works, but future maintainers would benefit from a brief comment explaining why this exists and linking to the upstream issue (onnx_graphsurgeon needs to be updated).
📝 Suggested documentation
import onnx.helper +# Workaround: onnx 1.21 removed onnx.helper.float32_to_bfloat16, but onnx_graphsurgeon +# still depends on it. Provide a compatible implementation until onnx_graphsurgeon is updated. +# See: https://github.com/NVIDIA/TensorRT/issues/... (link to upstream issue if available) if not hasattr(onnx.helper, "float32_to_bfloat16"): import ml_dtypes import numpy as np def _float32_to_bfloat16(value): + """Convert a scalar float32 value to its bfloat16 representation as uint16.""" arr = np.array(value, dtype=np.float32) return int(arr.astype(ml_dtypes.bfloat16).view(np.uint16)) onnx.helper.float32_to_bfloat16 = _float32_to_bfloat16🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelopt/onnx/__init__.py` around lines 21 - 31, Add a short explanatory comment above the monkey-patch in modelopt/onnx/__init__.py describing that onnx.helper.float32_to_bfloat16 is missing in some ONNX builds, that this runtime fallback (_float32_to_bfloat16 using ml_dtypes and numpy) restores expected behavior for callers, and reference the upstream onnx_graphsurgeon issue/PR that requires updating; place the comment adjacent to the onnx.helper.float32_to_bfloat16 assignment and mention the involved symbols (_float32_to_bfloat16, ml_dtypes, onnx_graphsurgeon) so future maintainers know why and where to remove it.examples/windows/onnx_ptq/genai_llm/requirements.txt (1)
2-2: Consider adding a lower version bound foronnx.Same as the whisper requirements file - consider specifying at minimum a lower bound to ensure compatibility while allowing newer versions.
💡 Suggested constraint
-onnx +onnx>=1.19.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/windows/onnx_ptq/genai_llm/requirements.txt` at line 2, The requirements line currently lists only "onnx" with no lower bound; update the dependency entry in requirements.txt to include a minimum compatible version (for example change "onnx" to "onnx>=1.14.0" or match the lower bound used in the whisper requirements) so consumers get a known-compatible ONNX release while still allowing newer versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/windows/onnx_ptq/genai_llm/requirements.txt`:
- Line 2: The requirements line currently lists only "onnx" with no lower bound;
update the dependency entry in requirements.txt to include a minimum compatible
version (for example change "onnx" to "onnx>=1.14.0" or match the lower bound
used in the whisper requirements) so consumers get a known-compatible ONNX
release while still allowing newer versions.
In `@examples/windows/onnx_ptq/whisper/requirements.txt`:
- Line 7: The requirements file currently lists an unpinned dependency "onnx";
update examples/windows/onnx_ptq/whisper/requirements.txt to add a sensible
lower bound (e.g., change "onnx" to "onnx>=1.19.0" or "onnx>=1.21.0" depending
on tested compatibility) so older incompatible versions are not installed;
ensure the new constraint is the only change and matches the tested runtime for
the whisper/onnx_ptq example.
In `@modelopt/onnx/__init__.py`:
- Around line 21-31: Add a short explanatory comment above the monkey-patch in
modelopt/onnx/__init__.py describing that onnx.helper.float32_to_bfloat16 is
missing in some ONNX builds, that this runtime fallback (_float32_to_bfloat16
using ml_dtypes and numpy) restores expected behavior for callers, and reference
the upstream onnx_graphsurgeon issue/PR that requires updating; place the
comment adjacent to the onnx.helper.float32_to_bfloat16 assignment and mention
the involved symbols (_float32_to_bfloat16, ml_dtypes, onnx_graphsurgeon) so
future maintainers know why and where to remove it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8003d185-395c-417c-afb2-f5eefda83238
📒 Files selected for processing (3)
examples/windows/onnx_ptq/genai_llm/requirements.txtexamples/windows/onnx_ptq/whisper/requirements.txtmodelopt/onnx/__init__.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
+ Coverage 71.86% 76.94% +5.08%
==========================================
Files 352 352
Lines 40343 40351 +8
==========================================
+ Hits 28992 31050 +2058
+ Misses 11351 9301 -2050
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5 tasks
| @@ -1,4 +1,4 @@ | |||
| datasets>=2.14.5 | |||
| onnx==1.19.0 | |||
| onnx | |||
Contributor
There was a problem hiding this comment.
Should we keep this unconditional (no version cap) or should this be like onnx >= 1.19.0? Not so sure of the down side though, unless it gets older incompatible version.
(same for other such places)
Contributor
Author
There was a problem hiding this comment.
I think if onnx is not installed . Atleast for python >=3.10 it will install the latest version , if user is installing via requirements.txt for the first time. If onnx is installed via pyproject.toml , currently it will install 1.19 onnx which is fine i think .
vishalpandya1990
approved these changes
Apr 8, 2026
Contributor
|
Just to note, another older onnx-incompatibility issue in TRT, onnx-graphsurgeon - NVIDIA/TensorRT#4653 |
Contributor
Author
Yeah i saw this, but they reverted the change in this PR NVIDIA/TensorRT#4688. There seems to be some test failures there. |
Contributor
Author
|
Merging this PR |
Edwardf0t1
pushed a commit
that referenced
this pull request
Apr 9, 2026
) ### What does this PR do? Type of change: Bug Fix With onnx 1.21 and onnx graphsurgeon there is a incompatibility issue where onnx discontinued the float32 to bfloat16 helper , thereby causing an import failure while importing onnx graph surgeon which has that function dependency. ``` Traceback (most recent call last): File "C:\Users\local-hthadicherla\Downloads\ModelOpt_Bench\Model-Optimizer\examples\windows\onnx_ptq\genai_llm\quantize.py", line 29, in <module> from modelopt.onnx.quantization.int4 import quantize as quantize_int4 File "C:\Users\local-hthadicherla\Downloads\ModelOpt_Bench\Model-Optimizer\modelopt\onnx\__init__.py", line 36, in <module> from . import quantization File "C:\Users\local-hthadicherla\Downloads\ModelOpt_Bench\Model-Optimizer\modelopt\onnx\quantization\__init__.py", line 19, in <module> from .int4 import quantize as quantize_int4 File "C:\Users\local-hthadicherla\Downloads\ModelOpt_Bench\Model-Optimizer\modelopt\onnx\quantization\int4.py", line 30, in <module> import onnx_graphsurgeon as gs File "C:\Users\local-hthadicherla\Downloads\ModelOpt_Bench\venv\Lib\site-packages\onnx_graphsurgeon\__init__.py", line 1, in <module> from onnx_graphsurgeon.exporters.onnx_exporter import export_onnx File "C:\Users\local-hthadicherla\Downloads\ModelOpt_Bench\venv\Lib\site-packages\onnx_graphsurgeon\exporters\onnx_exporter.py", line 134, in <module> np.uint16, onnx.helper.float32_to_bfloat16 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: module 'onnx.helper' has no attribute 'float32_to_bfloat16' ``` Added mldtypes converter and overrided the helper to use this workaround function as onnx.helper.float32_to_bfloat16. Long term fix is an AI for TensorRT team to fix this within onnx graph surgeon i guess. ### Testing Tested quantization works after fix with onnx 1.21. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Relaxed ONNX version constraints in example configurations to support a wider range of compatible versions. * **Bug Fixes** * Added compatibility support for ONNX to ensure proper functionality across different library versions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Hrishith Thadicherla <hthadicherla@nvidia.com>
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
2 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
With onnx 1.21 and onnx graphsurgeon there is a incompatibility issue where onnx discontinued the float32 to bfloat16 helper , thereby causing an import failure while importing onnx graph surgeon which has that function dependency.
Added mldtypes converter and overrided the helper to use this workaround function as onnx.helper.float32_to_bfloat16.
Long term fix is an AI for TensorRT team to fix this within onnx graph surgeon i guess.
Testing
Tested quantization works after fix with onnx 1.21.
Summary by CodeRabbit
Chores
Bug Fixes