-
Notifications
You must be signed in to change notification settings - Fork 311
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
make C++ tests run faster (fewer tests) #1989
make C++ tests run faster (fewer tests) #1989
Conversation
Codecov Report
@@ 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.
|
There was a problem hiding this 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)); | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)); | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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())); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@gpucibot merge |
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.