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

Benchmark tool for uniform neighbor sampling C API #3071

Closed

Conversation

ChuckHastings
Copy link
Collaborator

This PR contains a new benchmarking tool for timing the uniform neighbor sampling algorithm from the C API.

The code is a mix of C/C++ - to take advantage of the C++ test infrastructure that we have built.

This branch contains a hack (adding rt to the libraries to work around the problem corrected by #3049). Once PR 3049 is merged we can undo that hack and this will be ready to review/merge.

@ChuckHastings ChuckHastings requested review from a team as code owners December 9, 2022 17:37
@ChuckHastings ChuckHastings self-assigned this Dec 9, 2022
@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function DO NOT MERGE Hold off on merging; see PR for details non-breaking Non-breaking change labels Dec 9, 2022
@ChuckHastings ChuckHastings added this to the 23.02 milestone Dec 9, 2022
naimnv
naimnv previously approved these changes Dec 16, 2022
@@ -53,6 +53,7 @@ target_link_libraries(cugraphtestutil
PRIVATE
cuco::cuco
GTest::gtest
rt
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we should be able to remove this.

@@ -68,6 +69,7 @@ function(ConfigureTest CMAKE_TEST_NAME)
GTest::gtest
GTest::gtest_main
NCCL::NCCL
rt
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we should be able to remove this.

@@ -95,6 +97,7 @@ function(ConfigureTestMG CMAKE_TEST_NAME)
GTest::gtest_main
NCCL::NCCL
MPI::MPI_CXX
rt
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we should be able to remove this.

@@ -126,6 +129,7 @@ function(ConfigureCTest CMAKE_TEST_NAME)
cugraph_c_testutil
GTest::gtest
GTest::gtest_main
rt
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we should be able to remove this.

@@ -152,6 +156,7 @@ function(ConfigureCTestMG CMAKE_TEST_NAME)
GTest::gtest_main
NCCL::NCCL
MPI::MPI_CXX
rt
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we should be able to remove this.

@@ -412,6 +417,7 @@ if(BUILD_CUGRAPH_MG_TESTS)
MPI::MPI_CXX
PRIVATE
GTest::gtest
rt
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we should be able to remove this.

raft::handle_t* raft_handle = static_cast<raft::handle_t*>(create_raft_handle(prows));
handle = cugraph_create_resource_handle(raft_handle);

#ifdef AFTER_3601_MERGES
Copy link
Contributor

Choose a reason for hiding this comment

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

I think 3601 here should be 3061.


rmm::device_scalar<vertex_t> d_num_vertices(number_of_vertices, handle.get_stream());
raft::random::RngState rng_state(seed);

Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to consider when #ifdef NO_CUGRAPH_OPS is true.

And should this low level testing utility function be dependent on cugraph-ops? (and under the current cugraph-ops implementation, this will be served by 1 CUDA warp).

We may consider an alternative.

Say when count << num_vertices,

Generate count + alpha random indices and remove duplicates and take first count (generate additional random numbers if the resulting unique indices are fewer than count).

If count is in the same order of num_vertices, use thrust::shuffle to shuffle indices from 0 to num_vertices - 1 and take the first count indices.

number_of_vertices = vertex_ends.back();
int rank = handle.get_comms().get_rank();
if (rank > 0) base_index = vertex_ends[rank - 1];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What will be the behavior of this function if count is larger than number_of_vertices?

jnke2016
jnke2016 previously approved these changes Jan 13, 2023
@BradReesWork BradReesWork changed the base branch from branch-23.02 to branch-23.04 January 27, 2023 15:26
@BradReesWork BradReesWork dismissed stale reviews from jnke2016 and naimnv January 27, 2023 15:26

The base branch was changed.

@BradReesWork BradReesWork modified the milestones: 23.02, 23.04 Jan 27, 2023
@kingmesal kingmesal mentioned this pull request Feb 8, 2023
@ChuckHastings ChuckHastings removed this from the 23.04 milestone Mar 30, 2023
@BradReesWork BradReesWork changed the base branch from branch-23.04 to branch-23.06 March 30, 2023 16:39
@BradReesWork BradReesWork added this to the 23.06 milestone Apr 3, 2023
@ChuckHastings
Copy link
Collaborator Author

Not going to merge this. We don't have a real call for this capability - it was a temporary branch to do some performance testing. If we do have a need to do performance testing at the C API we should create a more comprehensive mechanism for this.

@ChuckHastings ChuckHastings deleted the c_api_uns_benchmark branch September 27, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Hold off on merging; see PR for details 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.

5 participants