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.

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

Support corner_pool related custom operators for onnxruntime in mmcv #997

Merged
merged 13 commits into from
May 1, 2021

Conversation

v-qjqs
Copy link
Contributor

@v-qjqs v-qjqs commented Apr 27, 2021

Hi, this PR supports to export corner_pool related custom operators (TopPool,BottomPool,LeftPool,RightPool ) to onnx, and run it with onnxruntime.
It's working in process during my free time.

@v-qjqs v-qjqs changed the title Support corner_pool related custom operators for ONNX Runtime in MMCV Support corner_pool related custom operators for onnxruntime in mmcv Apr 27, 2021
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #997 (68bcc9c) into master (c4aa414) will decrease coverage by 0.03%.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #997      +/-   ##
==========================================
- Coverage   64.60%   64.57%   -0.04%     
==========================================
  Files         152      152              
  Lines        9779     9792      +13     
  Branches     1779     1779              
==========================================
+ Hits         6318     6323       +5     
- Misses       3133     3141       +8     
  Partials      328      328              
Flag Coverage Δ
unittests 64.57% <38.46%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmcv/ops/corner_pool.py 52.77% <38.46%> (-3.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4aa414...68bcc9c. Read the comment docs.

@v-qjqs
Copy link
Contributor Author

v-qjqs commented Apr 28, 2021

Hi, any reviewers?

@grimoire
Copy link
Member

Thanks for the contribution!
Would it be better to add a cummax custom op instead of CornerPool op? It is more general and can support torch>1.5.0.

@v-qjqs
Copy link
Contributor Author

v-qjqs commented Apr 28, 2021

Hi @grimoire, I tested the implemented mmcv::CornerPool with my torch1.5.0 environment and it works. I wonder why this line is needed? Isn't it ok for just using self.corner_pool.apply(x)?

@grimoire
Copy link
Member

PyTorch built-in ops support more platforms and might get better support from different deploy backends in the future.
As we do plan to add more backends support. It is a good idea to avoid custom ops unless we have to do.

Copy link
Member

@grimoire grimoire left a comment

Choose a reason for hiding this comment

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

Hi, could you please add document about your ops in onnxruntime_custom_ops and onnxruntime_op?

mmcv/ops/csrc/onnxruntime/cpu/corner_pool.cpp Outdated Show resolved Hide resolved
mmcv/ops/csrc/onnxruntime/cpu/corner_pool.cpp Outdated Show resolved Hide resolved
@v-qjqs
Copy link
Contributor Author

v-qjqs commented Apr 29, 2021

Thanks for the contribution!
Would it be better to add a cummax custom op instead of CornerPool op? It is more general and can support torch>1.5.0.

The implementation of mmcv::CornerPool helps to those who use self.corner_pool.apply(x) as the forward function.
I'm newer to onnx, perhaps a custom cummax op will be supported in next PR during my free time, following the official pytorch implementation.

docs/onnxruntime_custom_ops.md Outdated Show resolved Hide resolved
docs/onnxruntime_op.md Outdated Show resolved Hide resolved
mmcv/ops/csrc/onnxruntime/cpu/corner_pool.cpp Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Apr 29, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@grimoire grimoire left a comment

Choose a reason for hiding this comment

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

LGTM!

@grimoire
Copy link
Member

@ZwwWayne any comments?

@v-qjqs v-qjqs mentioned this pull request Apr 30, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants