Add `rapids_test` allowing projects to run gpu tests in parallel #328

robertmaynard · 2022-12-12T16:55:54Z

Description

Introduces rapids_test functionality to allow tests executed via ctest -j to properly resource share GPUs.

This is done by having tests state how many GPUs allocations they require, and uses CTest internal job scheduler to properly load balance.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.
The cmake-format.json is up to date with these changes.
I have added new files under rapids-cmake/
- I have added include guards (include_guard(GLOBAL))
- I have added the associated docs/ rst file and update the api.rst

bdice

This is a cool feature. I am guessing we want this to parallelize RAPIDS tests in CI? One concern is that percentages may not be the most useful metric. Some tests might allocate 8 GB of memory regardless of the GPU size, meaning that the percentage is different on a 32 GB GPU than a 48 GB GPU.

docs/cpp_code_snippets/rapids_cmake_ctest_allocation.cpp

docs/cpp_code_snippets/rapids_cmake_ctest_allocation.hpp

testing/test/generate_resource_spec-simple.cmake

testing/test/init-activate_allocation_simple/main.cu

testing/test/init-activate_multi_allocations_same_gpu/CMakeLists.txt

testing/test/init-activate_multi_allocations_same_gpu/main.cu

testing/test/init-activate_multi_jobs_single_gpu/main.cpp

robertmaynard · 2022-12-12T19:55:05Z

I am guessing we want this to parallelize RAPIDS tests in CI?

Yes, and other NVIDIA projects. This first step should allow some projects like Thrust to have correct parallel support now.
But we are missing a component for RAPIDS ( see below ) CI, but this will alllow local developers using ctest.

detect_number_of_gpus.cmake why not hard errors?

So this comes back to the missing part needed for RAPIDS. There is no hard requirement when configuring and building RAPIDS projects that you have an actual GPU on the machine. Therefore we shouldn't expect that the test harness code to start hard erroring when it executes in these enviornments. RAPIDS CI runs the configure && build steps without any GPU devices so we want to gracefully fallback to presuming 1 GPU in those cases.

This comes back to the missing component. What rapids-cmake needs to offer is some executable / process for CI structures like RAPIDS to re-detect the number of GPUs on the machine before ctest is run. This way CI can build the binaries on machine A and ship to B and properly use the hardware.

I am currently trying to figure out an approach for this, but didn't want to hold up the initial PR

robertmaynard · 2022-12-12T20:16:29Z

Some tests might allocate 8 GB of memory regardless of the GPU size, meaning that the percentage is different on a 32 GB GPU than a 48 GB GPU.

I wanted to keep the usage as simple as possible. I am worried that we use HIGH_WATER_MEMORY instead of PERCENT it will make people think they need to be super accurate.

Another issue with using MEMORY is how we describe tests that use all GPU resources. We can't say each test requires 1TB of memory as that request can't be fulfilled. Likewise we can't use a magic placeholder like ALL since the GPUs that exist at CMake configure time might not match the GPUs that exist when we run the tests / update tool.

rapids-cmake/test/install_relocatable.cmake

bdice

@robertmaynard I started a review a while ago (probably a month ago) and didn't click submit. I am submitting those comments now -- forgive me if the comments are outdated.

README.md

cmake-format-rapids-cmake.json

docs/api.rst

docs/cpp_code_snippets/rapids_cmake_ctest_allocation.hpp

rapids-cmake/test/detail/generate_resource_spec.cpp

rapids-cmake/test/detail/run_gpu_test.cmake

rapids-cmake/test/generate_resource_spec.cmake

bdice · 2023-01-04T05:15:12Z

rapids-cmake/test/gpu_requirements.cmake

+  endif()
+
+  # verify that percent is inside the allowed bounds
+  if(percent GREATER 100 OR percent LESS 1 OR (NOT percent MATCHES "^[0-9]+$"))


Sounds good. I'm fine with whatever way you want to resolve this.

testing/test/add-single-job-multi-gpu/main.cu

ajschmidt8

Approving ops-codeowner file changes

bdice

This PR is pretty hefty. I skimmed it again (most code was familiar) and had only a few minor suggestions. I'll approve this and you can handle my suggestions as you wish.

README.md

rapids-cmake/test/install_relocatable.cmake

.clang-format

Introduces rapids_test functionality to allow tests executed via ctest -j to properly resource share GPUs. This is done by having tests state how many GPUs allocations they require, and uses CTest internal job scheduler to properly load balance.

robertmaynard · 2023-02-27T14:42:20Z

Squashed everything down to a single commit and removed all the unneeded changes thanks to #378 being merged.

vyasr

This is really cool!

I have a general question about properties that came up a few times in this review: can you set_property with any arbitrary property string, even if it doesn't already exist? Do you not need to define_property first in those cases?

I haven't reviewed the tests yet, will do that in a subsequent pass.

README.md

dependencies.yaml

rapids-cmake/rapids-test.cmake

rapids-cmake/test/detail/generate_resource_spec.cpp

rapids-cmake/test/install_relocatable.cmake

rapids-cmake/test/gpu_requirements.cmake

rapids-cmake/test/add.cmake

rapids-cmake/test/install_relocatable.cmake

This reverts commit 642abb3.

…_COUNT

vyasr

LGTM! It would be nice if we could somehow template the testing files with preprocessor macros or something since there's so much duplication (50% of this PR seems like it's just the license headers) but other than that everything looks good.

rapids-cmake/test/add.cmake

Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>

robertmaynard · 2023-03-07T15:30:58Z

/merge

robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change labels Dec 12, 2022

robertmaynard requested a review from a team as a code owner December 12, 2022 16:55

bdice requested changes Dec 12, 2022

View reviewed changes

robertmaynard force-pushed the fea/rapids-cmake-test branch 4 times, most recently from d65f91c to aa41d6e Compare December 21, 2022 16:40

robertmaynard mentioned this pull request Dec 21, 2022

Use rapids-cmake parallel testing feature rapidsai/rmm#1183

Merged

3 tasks

robertmaynard commented Dec 27, 2022

View reviewed changes

rapids-cmake/test/install_relocatable.cmake Outdated Show resolved Hide resolved

robertmaynard force-pushed the fea/rapids-cmake-test branch from b22e3d1 to 1091830 Compare December 30, 2022 15:19

robertmaynard requested a review from bdice January 3, 2023 16:12

robertmaynard force-pushed the fea/rapids-cmake-test branch from a75c984 to a892495 Compare January 3, 2023 21:50

robertmaynard changed the title ~~Fea/rapids cmake test~~ Add rapids_test allowing projects to run gpu tests in parallel Jan 10, 2023

robertmaynard changed the base branch from branch-23.02 to branch-23.04 January 24, 2023 16:02

bdice reviewed Feb 16, 2023

View reviewed changes

robertmaynard requested a review from a team as a code owner February 21, 2023 19:53

ajschmidt8 approved these changes Feb 21, 2023

View reviewed changes

robertmaynard force-pushed the fea/rapids-cmake-test branch from 82997a9 to 7961f8c Compare February 21, 2023 20:42

robertmaynard requested a review from bdice February 21, 2023 20:50

bdice approved these changes Feb 23, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

rapids-cmake/test/install_relocatable.cmake Outdated Show resolved Hide resolved

rapids-cmake/test/install_relocatable.cmake Outdated Show resolved Hide resolved

.clang-format Outdated Show resolved Hide resolved

bdice mentioned this pull request Feb 23, 2023

Introduce clang-format to rapids-cmake to format C++ code examples #378

Merged

2 tasks

robertmaynard force-pushed the fea/rapids-cmake-test branch from 611f4c0 to ff710b8 Compare February 27, 2023 14:41

Provide some rapids_test_install_relocatable docs

4768e3a

vyasr requested changes Feb 27, 2023

View reviewed changes

Properly propagate runtime exceptions as failure

a6304ee

robertmaynard added 10 commits February 28, 2023 11:25

Remove changes to dependencies.yaml

642abb3

Revert "Remove changes to dependencies.yaml"

d208854

This reverts commit 642abb3.

Updated readme as requested in code review

50fee45

rapids-cmake/rapids-test.cmake includes generate_resource_spec

2781c11

More style changes found during code review

52dca15

remove unneeded extraction of percent value from CTEST_RESOURCE_GROUP…

8c79f17

…_COUNT

remove unneeded spaces around brackets

cfaffb3

Correct ci test failures

cbf8d90

refactor generated file names to a common location

b1fe07d

simplify generate_resource_spec.cpp

d317e42

vyasr approved these changes Mar 6, 2023

View reviewed changes

rapids-cmake/test/add.cmake Outdated Show resolved Hide resolved

Update rapids-cmake/test/add.cmake

e6a8403

Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>

robertmaynard force-pushed the fea/rapids-cmake-test branch from e53dcc0 to e6a8403 Compare March 7, 2023 15:19

Merge branch 'branch-23.04' into fea/rapids-cmake-test

8cf2eb0

rapids-bot bot merged commit f7876e6 into rapidsai:branch-23.04 Mar 7, 2023

robertmaynard deleted the fea/rapids-cmake-test branch March 7, 2023 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rapids_test` allowing projects to run gpu tests in parallel #328

Add `rapids_test` allowing projects to run gpu tests in parallel #328

robertmaynard commented Dec 12, 2022 •

edited

Loading

bdice left a comment

robertmaynard commented Dec 12, 2022 •

edited

Loading

robertmaynard commented Dec 12, 2022

bdice left a comment

bdice Jan 4, 2023

ajschmidt8 left a comment

bdice left a comment

robertmaynard commented Feb 27, 2023

vyasr left a comment

vyasr left a comment

robertmaynard commented Mar 7, 2023

Add rapids_test allowing projects to run gpu tests in parallel #328

Add rapids_test allowing projects to run gpu tests in parallel #328

Conversation

robertmaynard commented Dec 12, 2022 • edited Loading

Description

Checklist

bdice left a comment

Choose a reason for hiding this comment

robertmaynard commented Dec 12, 2022 • edited Loading

robertmaynard commented Dec 12, 2022

bdice left a comment

Choose a reason for hiding this comment

bdice Jan 4, 2023

Choose a reason for hiding this comment

ajschmidt8 left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

robertmaynard commented Feb 27, 2023

vyasr left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

robertmaynard commented Mar 7, 2023

Add `rapids_test` allowing projects to run gpu tests in parallel #328

Add `rapids_test` allowing projects to run gpu tests in parallel #328

robertmaynard commented Dec 12, 2022 •

edited

Loading

robertmaynard commented Dec 12, 2022 •

edited

Loading