-
Notifications
You must be signed in to change notification settings - Fork 94
Set 0d shape to python constant tensors #1918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set 0d shape to python constant tensors #1918
Conversation
onnxscript/function_libs/torch_lib/graph_building/_graph_building_torch.py
Fixed
Show fixed
Hide fixed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1918 +/- ##
==========================================
- Coverage 75.49% 75.49% -0.01%
==========================================
Files 252 252
Lines 27346 27355 +9
Branches 3172 3176 +4
==========================================
+ Hits 20645 20651 +6
Misses 5740 5740
- Partials 961 964 +3 ☔ View full report in Codecov by Sentry. |
| start = op.Reshape(start, op.Constant(value_ints=[-1])) | ||
| length = op.Reshape(length, op.Constant(value_ints=[-1])) | ||
| if IsScalar(dim): | ||
| dim = op.Reshape(dim, op.Constant(value_ints=[-1])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest changing the -1 to 1 ... since it is going to have the shape [1]. Ensures better shape inference with all of the various shape-inference implementations around.
| dim = op.Reshape(dim, op.Constant(value_ints=[-1])) | ||
|
|
||
| if IsScalar(start): | ||
| start = op.Reshape(start, op.Constant(value_ints=[-1])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, here and below
| attributes=dict(value=constant_tensor), | ||
| )[0] | ||
| value.setDebugName(_rename_intermediate_value(value.debugName())) | ||
| if shape is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest not touching this file as it is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest doing this differently. The shape info may not be needed?
This may be low priority. It all starts #1916 that I re-wrote aten_narrow to adapt the change that I moved YracedONNXFunction to promote python constants to tensors before function call. With doing that, IsScalar loses its functionnality because we didn't assign shape to tensors. That's why I create this PR. |
|
Leave it as it is for now. For torchlib, we keep the implementation, and CI tests will be deprecated soon with OpSignature PR. |
From #1916, it is found that the undefined shape disables the functionality of IsScalar op:
onnxscript/onnxscript/function_libs/torch_lib/graph_building/_graph_building_torch.py
Line 357 in 561a600