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

Use rapids-cmake parallel testing feature #12451

Merged
merged 9 commits into from
Mar 22, 2023

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Dec 29, 2022

Description

Converts libcudf over to use rapids-cmake new GPU aware parallel testing feature, which allows tests to run across all the GPUs on a machine without oversubscription.

This will allow developers to run ctest -j<N> and ctest will figure out given the current machine how many tests it can run in parallel given the current GPU set ( currently 2 tests per GPU ).

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added ci CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Dec 29, 2022
@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.04@bf18cea). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 2536530 differs from pull request most recent head 97ff54b. Consider uploading reports for the commit 97ff54b to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-23.04   #12451   +/-   ##
===============================================
  Coverage                ?   85.67%           
===============================================
  Files                   ?      155           
  Lines                   ?    24810           
  Branches                ?        0           
===============================================
  Hits                    ?    21255           
  Misses                  ?     3555           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ajschmidt8
Copy link
Member

(updating the branch because it will help me test something for GHAs)

@robertmaynard robertmaynard changed the base branch from branch-23.02 to branch-23.04 January 24, 2023 16:03
@robertmaynard
Copy link
Contributor Author

Moving to 23.04

@robertmaynard robertmaynard force-pushed the fea/parallel_testing branch 3 times, most recently from 76cd036 to ad7e4a8 Compare February 28, 2023 19:15
@robertmaynard robertmaynard force-pushed the fea/parallel_testing branch from 23136b1 to fe34fd4 Compare March 8, 2023 15:34
@robertmaynard robertmaynard marked this pull request as ready for review March 8, 2023 15:34
@robertmaynard robertmaynard requested review from a team as code owners March 8, 2023 15:34
@robertmaynard robertmaynard added 3 - Ready for Review Ready for review by team and removed 5 - Merge After Dependencies labels Mar 8, 2023
@robertmaynard robertmaynard force-pushed the fea/parallel_testing branch 2 times, most recently from 66c7a0e to 8e34774 Compare March 8, 2023 21:31
@robertmaynard robertmaynard force-pushed the fea/parallel_testing branch 5 times, most recently from 8de0e2a to 66cde87 Compare March 10, 2023 20:54
@robertmaynard robertmaynard requested a review from a team as a code owner March 13, 2023 16:09
@robertmaynard robertmaynard force-pushed the fea/parallel_testing branch 4 times, most recently from 1d7cd79 to 2735e79 Compare March 13, 2023 20:09
rapids-bot bot pushed a commit that referenced this pull request Mar 14, 2023
This PR builds on #11875 and partially addresses #11943. This PR allows us to run all tests on a precise stream (the newly introduced `cudf::test::get_default_stream()`) and then verify that all CUDA APIs end up invoked on that stream. This implements the feature required in #11943, but to apply it universally across libcudf will require the API changes that will expose streams so I plan to make those changes incrementally after this PR is merged.

The preload library is now compiled twice, once to overload `cudf::get_default_stream` and once to overload `cudf::test::get_default_stream`. For now there is still some manual coordination associated with determining which one should be used with a given test, but once #12451 is merged and we start running all tests via ctest instead of direct invocation of the test executables we can start encoding this information in the CMake configuration of the tests by associating the require environment variables directly with the test executable using `set_tests_properties`.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Ray Douglass (https://github.com/raydouglass)
  - Nghia Truong (https://github.com/ttnghia)

URL: #12089
Converts libcudf over to use rapids-cmake new GPU aware parallel testing feature, which allows tests to run across all the GPUs on a machine without oversubscription.

This will allow developers to run ctest -j<N> and ctest will figure out given the current machine how many tests it can run in parallel given the current GPU set ( currently 4 tests per GPU ).
ci/test_cpp.sh Show resolved Hide resolved
ci/test_cpp.sh Outdated Show resolved Hide resolved
cpp/include/cudf_test/base_fixture.hpp Outdated Show resolved Hide resolved
cpp/include/cudf_test/base_fixture.hpp Show resolved Hide resolved
cpp/libcudf_kafka/tests/CMakeLists.txt Show resolved Hide resolved
fetch_rapids.cmake Outdated Show resolved Hide resolved
ci/test_cpp.sh Outdated Show resolved Hide resolved
ci/test_cpp.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM!

ci/test_cpp.sh Show resolved Hide resolved
@robertmaynard
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 253f2ab into rapidsai:branch-23.04 Mar 22, 2023
rapids-bot bot pushed a commit that referenced this pull request Jun 9, 2023
Instead of configuring test settings with environment variables in the bash script for CI, these changes allow all that configuration to happen directly in CMake, which is more robust and also makes the test configuration portable. With these changes any execution of the tests with `ctest` will use the preload library with all the other necessary environment variables configured as well. Running the test executables directly, on the other hand, will skip stream validation but still run correctly (except for the one test that explicitly tests the stream validation preload lib itself). These changes are now possible because our CI test scripts now run ctest instead of the executables (as of #12451) and due to full support for generator expression propagation even with the parallel testing feature (as of rapidsai/rapids-cmake#410).

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Mike Wilson (https://github.com/hyperbolic2346)

URL: #13513
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants