Fix optional input positions in converter - #2961
Fix optional input positions in converter#2961Theodore Cooper (the0cp) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a converter/argument-partitioning bug where omitted optional ONNX inputs were dropped, causing later optional inputs (e.g., Resize(..., sizes=...)) to shift into the wrong input positions and produce invalid models for ONNX Runtime.
Changes:
- Preserve “holes” for omitted optional inputs when a later input is provided (positional or keyword) during argument partitioning.
- Update the converter to accept normalized
Noneplaceholders (in addition to parsedast.Constant(None)) for optional inputs. - Add targeted unit tests and an ONNX Runtime regression test for
Resize(..., sizes=...)ensuring required empty inputs are preserved.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| onnxscript/_internal/param_manipulation.py | Tracks omitted optional inputs and inserts None placeholders only when needed to preserve later input positions. |
| onnxscript/_internal/param_manipulation_test.py | Adds parameterized tests covering preserved optional input positions (including explicit None). |
| onnxscript/_internal/converter.py | Allows _translate_opt_expr to handle None placeholders introduced by normalization. |
| onnxscript/_internal/converter_test.py | Adds an ONNX Runtime regression test validating Resize inputs include the required empty slots. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2961 +/- ##
==========================================
+ Coverage 72.63% 72.65% +0.02%
==========================================
Files 265 265
Lines 32208 32242 +34
Branches 3042 3045 +3
==========================================
+ Hits 23395 23427 +32
- Misses 7779 7780 +1
- Partials 1034 1035 +1 ☔ View full report in Codecov by Harness. |
|
Hi Justin Chu (@justinchuby), could you please review this PR or point me to the right maintainer? Thanks! |
|
The only failing jobs are the three Torch nightly jobs. These appear unrelated to this PR. |
Summary
NoneplaceholdersFixes #2007