Repository navigation
Fix Windows ONNX external data corruption by upgrading ONNX - #2816
Conversation
Signed-off-by: anmaurya001 <anmaurya@nvidia.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
ONNX dependency and external-data regression test pyproject.toml, docs/source/getting_started/windows/_installation_windows_arm64.rst, tests/unit/onnx/test_external_data.py, CHANGELOG.rst |
The optional ONNX dependency uses version 1.23.2 on Windows and 1.21.0 on other platforms. The Windows ARM64 installation command and changelog are updated. A parametrized test checks external data bytes for three final-byte values and verifies that loading restores both tensors. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- NVIDIA/Model-Optimizer#2939: Adds shared ONNX model saving with external sidecar output and tests external tensor-data round trips, related to the same serialization behavior.
Merge Risk: 
3d639
-
No actionable merge-blocking risk remains in the reviewed changes.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: upgrading ONNX to fix Windows external-data corruption. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Anti-Patterns PASS. The PR changes only one Python file, tests/unit/onnx/test_external_data.py, and it adds notorch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardcoded `trust_remote_…Full details: Security Anti-Patterns
-
Explanation
PASS. The PR changes only one Python file,
tests/unit/onnx/test_external_data.py, and it adds notorch.load(..., weights_only=False),numpy.load(..., allow_pickle=True), hardcodedtrust_remote_code=True,eval(),exec(), or# nosec. Thepyproject.tomlchange only splits the existingonnxdependency by Windows platform and updates its version; it does not add a new dependency.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
| "ml_dtypes", | ||
| "onnx-graphsurgeon>=0.6.1", | ||
| "onnx~=1.21.0", | ||
| "onnx~=1.23.2", |
There was a problem hiding this comment.
@ajrasane does this affect Linux onnx features?
|
/ok to test 50eecd0 |
Signed-off-by: anmaurya001 <anmaurya@nvidia.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2816 +/- ##
==========================================
- Coverage 79.82% 79.11% -0.71%
==========================================
Files 661 661
Lines 74928 74928
==========================================
- Hits 59810 59282 -528
- Misses 15118 15646 +528
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
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5-5) — DM the bot to share feedback.
The ONNX bump and the regression test look correct, but the Windows ARM64 install guide still pins the ONNX version that has the corruption bug, so the fix only reaches part of the affected Windows users.
Needs action:
- Update
"onnx~=1.21.0"to"onnx~=1.23.2"in the manualpip installblock ofdocs/source/getting_started/windows/_installation_windows_arm64.rst. Native Windows ARM64 users install from that command, so they would still get corrupted external data. - Confirm
tests/unit/onnx/test_external_data.pyruns in a Windows CI job. On Linux the test passes with either ONNX version, so it only catches this regression on Windows. - Confirm
onnxconverter-common~=1.16.0,onnx-graphsurgeon,polygraphyand the pinnedonnxruntime-gpu==1.22.0(Windows x64) all work with ONNX 1.23.x. Your validation was a single end-to-end run.
No action needed:
- The test matches how
save_onnxwrites external data: the file is namedbasename + "_data"andsize_threshold=1024. Each 1 KiB tensor gets written out, and both are well below ONNX's alignment threshold, so the expected bytes are just the two tensors back to back. - The new test file's header is the standard NVIDIA Apache-2.0 header. No other licensing changes.
Signed-off-by: anmaurya001 <anmaurya@nvidia.com>
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5-5) — DM the bot to share feedback.
Nudge: the earlier concerns are fixed, but a maintainer's question about Linux impact is still unanswered. The ONNX owner needs to confirm the 1.21 → 1.23 bump before this merges.
Needs action:
- Confirm the 1.21 → 1.23 ONNX bump doesn't regress Linux ONNX features. @kevalmorabia97 asked @ajrasane on
pyproject.toml:61and nobody has replied yet. The compatibility checks the author reported were local only, and the lockfile is deliberately left to the weekly bump workflow.
No action needed:
- ✔️ Resolved since the last review:
- The ARM64 install guide now pins
onnx~=1.23.2. - The CHANGELOG entry sits under 0.48.0 Bug Fixes, as requested.
- The new test runs on Windows: the
windowsjob inunit_tests.ymlruns the full unit suite (not merge-gating), and the author reports all three cases passed there.
- The ARM64 install guide now pins
- The new test in
tests/unit/onnx/test_external_data.pymatches howsave_onnxwrites external data, and it targets the trailing-0x1Atruncation. Its header is the standard NVIDIA Apache-2.0 header.
Signed-off-by: anmaurya001 <anmaurya@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Signed-off-by: anmaurya001 <anmaurya@nvidia.com>
|
/ok to test 3d63956 |
@kevalmorabia97, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 3d63956 |
What does this PR do?
Type of change: Bug fix
Upgrade the ONNX extra from
~=1.21.0to the locally validated~=1.23.2release line on Windows only. Non-Windows platforms retain~=1.21.0, as requested in review. On Windows, exporting multiple tensors into shared external data with ONNX 1.21.0 can truncate a trailing0x1Abyte when reopening the file. Subsequent tensors overwrite earlier data, corrupting quantized weights and scales. This can produce incorrect output and runaway generation that eventually overflows the conversation context.ONNX already corrected Windows binary-mode file opening upstream in onnx/onnx#7790. This change consumes the released fix and adds a regression test through ModelOpt's
save_onnx, checking shared-file bytes and tensor round trips. Existing corrupted exports must be regenerated from clean weights.The lockfile is intentionally left to the weekly workflow, as required by CONTRIBUTING.md.
Usage
Reinstall the ONNX extra with the updated dependency and regenerate affected exports from clean weights. No API changes.
Testing
0x1Acase (byte 1023 is overwritten), while the two control cases pass.python -m pytest -o addopts='' --noconftest tests/unit/onnx/test_external_data.py -q. The normal ONNX autouse C++ compilation fixture failed in this local environment; this run excludes repository conftest fixtures and optional coverage hooks.Before your PR is "Ready for review"
Additional Information
Addresses NVBugs 6309022 and 6561663 through the shared Windows external-data corruption fix. Local evidence supports this cause; original CI acceptance remains pending.
Summary by CodeRabbit