You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/02/10 21:17:43 UTC
[GitHub] [tvm] electriclilies opened a new pull request #7438: [ONNX] Enable GPU in ONNX importer tests
electriclilies opened a new pull request #7438:
URL: https://github.com/apache/tvm/pull/7438
In the onnx test, we were setting target and ctx to llvm and cpu every time we ran the TVM version of the op, regardless of the target and ctx actually passed in. The result is that the accuracy of TVM GPU outputs were never checked against ONNX.
The purpose of this PR is to see which of the tests in tests/python/frontend/onnx/test_forward.py break after enabling GPU.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-809990924
Thanks @electriclilies @mbrookhart this is merged!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies edited a comment on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies edited a comment on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-804401474
@masahi I forgot to disable the dynamic version of the batch matmul test on cuda, it should be fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-804401474
@masahi I forgot to disable the dynamic version of batch matmul on cuda, it should be fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597177115
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
ok, surprised to hear that `use_gpu` doesn't do anything. Let's make it
```
PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS" run_pytest cython python-frontend-onnx tests/python/frontend/onnx
```
This should only modify env vars when running the onnx test.
Also, can you go through `TODO(mbrookhart)` in the onnx frontend test and add `use_gpus`? There are about 10-15 of them.
https://github.com/apache/tvm/blob/3beec22264f56f734de7d14cd6382b96e83e280a/tests/python/frontend/onnx/test_forward.py#L277
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r596555017
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Are we sure we want this? I think this is skipping a lot of tests that were previously running. The GPU frontend test only took 1h 16 min which seems too fast to me https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-7438/6/pipeline/390
It seems there are many tests that don't use `@tvm.testing.uses_gpu`. For example https://github.com/apache/tvm/blob/96b09817fc1796a789524ae30cd2d7e9d6f73d6c/tests/python/frontend/pytorch/test_object_detection.py#L94
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597194395
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
I can open a separate PR to remove these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r596555017
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Are we sure we want this? I think this is skipping a lot of tests that were previously running.
It seems there are many tests that don't use `@tvm.testing.uses_gpu`. https://github.com/apache/tvm/blob/96b09817fc1796a789524ae30cd2d7e9d6f73d6c/tests/python/frontend/pytorch/test_object_detection.py#L93-L94
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597072538
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Overall, I think that hardcoding targets is bad because it's not easy to see -- that's what caused none of the ONNX tests to be run on GPU in the first place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-777047550
Definitely a needed improvement, we'll see what it uncovers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r596556078
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Probably we should use `targets` option in `verify_with_ort_with_inputs` for now https://github.com/apache/tvm/blob/3beec22264f56f734de7d14cd6382b96e83e280a/tests/python/frontend/onnx/test_forward.py#L126
See for example https://github.com/apache/tvm/blob/3beec22264f56f734de7d14cd6382b96e83e280a/tests/python/frontend/onnx/test_forward.py#L3421-L3423 (we should test this function on GPU, btw)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-809957791
@masahi It's green! Can you merge? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597177115
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
ok, surprised to hear that `use_gpu` doesn't do anything. Let's make it
```
PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS" run_pytest cython python-frontend-onnx tests/python/frontend/onnx
```
This should only modify env vars when running the onnx test.
Also, can you go through `TODO(mbrookhart)` and add `use_gpus`? There are about 10-15 of them.
https://github.com/apache/tvm/blob/3beec22264f56f734de7d14cd6382b96e83e280a/tests/python/frontend/onnx/test_forward.py#L277
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597194246
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Doh! I thought I had removed all of those comments, but I must have only done it for the dynamic test files and missed the onnx file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #7438:
URL: https://github.com/apache/tvm/pull/7438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597177115
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
ok, surprised to hear that `use_gpu` doesn't do anything. Let's make it
```
PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS" run_pytest cython python-frontend-onnx tests/python/frontend/onnx
```
This should only modify env vars when running the onnx test.
Also, can you go through `TODO(mbrookhart)` in the onnx frontend test and add `use_gpus`? There are about 10-15 of them. I'm assuming that
https://github.com/apache/tvm/blob/3beec22264f56f734de7d14cd6382b96e83e280a/tests/python/frontend/onnx/test_forward.py#L277
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
ok, surprised to hear that `use_gpu` doesn't do anything. Let's make it
```
PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS" run_pytest cython python-frontend-onnx tests/python/frontend/onnx
```
This should only modify env vars when running the onnx test.
Also, can you go through `TODO(mbrookhart)` in the onnx frontend test and add `use_gpus`? There are about 10-15 of them.
https://github.com/apache/tvm/blob/3beec22264f56f734de7d14cd6382b96e83e280a/tests/python/frontend/onnx/test_forward.py#L277
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-808604311
@electriclilies Please try kick again, the flaky issue should be fixed now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r596555017
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Are we sure we want this? I think this is skipping a lot of tests that were previously running. The GPU frontend test only took 1h 16 min which seems too fast to me https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-7438/6/pipeline/390
It seems there are many tests that don't use `@tvm.testing.uses_gpu`. https://github.com/apache/tvm/blob/96b09817fc1796a789524ae30cd2d7e9d6f73d6c/tests/python/frontend/pytorch/test_object_detection.py#L93-L94
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597212799
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
I already did it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-777371795
oops indeed there are some problems with gpu test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597071262
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Right now, all the tests are running on GPU, which causes the argmin / argmax onnx test to fail. The ONNX test file uses @tvm.testing.uses_gpu to indicate whether we should run tests on GPU or not, but right now those decorators don't actually do anything, which is definitely not good. I think we should try to move towards enabling the @tvm.testing.uses_gpu in the long run.
What if I set PYTEST_ADDOPTS before the onnx tests, and reset it after? Since the file uses the @tvm.testing.uses_gpu a lot, I think we should either remove the decorators or let them actually do something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#discussion_r597071262
##########
File path: tests/scripts/task_python_frontend.sh
##########
@@ -31,6 +31,9 @@ find . -type f -path "*.pyc" | xargs rm -f
# Rebuild cython
make cython3
+# Only run GPU enabled tests on GPU
+export PYTEST_ADDOPTS="-m gpu $PYTEST_ADDOPTS"
Review comment:
Right now, all the tests are running on GPU, which causes the argmin / argmax onnx test to fail. The ONNX test file uses @tvm.testing.uses_gpu to indicate whether we should run tests on GPU or not, but right now it doesn't do anything, which is definitely not good. I think we should try to move towards enabling the @tvm.testing.uses_gpu in the long run.
What if I set PYTEST_ADDOPTS before the onnx tests, and reset it after? Since the file uses the @tvm.testing.uses_gpu a lot, I think we should either remove the decorators or let them actually do something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7438: [ONNX] Enable GPU in ONNX importer tests
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7438:
URL: https://github.com/apache/tvm/pull/7438#issuecomment-804393643
@electriclilies Can you take a look at the CI issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org