[ONNX] Slightly improve indexing with ellipsis under scripting #46571
Conversation
💊 CI failures summary and remediationsAs of commit ada6f36 (more details on the Dr. CI page): Commit ada6f36 was recently pushed. Waiting for builds... This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 1 time. |
facebook-github-bot
added
the
oncall: jit
Codecov Report
@@ Coverage Diff @@
## master #46571 +/- ##
=======================================
Coverage 68.40% 68.40%
=======================================
Files 411 411
Lines 54098 54098
=======================================
+ Hits 37006 37007 +1
+ Misses 17092 17091 -1 |
ezyang
added
the
triaged
| @skipIfUnsupportedMinOpsetVersion(11) | ||
| @disableScriptTest() # Missing input size (with ellipsis indexing) | ||
| # TODO: Limited scripting support with ellipsis indexing. | ||
| # Due to dependency on input tensor rank being known. |
There was a problem hiding this comment.
does this mean test will pass only when shape_inference is turned on?
There was a problem hiding this comment.
no, shape inference won't affect this case. I will add stricter test case which require shape inference, with the PR of enabling passes dependent on shape/type info.
neginraoof
left a comment
neginraoof
left a comment
There was a problem hiding this comment.
Perfect, looks good. Thanks for enabling this.
Can you let us know which cases are not supported, i.e. shape cannot be inferred?
Is it a known situation, or potential case?
It's not supported when rank cannot be inferred. I think this is a potential case for now. |
neginraoof
left a comment
neginraoof
left a comment
There was a problem hiding this comment.
Looks good. Can we keep a (disabled) test case for the unsupported case?
|
Hi @BowenBao! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but we do not have a signature on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
7 participants
Still depending on rank of original tensor being known. i.e.