Skip to content

make C++ tests run faster (fewer tests)#1989

Merged
rapids-bot[bot] merged 6 commits intorapidsai:branch-22.02from
ChuckHastings:fix_make_tests_faster
Jan 5, 2022
Merged

make C++ tests run faster (fewer tests)#1989
rapids-bot[bot] merged 6 commits intorapidsai:branch-22.02from
ChuckHastings:fix_make_tests_faster

Conversation

@ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Dec 10, 2021

Modified the most expensive C++ tests to run fewer tests. Closes #1555

Added an option (like in the rmat tests) to run a specific test file if the developer wants to manually run larger tests. For example:

tests/COARSEN_GRAPH_TEST --gtest_filter=file_ben* --test_file_name=test/datasets/ljournal-2008.mtx

The smaller graphs should be small enough to test things. Once we add C++ code coverage we should be able to verify this.

On my local workstation this reduced the time spent executing the C++ tests by about 25 minutes.

@ChuckHastings ChuckHastings requested a review from a team as a code owner December 10, 2021 22:18
@ChuckHastings ChuckHastings self-assigned this Dec 10, 2021
@ChuckHastings ChuckHastings added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 10, 2021
@ChuckHastings ChuckHastings modified the milestones: 21.12, 22.02 Dec 10, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.02@938e53f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.02    #1989   +/-   ##
===============================================
  Coverage                ?   70.50%           
===============================================
  Files                   ?      142           
  Lines                   ?     8880           
  Branches                ?        0           
===============================================
  Hits                    ?     6261           
  Misses                  ?     2619           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938e53f...eac62bd. Read the comment docs.

Copy link
Collaborator

@kaatish kaatish left a comment

Choose a reason for hiding this comment

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

lgtm

{
run_current_test<vertex_t, edge_t, weight_t>(std::get<0>(param), std::get<1>(param));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intention of creating this version of run_current_test? If we don't need to keep both, shouldn't we better remove one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new signature (std::tuple<...> instead of sending in the two input arguments) makes things a little cleaner.

I have only applied it to the more expensive tests as part of this PR. I can certainly combine the two run_current_test calls in these tests, it would be a little cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in latest push.

{
run_current_test<vertex_t, edge_t, weight_t, store_transposed>(std::get<0>(param),
std::get<1>(param));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned above, should we keep both versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in latest push.

}

CUDA_TRY(cudaStreamSynchronize(handle.get_stream()));
CUDA_TRY(cudaStreamSynchronize(handle.get_stream()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume now we should better use handle.sync_stream() with the recent raft update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will look at this. raft changes were not merged when I started working on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have a number of calls like this in the code base, I'm inclined to address this in a separate PR that picks up all of them.

@ChuckHastings
Copy link
Collaborator Author

rerun tests

Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

LGTM

@ChuckHastings
Copy link
Collaborator Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c49f049 into rapidsai:branch-22.02 Jan 5, 2022
@ChuckHastings ChuckHastings deleted the fix_make_tests_faster branch February 1, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA] Reduce the testing done by CI

4 participants