Use rapids-cmake testing to run tests in parallel #5487
Conversation
Serial test execution time: 232sec Parallel test execution 1 GPU: 128sec
robertmaynard
added
feature request
| echo "Running gtest $test_name" | ||
| ${gt} --gtest_output=xml:${RAPIDS_TESTS_DIR} | ||
| done | ||
| ctest -j9 --output-on-failure |
There was a problem hiding this comment.
How did you decide on 9? Is that based on running on the 16 core runner?
There was a problem hiding this comment.
I selected a value that was greater than the max concurrency based on the default value gpu percent value. Since currently each test takes 15% we can run 6 at a time, so I selected a number greater than that in-case we tune some tests to use less of a GPU.
There was a problem hiding this comment.
Changes look great! Sorry for the delayed review @robertmaynard, PR totally missed my radar for some reason until now.
| set(TEST_INSTALL_PATH bin/gtests/libcuml_prims) | ||
| else() | ||
| set(TEST_INSTALL_PATH bin/gtests/libcuml) | ||
| set(options CUMLPRIMS MPI ML_INCLUDE RAFT_DISTRIBUTED) |
There was a problem hiding this comment.
Could we get infer ML_INCLUDE from PREFIX == PRIMS? We already do that for the install component it seems, and that seems like the only case we don't want the ml includes.
| @@ -108,7 +108,7 @@ TYPED_TEST_CASE(WorkingSetTest, FloatTypes); | |||
| TYPED_TEST(WorkingSetTest, Init) | |||
| { | |||
| auto stream = this->handle.get_stream(); | |||
| this->ws = new WorkingSet<TypeParam>(this->handle, this->handle.get_stream(), 10); | |||
| this->ws = new WorkingSet<TypeParam>(this->handle, stream, 10); | |||
There was a problem hiding this comment.
Out of scope for this PR, but @dantegd this fixture is structured a bit oddly... there's really no need for it to be stateful, is there? The Init test could be done without accessing this at all (just create a WorkingSet on the stack) and the Select test could initialize everything in the WorkingSetTest constructor by itself since that's the only place that data is used. Am I missing something?
There was a problem hiding this comment.
yeah, I think you're correct and this could be simplified that way. As a matter of fact, there's a lot of tech debt in our C++ tests that we need to get to in a future refactor of things.
|
/merge |
C++ tests were not being run because the `test_cpp.sh` script was not executing `ctest` from the tests directory. This PR fixes that problem and adds the flag `--no-tests=error` to fail if no tests are found in the future. It seems this was broken in #5487. Also closes #5757 (includes the same changes). Authors: - Dante Gama Dessavre (https://github.com/dantegd) - Bradley Dice (https://github.com/bdice) - Ray Douglass (https://github.com/raydouglass) Approvers: - Ray Douglass (https://github.com/raydouglass)
By switching to rapids-cmake testing infrastructure we can both run all the tests in parallel from the build environment and the test/install env.
Performance improvements on a single Quadro RTX 8000 are:
Serial test execution time: 232sec
Parallel test execution 1 GPU: 128sec