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

[OpenCL] Enable OpenCL for GPU tests #12490

Merged
merged 32 commits into from
Sep 9, 2022
Merged

Conversation

valmat07
Copy link
Contributor

Enable OpenCL support for GPU tests.

@valmat07
Copy link
Contributor Author

This PR enables OpenCL test in CI. For that:

  • Added USE_OPENCL flag in the build gpu script
  • Added adreno target in the CI target
  • Moved relay opencl texture tests in different location to support run them independently. This is necessary due to not all relay tests supports OpenCL CI configuration
  • Now opencl texture tests working in fp32.

@valmat07 valmat07 marked this pull request as ready for review August 30, 2022 10:29
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! Please, rebase it to the latest mainline. And one question. Will we run our cpp opencl tests in the CI?

CMakeLists.txt Outdated
@@ -798,4 +798,4 @@ if(${SUMMARIZE})
print_summary()
endif()

dump_options_to_file("${TVM_ALL_OPTIONS}")
dump_options_to_file("${TVM_ALL_OPTIONS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

No meaningful changes. Please revert it.

tests/cpp-runtime/opencl/run_gtests.cc Outdated Show resolved Hide resolved
tests/python/contrib/test_opencl/test_run_gtests.py Outdated Show resolved Hide resolved
tests/python/relay/opencl_texture/utils/adreno_utils.py Outdated Show resolved Hide resolved
@valmat07
Copy link
Contributor Author

valmat07 commented Sep 5, 2022

@echuraev I found better solutions to exclude tests, now they are excluded in CI scripts. Please check

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Several minor comments

tests/python/relay/opencl_texture/utils/adreno_utils.py Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@valmat07 valmat07 changed the title [OpenCL] Enable OpenCL for GPU tasks [OpenCL] Enable OpenCL for GPU tests Sep 6, 2022
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@echuraev
Copy link
Contributor

echuraev commented Sep 7, 2022

cc: @masahi, @csullivan, @TejashShah, @driazati


# forked is needed because the global registry gets contaminated
TVM_TEST_TARGETS="${TVM_RELAY_TEST_TARGETS:-llvm;cuda}" \
run_pytest ctypes ${TVM_INTEGRATION_TESTSUITE_NAME}-relay tests/python/relay

# OpenCL texture test. Deselected specific tests that fails in CI
TVM_TEST_TARGETS="${TVM_RELAY_OPENCL_TEXTURE_TARGETS:-opencl}" \
run_pytest ctypes ${TVM_INTEGRATION_TESTSUITE_NAME}-opencl-texture tests/python/relay/opencl_texture --deselect=tests/python/relay/opencl_texture/test_conv2d_nchw_texture.py::test_residual_block
Copy link
Member

Choose a reason for hiding this comment

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

Instead of deselecting here can you:

  1. File a GitHub issue with the test name and some context on why it fails with OpenCL
  2. Move the skipping to the test itself via pytest.mark.skipIf and be sure to add a reason="See <link to apache/tvm GitHub issue>"?

That might make it so you don't need to change this script at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the main thing here is that this test works well locally on adreno devices, so you don’t want to skip it. But in CI environment due to we have nvidia device so different libOpenCL, this test doesn't work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally we want to exclude some tests only for nvidia devices, but I couldn't find the proper way to exclude tests for nvidia. So maybe there is some way to exclude tests by making sure it doesn't run on nvidia?

Copy link
Member

Choose a reason for hiding this comment

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

You can do whatever logic is necessary in the condition for the skipIf so that it's only skipped in the specific cases you want (e.g. here maybe just skipIf(tvm.testing.utils.IS_IN_CI, reason="...")). Also in tvm/testing/utils.py take a look at requires_cuda or uses_gpu as they may already do what you need

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added skipif instead of deselect. Thanks for the suggestion!

@valmat07
Copy link
Contributor Author

valmat07 commented Sep 8, 2022

@tvm-bot rerun

# forked is needed because the global registry gets contaminated
TVM_TEST_TARGETS="${TVM_RELAY_TEST_TARGETS:-llvm;cuda}" \
run_pytest ctypes ${TVM_INTEGRATION_TESTSUITE_NAME}-relay tests/python/relay

# OpenCL texture test. Deselected specific tests that fails in CI
TVM_TEST_TARGETS="${TVM_RELAY_OPENCL_TEXTURE_TARGETS:-opencl}" \
run_pytest ctypes ${TVM_INTEGRATION_TESTSUITE_NAME}-opencl-texture tests/python/relay/opencl_texture
Copy link
Member

Choose a reason for hiding this comment

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

one more thing, won't this get run as part of test/python/relay in the run_pytest above this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a large number of tests in tests/python/realy/ for OpenCL do not run, for various reasons, so I took out the texture tests separately so as not to mark relay tests for some special occasions.

Copy link
Member

@driazati driazati left a comment

Choose a reason for hiding this comment

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

tests/ script changes look good to me

@TejashShah
Copy link

cc @masahi @junrushao, it needs a merge trigger!

@masahi masahi merged commit 574794e into apache:main Sep 9, 2022
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* Add opencl target in test build script

* Fix fp16 test and compile test for opencl

* fix lint

* Fix relay OpenCL texture tests

* Fix lint

* Enable relay OpenCL tests

* Fix opencl relay texture tests

* fix lint

* Remove OpenCL gtest variable

* Fix unbound variable

* Skip tests that are not supported in CI

* fix lint

* Add path for opencl gtest directory

* Fix opencl gtests include directory

* Enable OpenCL googletest. Fix bug in opencl timer test

* testing fix for build cpp tests

* update googletest git version for opencl tests build

* update cmakelist

* Update CMakeList

* Update CMakeList

* Disable opencl googletests

* update Opecnl.cmake

* fix Opecnl.cmake

* Apply comments. Remove xfail decerator for opencl tests. Now specific tests are skipped in the environment script

* minor code changes

* apply comments

* apply comment

* skip test in ci by decorator

* fix pytest skipif warnings

* Fix skipif for opencl gtests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants