[ONNX] Remove conditional import of onnxruntime #4185
Conversation
garymm
force-pushed
the
conditional-ort-import
branch
from
3449adf to
6400b4c
Compare
There was a problem hiding this comment.
Thanks @garymm , LGTM with a minor comment: I think we don't need to hide the tests that are skipped in the CI
| # test_onnx requires additional packages to be installed and has its own pipeline. | ||
| pytest --cov=torchvision --junitxml=test-results/junit.xml -v --durations 20 test \ | ||
| --ignore=test/test_datasets_download.py \ | ||
| --ignore=test/test_onnx.py |
There was a problem hiding this comment.
Let's remove this: I think it's actually valuable to see the tests being skipped in the CI: it shows they exist, and that they're skipped. If anything, it shows that the skipif logic actually works as expected.
As you said they're being run by another CI run anyway so there's no risk of missing them.
There was a problem hiding this comment.
Without this change we can't remove the try/except around the import, so your "LGTM with a minor comment" is a bit confusing.
Anyways I've reverted the run_test.sh changes and now all this PR does is update a comment.
I'd still prefer to remove the try/except, so let me know if I can do that.
There was a problem hiding this comment.
Sorry, I forgot that part.
If you wish to remove the try/except, we should be able to convert it into importorskip: https://docs.pytest.org/en/latest/how-to/skipping.html#skipping-on-a-missing-import-dependency
garymm
force-pushed
the
conditional-ort-import
branch
4 times, most recently
from
4b6bb75 to
91b7aeb
Compare
test/test_onnx.py
Outdated
| # This import should be before that of torch | ||
| # see https://github.com/onnx/onnx/issues/2394#issuecomment-581638840 |
There was a problem hiding this comment.
Thanks @garymm , looks good. Do you know if this comment is still relevant? The original issue was closed so I feel like we should be able to remove the comment and do that importskip lower down, where it will be easier to find.
Do you mind trying that, and if there are still issue, we can put it back?
There was a problem hiding this comment.
Thanks for checking this. Done.
The comment indicates that this was put in place to support older versions of python, but torchvision now requires python >=3.6. Update the comment to reflect the real reason we use a conditional import, and use pytest.importorskip to make the code a bit shorter and more obvious.
garymm
force-pushed
the
conditional-ort-import
branch
from
91b7aeb to
4c804a9
Compare
…ic (#4185) Reviewed By: fmassa Differential Revision: D29932700 fbshipit-source-id: f63847dfc774485576101c9f8551e4af8606bd75
The comment indicates that this was put in place to support older
versions of python, but torchvision now requires python >=3.6.
Instead of having the test pipeline invoke test_onnx.py and just have
it skip all the tests contained therein, instead configure the pipeline
to not invoke test_onnx.py. This is better because otherwise people
may be given the false impression that the onnx tests are running and
passing, when in fact they're being skipped.