-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
[ONNX] Support exporting aten::copy_ and aten::index_put to ONNX opset 11 #26941
Conversation
a782a1b
to
990a0e1
Compare
cf89f4e
to
d0207c7
Compare
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
Thanks for the comments, they make the code easy to follow.
The code is clean and looks good, I left some minor comments.
6213678
to
d18c6d2
Compare
Looking at failed tests after rebasing with master. |
The failed tests are related to this issue #29008. The traced graph produced by tracer is empty, resulting in incorrect ONNX graph. |
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
e15f103
to
84c0523
Compare
@pytorchbot retest this please |
@pytorchbot stopped working. a manual rebase? |
84c0523
to
a6d8571
Compare
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
Shall we just abandonrebase this PR?
4fd69f6
to
eff8cb4
Compare
5baa626
to
ff5fdad
Compare
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.
@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Looks good |
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.
LGTM thanks!
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.
Looks good.
@houseroad merged this pull request in 63f1b78. |
…ytorch#26941) Summary: - [x] Add more comments and refactor the logic of `ReshapeToAdvancedIndexingFormat` - [x] Add more description here. Cases that are/aren't supported, and how they are supported. - [x] Need to merge this PR pytorch#27186 to enable testing inplace operators. We are now supporting exporting aten::copy_ and aten::index_put to ONNX. Here's a breakdown of the different cases in PyTorch code. ``` # Case 1: Scalar Indices x[0, 1, 2] = data # Case 2: Slice Indices x[1:3, :, ::2] = data # Case 3: Ellipsis Indices x[..., 0] = data # Case 4: Tensor Indices ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) x[ind1, ind2] = data # Case 5: Mixing all the above cases ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) x[1:3, ind1, ind2, ..., 3] = data ``` Limitations: Tensor indices must be consecutive, and 1-d tensors. ``` # Supported ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) x[ind1, ind2] = data # Not supported ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) ind3 = torch.tensor([[0], [1]]) x[ind1, :, ind2] = data x[ind3] = data ``` Negative indices are not supported. ``` # Not supported x[-1] = data ``` Pull Request resolved: pytorch#26941 Differential Revision: D17951030 Pulled By: houseroad fbshipit-source-id: 4357777072f53aa0bc4b297aa1ee53457a7f8dec
…ytorch#26941) Summary: - [x] Add more comments and refactor the logic of `ReshapeToAdvancedIndexingFormat` - [x] Add more description here. Cases that are/aren't supported, and how they are supported. - [x] Need to merge this PR pytorch#27186 to enable testing inplace operators. We are now supporting exporting aten::copy_ and aten::index_put to ONNX. Here's a breakdown of the different cases in PyTorch code. ``` # Case 1: Scalar Indices x[0, 1, 2] = data # Case 2: Slice Indices x[1:3, :, ::2] = data # Case 3: Ellipsis Indices x[..., 0] = data # Case 4: Tensor Indices ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) x[ind1, ind2] = data # Case 5: Mixing all the above cases ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) x[1:3, ind1, ind2, ..., 3] = data ``` Limitations: Tensor indices must be consecutive, and 1-d tensors. ``` # Supported ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) x[ind1, ind2] = data # Not supported ind1 = torch.tensor([0, 2]) ind2 = torch.tensor([1, 1]) ind3 = torch.tensor([[0], [1]]) x[ind1, :, ind2] = data x[ind3] = data ``` Negative indices are not supported. ``` # Not supported x[-1] = data ``` Pull Request resolved: pytorch#26941 Differential Revision: D17951030 Pulled By: houseroad fbshipit-source-id: 4357777072f53aa0bc4b297aa1ee53457a7f8dec
ReshapeToAdvancedIndexingFormat
We are now supporting exporting aten::copy_ and aten::index_put to ONNX.
Here's a breakdown of the different cases in PyTorch code.
Limitations:
Tensor indices must be consecutive, and 1-d tensors.
Negative indices are not supported.