-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Add trigonometry functions for ONNX export #7540
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
Conversation
|
I'm just trying to make the support for these functions faster. Feel free to close this if it is more convenient to implement all ONNX opset version bump together in a later time rather than in separate PRs like this. |
|
These seem pretty low risk, even if we don't bump the opset version. |
|
@bddppq It should be easy to add in tree tests for these now, is that right? |
|
@ezyang yes in test/onnx/test_operators.py But note caffe2 currently does not suppory asin, acos, atan and tan yet. Should be easy to add though. |
|
@ezyang these are new ops added in the newest opset, so the pytorch export opset has to be bumped |
|
I think if we add test, we have to bump up the op set version. Otherwise the test will not pass. If needed, I can bump up the version and take care of the other ops changed as specified in ONNX/Changelog.md. But, is it safe to do this now, since ONNX opset version 7 is not stable yet? (In the future, if some changes happens on ONNX, then pytorch exporter might fail automatically (or maybe even worse, silently), and new changes are required to make unit tests pass. This might introduce additional maintenance burden.) |
|
Alright, sounds good, let's wait. (BTW, this is one of the reasons why I originally advocated for having all opset versions in ONNX be stable.) |
|
@zasdfgbnm is this good to merge now? |
bddppq
left a comment
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.
This PR requires bumping the opset version in pytorch exported onnx models from 6 to 7. onnx has introduced some breaking changes (most notably broadcasting semantics) that caffe2 does not fully support at this moment. We are expecting to get the bump in around 1 week, until then we can not merge this PR.
|
@zou3519 But I do think it is time to bump up this version to 7, because ONNX has just released 1.2 with opset version 7. The opset version 7 should be considered stable now, since by @linkerzhang:
|
|
@bddppq Are we ready for opset version bump? |
|
How long is the expected time that it will be bumped to version 7? |
|
Opset version has been bumped to 7 https://github.com/pytorch/pytorch/blob/master/torch/onnx/symbolic.py#L146 |
|
@pytorchbot test this please |
|
@pytorchbot retest this please |
|
@zasdfgbnm could you rebase to master and add test cases in tests/onnx/test_pytorch_onnx_caffe2.py? (test_operators only check the export successfully, it's better to also test the real computation). |
|
@bddppq All done. Should be working now. I have some trouble |
Need to choose better test input data :-) |
|
@pytorchbot retest this please |
|
@bddppq all fixed |
facebook-github-bot
left a comment
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.
bddppq has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
bddppq
left a comment
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.
LG. Thanks!
Summary: Trigonometry functions are newly added to ONNX in a recent PR onnx/onnx#869 This PR makes pytorch support exporting graphs with trigonometry functions. This PR might need to wait until it is ready to change ```python _onnx_opset_version = 6 ``` to ```python _onnx_opset_version = 7 ``` Pull Request resolved: pytorch#7540 Differential Revision: D9395041 Pulled By: bddppq fbshipit-source-id: bdf3e9d212b911c8c4eacf5a0753bb092e4748d2
Trigonometry functions are newly added to ONNX in a recent PR onnx/onnx#869
This PR makes pytorch support exporting graphs with trigonometry functions.
This PR might need to wait until it is ready to change
to