Neighborhood sampling C API implementation #2156

ChuckHastings · 2022-03-23T21:25:01Z

Provide the implementation for the C API for neighborhood sampling.

Currently needs some debugging.

merge after 2150

seunghwak · 2022-03-28T17:41:12Z

cpp/src/detail/shuffle_wrappers.cu

+                                    rmm::device_uvector<vertex_t>& d_vertices,
+                                    rmm::device_uvector<value_t>& d_values,
+                                    rmm::device_uvector<value_t>& d_local_values,
+                                    vertex_t local_vertex_first)


This looks same to the code added in PR #2150 and the same comment applies here.

seunghwak · 2022-03-28T17:45:19Z

cpp/src/c_api/array.hpp

-  std::unique_ptr<std::byte[]> data_;
-  size_t size_;
-  size_t num_bytes_;
+  // NOTE: size must be first here because the device buffer is released


I don't understand this comment, and could you elaborate this comment? It seems like there is more context to this.

Comment was copied from above. In the type_erased_device_buffer we do a release from the rmm::device_uvector which changes the size of the vector to 0. It is important in that case that we copy the size before we call release.

Should delete this comment, std::vector doesn't support release, we do a copy instead.

Gotcha, I can see that from the code of cugraph_type_erased_device_array_t. No idea why this is necessary (because it is not...)for cugraph_type_erased_host_array_t reading the code. Better delete the comment to avoid confusion.

seunghwak · 2022-03-28T17:46:54Z

cpp/src/c_api/hits.cpp

+                                                            do_expensive_check_);
+
+        cugraph::detail::collect_vertex_values_to_local<vertex_t, weight_t, multi_gpu>(
+          handle_, guess_vertices, guess_values, hubs, graph_view.get_local_vertex_first());


Is this PR pulling updates from PR #2150??? If that's the case, this PR should better be reviewed after PR #2150 gets merged (to avoid reviewing the same code twice).

Yes. I mentioned this in the slack thread.

…on tests

…pling_capi_implementation

…mentation

… when results don't partition evenly, add adjacenct_difference, since shuffle wants counts not offsets

…mentation

kaatish · 2022-03-29T14:17:51Z

cpp/src/sampling/nbr_sampling_impl.cuh

@@ -332,8 +335,12 @@ uniform_nbr_sample_impl(

  if constexpr (graph_view_t::is_multi_gpu) {
    size_t num_starting_vs = d_in.size();
+#if 0


Is this meant to be removed?

Yes. I will remove it.

seunghwak · 2022-03-29T16:42:40Z

cpp/src/sampling/nbr_sampling_impl.cuh

-
-        // resize accumulators:
+      // FIXME: Need to call collective operations from all threads
+      // if (in_sz > 0) {


So, is this a remaining bug to be fixed later?

Nope. I will delete the FIXME, I fixed that problem in order to get the unit tests to pass.

seunghwak · 2022-03-29T16:43:23Z

cpp/src/sampling/nbr_sampling_impl.cuh

+                           d_in,
+                           d_ranks,
+                           gpu_t{});
+      //}


Better delete?

I will clean up some of the commented out bits in the code. Part of my work to get things running.

seunghwak · 2022-03-29T16:43:34Z

cpp/src/sampling/nbr_sampling_impl.cuh

+                           d_ranks,
+                           gpu_t{});
+      //}
+
      //}


Better delete?

Cleaned up in next push

seunghwak · 2022-03-29T16:44:18Z

cpp/tests/c_api/mg_uniform_neighbor_sample_test.c

+  C_CUDA_TRY(cudaGetDeviceCount(&num_gpus_per_node));
+  C_CUDA_TRY(cudaSetDevice(comm_rank % num_gpus_per_node));
+
+#if 0


Should we keep this?

Deleted in next push

seunghwak · 2022-03-29T16:44:32Z

cpp/tests/c_api/uniform_neighbor_sample_test.c

+              ret_code != CUGRAPH_SUCCESS,
+              "cugraph_uniform_neighbor_sample expected to fail in SG test");
+
+#if 0


Should we keep this?

I'm hoping to add support for SG in the next release. This should be the code to do the test once SG is working (although it will need to be debugged). I can delete it and recreate it from the MG version next release if we want to keep this clean, although I'm inclined to keep it for now.

Sure, no problem.

codecov-commenter · 2022-03-29T19:27:22Z

Codecov Report

Merging #2156 (bf99d28) into branch-22.04 (87be0b3) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head bf99d28 differs from pull request most recent head 27c7895. Consider uploading reports for the commit 27c7895 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.04    #2156      +/-   ##
================================================
+ Coverage         73.95%   73.99%   +0.04%     
================================================
  Files               157      157              
  Lines             10496    10496              
================================================
+ Hits               7762     7767       +5     
+ Misses             2734     2729       -5

Impacted Files	Coverage Δ
.../cugraph/tests/test_edge_betweenness_centrality.py	`84.93% <0.00%> (+0.60%)`	⬆️
...graph/cugraph/tests/test_betweenness_centrality.py	`83.01% <0.00%> (+0.62%)`	⬆️
python/cugraph/cugraph/tests/test_graph_store.py	`100.00% <0.00%> (+1.56%)`	⬆️
python/cugraph/cugraph/tests/test_utils.py	`75.55% <0.00%> (+4.44%)`	⬆️

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 87be0b3...27c7895. Read the comment docs.

BradReesWork · 2022-03-29T20:48:25Z

@gpucibot merge

Closes #2108 when merged. Requires both #2088 and #2156 to be merged before, the former because this uses MGGraph, and the later because of the C implementation of neighborhood sampling. Authors: - https://github.com/betochimas - Joseph Nke (https://github.com/jnke2016) - Rick Ratzel (https://github.com/rlratzel) Approvers: - Don Acosta (https://github.com/acostadon) - Rick Ratzel (https://github.com/rlratzel) - Joseph Nke (https://github.com/jnke2016) - Chuck Hastings (https://github.com/ChuckHastings) - Jordan Jacobelli (https://github.com/Ethyling) URL: #2118

ChuckHastings added 3 commits March 23, 2022 10:53

add hits implementation

7217cfa

fix clang-format issues

38d63f8

first cut at NBR sampling C API implementation

315798d

ChuckHastings requested review from a team as code owners March 23, 2022 21:25

ChuckHastings self-assigned this Mar 23, 2022

ChuckHastings added 2 - In Progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 23, 2022

ChuckHastings added this to the 22.04 milestone Mar 23, 2022

BradReesWork requested a review from seunghwak March 24, 2022 01:01

ChuckHastings added 2 commits March 24, 2022 12:45

Merge branch 'branch-22.04' into fea_hits_capi_implementation

cf1b6f8

update PR based on comments

c09087b

BradReesWork requested a review from kaatish March 28, 2022 13:30

BradReesWork added 3 - Ready for Review and removed 2 - In Progress labels Mar 28, 2022

seunghwak reviewed Mar 28, 2022

View reviewed changes

ChuckHastings added 5 commits March 28, 2022 13:56

Updates based on PR comments. Added some unit tests for Joseph's pyth…

3abd918

…on tests

update copyright years

5824ceb

fix clang-format issues

11cf46b

Merge branch 'fea_hits_capi_implementation' into fea_neighborhood_sam…

70834ad

…pling_capi_implementation

Merge branch 'branch-22.04' into fea_neighborhood_sampling_capi_imple…

bcc6e9e

…mentation

BradReesWork added the 5 - Merge After Dependencies label Mar 28, 2022

ChuckHastings added 5 commits March 28, 2022 19:12

debugging, remove checks for empty lists, since empty lists can occur…

5227a0c

… when results don't partition evenly, add adjacenct_difference, since shuffle wants counts not offsets

more PR cleanup

233845b

Merge branch 'branch-22.04' into fea_neighborhood_sampling_capi_imple…

c169690

…mentation

last few known changes, ready for review

a71e813

fix clang-format issues

4e570fa

kaatish reviewed Mar 29, 2022

View reviewed changes

ChuckHastings removed the 5 - Merge After Dependencies label Mar 29, 2022

seunghwak reviewed Mar 29, 2022

View reviewed changes

code cleanup based on PR comments

27c7895

seunghwak approved these changes Mar 29, 2022

View reviewed changes

betochimas mentioned this pull request Mar 29, 2022

Add MG neighborhood sampling to pylibcugraph & cugraph APIs #2118

Merged

rapids-bot bot merged commit 62993ef into rapidsai:branch-22.04 Mar 29, 2022

ChuckHastings deleted the fea_neighborhood_sampling_capi_implementation branch March 31, 2022 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Neighborhood sampling C API implementation #2156

Neighborhood sampling C API implementation #2156

ChuckHastings commented Mar 23, 2022 •

edited by BradReesWork

Loading

seunghwak Mar 28, 2022

seunghwak Mar 28, 2022

ChuckHastings Mar 28, 2022

seunghwak Mar 28, 2022

seunghwak Mar 28, 2022

ChuckHastings Mar 28, 2022

kaatish Mar 29, 2022

ChuckHastings Mar 29, 2022

seunghwak Mar 29, 2022

ChuckHastings Mar 29, 2022

seunghwak Mar 29, 2022

ChuckHastings Mar 29, 2022

seunghwak Mar 29, 2022

ChuckHastings Mar 29, 2022

seunghwak Mar 29, 2022

ChuckHastings Mar 29, 2022

seunghwak Mar 29, 2022

ChuckHastings Mar 29, 2022

seunghwak Mar 29, 2022

codecov-commenter commented Mar 29, 2022 •

edited

Loading

BradReesWork commented Mar 29, 2022

Neighborhood sampling C API implementation #2156

Neighborhood sampling C API implementation #2156

Conversation

ChuckHastings commented Mar 23, 2022 • edited by BradReesWork Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 29, 2022 • edited Loading

Codecov Report

BradReesWork commented Mar 29, 2022

ChuckHastings commented Mar 23, 2022 •

edited by BradReesWork

Loading

codecov-commenter commented Mar 29, 2022 •

edited

Loading