[6771663] Preserve ONNX API output types when wiring casts - #2451
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughFP16 conversion now uses preserved API-boundary I/O types when inserting output casts. A regression test verifies that a declared ChangesONNX output type preservation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The conversion change preserves declared ONNX output types and includes regression coverage for the reported inference scenario. No unresolved merge-blocking issue remains. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Approving: a small, correctly scoped fix that sources original_network_io from the boundary metadata instead of the post-inference working declaration, with a regression test and changelog entry.
No action needed.
- Checked the interaction with
_add_cast's existingmust_preserve_io_typeguard and_sanity_check; both now read the same boundary map, and the fallback whenoriginal_network_io_metadata is None(i.e.keep_io_types=False) reproduces the previous behavior exactly. - Cast-up/cast-down ordering for an output present in both lists still resolves to the boundary type, which is the intended outcome.
![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 #2451 +/- ##
==========================================
+ Coverage 71.50% 78.83% +7.33%
==========================================
Files 590 590
Lines 64749 64944 +195
==========================================
+ Hits 46297 51198 +4901
+ Misses 18452 13746 -4706
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:
|
Signed-off-by: Ajinkya Rasane <ajinkyaashwin@gmail.com>
Signed-off-by: Ajinkya Rasane <ajinkyaashwin@gmail.com>
ad24f7f to
aeb0e2c
Compare
|
/ok to test aeb0e2c |
3 participants
What does this PR do?
Type of change: Bug fix
Preserves the public ONNX graph I/O types captured at the API boundary when
PrecisionConverterwires output casts. Type inference can change the working graph's output declaration before conversion; consulting that mutated declaration caused the required cast back to the original public type to be discarded and metadata restoration to fail.The converter now derives its I/O type map from the preserved boundary metadata and uses that map when deciding whether a cast should become a public graph output. A regression test covers an FP32 output whose working declaration is inferred as FP16, and the changelog records the corrected behavior.
Usage
Testing
pytest tests/unit/onnx/autocast/test_precisionconverter.py(186 passed).pytest tests/unit/onnx/autocast(249 passed).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/AAdditional Information
Tracking: [6771663]
Summary by CodeRabbit
Bug Fixes
Tests