Skip to content

Conversation

@zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented May 13, 2018

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

_onnx_opset_version = 6

to

_onnx_opset_version = 7

@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented May 13, 2018

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.

@ezyang
Copy link
Contributor

ezyang commented May 14, 2018

These seem pretty low risk, even if we don't bump the opset version.

@ezyang
Copy link
Contributor

ezyang commented May 14, 2018

@bddppq It should be easy to add in tree tests for these now, is that right?

@bddppq
Copy link
Contributor

bddppq commented May 14, 2018

@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.

@bddppq
Copy link
Contributor

bddppq commented May 14, 2018

@ezyang these are new ops added in the newest opset, so the pytorch export opset has to be bumped

@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented May 14, 2018

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.)

@ezyang
Copy link
Contributor

ezyang commented May 15, 2018

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.)

@zou3519
Copy link
Contributor

zou3519 commented May 29, 2018

@zasdfgbnm is this good to merge now?

bddppq
bddppq previously requested changes May 29, 2018
Copy link
Contributor

@bddppq bddppq left a 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.

@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented May 29, 2018

@zou3519
I don't think it it good to merge now, because we still have in _onnx_opset_version = 6 in https://github.com/pytorch/pytorch/blob/master/torch/onnx/symbolic.py#L93

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:

Operator Changes
Latest operator set version is 8 for ONNX domain and 1 for ONNX_ML domain.

@ezyang
Copy link
Contributor

ezyang commented Jun 6, 2018

@bddppq Are we ready for opset version bump?

@sgarcia22
Copy link

How long is the expected time that it will be bumped to version 7?

@fmassa
Copy link
Member

fmassa commented Aug 14, 2018

Opset version has been bumped to 7 https://github.com/pytorch/pytorch/blob/master/torch/onnx/symbolic.py#L146

@soumith
Copy link
Contributor

soumith commented Aug 14, 2018

@pytorchbot test this please

@bddppq
Copy link
Contributor

bddppq commented Aug 15, 2018

@pytorchbot retest this please

@bddppq
Copy link
Contributor

bddppq commented Aug 15, 2018

@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 bddppq dismissed their stale review August 15, 2018 18:38

opset bumpted to 7 in master

@bddppq
Copy link
Contributor

bddppq commented Aug 15, 2018

Need to update the expect files: https://ci.pytorch.org/jenkins/job/caffe2-builds/job/onnx-py2-gcc5-ubuntu16.04-test/2003/console

@zasdfgbnm zasdfgbnm requested a review from Yangqing as a code owner August 17, 2018 15:59
@zasdfgbnm
Copy link
Collaborator Author

zasdfgbnm commented Aug 17, 2018

@bddppq All done. Should be working now. I have some trouble pip install caffe2 and python setup_caffe2.py install, so I can not test it on my computer. And the CI system seems to have some trouble right now...

@bddppq
Copy link
Contributor

bddppq commented Aug 17, 2018

17:35:10 >       test_func('acos')
...
17:35:10 E               AssertionError: 
17:35:10 E               Not equal to tolerance rtol=0.001, atol=1e-07
17:35:10 E               
17:35:10 E               (mismatch 100.0%)
17:35:10 E                x: array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan,
17:35:10 E                      nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan,
17:35:10 E                      nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan,...
17:35:10 E                y: array([nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan,
17:35:10 E                      nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan,
17:35:10 E                      nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan, nan,...

Need to choose better test input data :-)

@bddppq
Copy link
Contributor

bddppq commented Aug 17, 2018

@pytorchbot retest this please

@zasdfgbnm
Copy link
Collaborator Author

@bddppq all fixed

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

Copy link
Contributor

@bddppq bddppq left a comment

Choose a reason for hiding this comment

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

LG. Thanks!

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants