Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ONNX] Support exporting RoiAlign align=True to ONNX with opset 16 #6685

Merged
merged 3 commits into from
Oct 4, 2022
Merged

[ONNX] Support exporting RoiAlign align=True to ONNX with opset 16 #6685

merged 3 commits into from
Oct 4, 2022

Conversation

BowenBao
Copy link
Contributor

@BowenBao BowenBao commented Oct 3, 2022 •
edited
Loading

Long over due, but it's here.
ONNX upgrades its 'RoiAlign' spec in opset 16 to support align=True via 'coordinate_transformation_mode'.
This PR adds support to export 'RoiAlign' under opset 16, and re-enabled disabled tests.

Together with pytorch/pytorch#86169 will fix pytorch/pytorch#81121

This was referenced Oct 4, 2022
Closed
Closed
Closed
datumbox
datumbox approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @BowenBao.

We can merge on green CI.

@datumbox datumbox merged commit 45f87fa into pytorch:main Oct 4, 2022
BowenBao added a commit to pytorch/pytorch that referenced this pull request Oct 4, 2022
@BowenBao
Update base for Update on "[ONNX] Partially re-enable RoiAlign and Ro…
7e1e6a7 
…iPool unit tests"


This PR depends on pytorch/vision#6685


[ghstack-poisoned]
BowenBao added a commit to pytorch/pytorch that referenced this pull request Oct 4, 2022
@BowenBao
Update on "[ONNX] Partially re-enable RoiAlign and RoiPool unit tests"
da899fb 
This PR depends on pytorch/vision#6685


[ghstack-poisoned]
BowenBao added a commit to pytorch/pytorch that referenced this pull request Oct 4, 2022
@BowenBao
Update base for Update on "[ONNX] Partially re-enable RoiAlign and Ro…
d061294 
…iPool unit tests"


This PR depends on pytorch/vision#6685


[ghstack-poisoned]
BowenBao added a commit to pytorch/pytorch that referenced this pull request Oct 4, 2022
@BowenBao
Update on "[ONNX] Partially re-enable RoiAlign and RoiPool unit tests"
5a05ad9 
This PR depends on pytorch/vision#6685


[ghstack-poisoned]
jdsgomes pushed a commit to jdsgomes/vision that referenced this pull request Oct 4, 2022
@BowenBao @datumbox @jdsgomes
[ONNX] Support exporting RoiAlign align=True to ONNX with opset 16 (p…
e23e300 
…ytorch#6685)

* Support exporting RoiAlign align=True to ONNX with opset 16

* lint: ufmt

Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
@NicolasHug @datumbox @facebook-github-bot
[fbsync] [ONNX] Support exporting RoiAlign align=True to ONNX with op…
c9174e4 
…set 16 (#6685)

Summary:
* Support exporting RoiAlign align=True to ONNX with opset 16

* lint: ufmt

Reviewed By: datumbox

Differential Revision: D40138746

fbshipit-source-id: 06dcd122e762c02491fd68a864773894b527b854

Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
@BowenBao
Copy link
Contributor Author

@datumbox Can this be included in 0.14 release such that PyTorch 1.13 release has the fix?

@datumbox
Copy link
Contributor

@BowenBao I just checked and the PR was merged after the release cut. As this is a feature and not a bug fix, we would probably need to understand a bit better the confidence you have over this feature (for full transparency we are not ONNX power-users). Do you think this is safe to merge on the release branch? Have you tried exporting any model that uses the op to confirm? Thanks in advance.

cc @YosuaMichael who is the POC for this release.

BowenBao added a commit to pytorch/pytorch that referenced this pull request Oct 11, 2022
@BowenBao
Update base for Update on "[ONNX] Partially re-enable RoiAlign and Ro…
a39fd7e 
…iPool unit tests"


This PR depends on pytorch/vision#6685


[ghstack-poisoned]
BowenBao added a commit to pytorch/pytorch that referenced this pull request Oct 11, 2022
@BowenBao
Update on "[ONNX] Partially re-enable RoiAlign and RoiPool unit tests"
64c9594 
This PR depends on pytorch/vision#6685


[ghstack-poisoned]
@YosuaMichael
Copy link
Contributor

@BowenBao @datumbox Could you explain more on whether this PR is a bugfix (something will break if we dont include this PR), or is it more on feature (nothing broken if this PR is not included)?

I am not that familiar with ONNX as well, so need to know more detail on this. Also agree with @datumbox , would be great to know how safe is this and what test has been done to make sure it is safe.

@BowenBao
Copy link
Contributor Author

BowenBao commented Oct 11, 2022 •
edited
Loading

@BowenBao @datumbox Could you explain more on whether this PR is a bugfix (something will break if we dont include this PR), or is it more on feature (nothing broken if this PR is not included)?

Hi @YosuaMichael, this PR enables export of RoiAlign to ONNX on opset >= 16. For opset >= 16, export is broken without this PR. Also, aligned=True is only supported in opset >= 16, hence export is broken for aligned=True without this PR. For opset < 16, there is no change.
So, it's a bit of both, I'm more inclined to bugfix as this was breaking some unit tests in PyTorch for a while.

The change is covered by the test cases within this PR, as well as test cases for RoiAlign in pytorch/pytorch#86169 (the vision version used is after this commit), alongside existing coverages of vision models (https://github.com/pytorch/pytorch/blob/693250ac859fbb15ea4b4426d9cefaf97e151eb7/test/onnx/test_models_onnxruntime.py#L207).

If this is to be included in release, it'd be nice to also include 0e006a9 which does a bit of rephrasing for the warning message..

pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Oct 13, 2022
@BowenBao @pytorchmergebot
[ONNX] Partially re-enable RoiAlign and RoiPool unit tests (#86169)
45274c5 
This PR depends on pytorch/vision#6685

Pull Request resolved: #86169
Approved by: https://github.com/justinchuby, https://github.com/AllenTiTaiWang, https://github.com/abock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinchuby justinchuby justinchuby left review comments

@datumbox datumbox datumbox approved these changes

Assignees
No one assigned
Labels
cla signed enhancement module: onnx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ONNX tests fail after torchvision update
5 participants
@BowenBao @datumbox @YosuaMichael @justinchuby @facebook-github-bot