Remove edge renumber map from the homogeneous sampling API #4775

jnke2016 · 2024-11-19T23:05:54Z

edge renumbering is only supported in heterogeneous neighborhood sampling hence trying to extract it from the PLC API leads to a segmentation fault

This PR :

fixes a bug in homogeneous neighborhood sampling (both uniform and bias).
properly handle vertex_type_offsets when performing heterogeneous renumbering
fixes a typo in the homogeneous neighborhood sampling docstrings

…long with offsets

seunghwak · 2024-11-22T01:02:57Z

cpp/src/c_api/neighbor_sampling.cpp

@@ -1011,6 +1012,8 @@ struct neighbor_sampling_functor : public cugraph::c_api::abstract_functor {
                                        options_.with_replacement_},
              do_expensive_check_);
        } else {
+          raft::print_device_vector(
+            "labels", (*start_vertex_labels).data(), (*start_vertex_labels).size(), std::cout);


Do you need to keep this? If this is just for debugging, better delete.

seunghwak · 2024-11-22T01:06:02Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+  if (result_srcs.size() == 0) {
+    // Update the 'edgelist_label_offsets' array to be proportional to the
+    // number of labels
+    result_offsets->resize(num_labels + 1, handle.get_stream());


Is there any guarantee that if results_srcs.size() != 0, result_offsets.size() == num_labels + 1? Say, we have 100 labels but only one label has a sampled edge, will this still work?

Say, we have 100 labels but only one label has a sampled edge, will this still work?

This is a valid edge case to consider. I believe the current implementation of neighborhood sampling only keeps track of vertices that have outgoing edges. @ChuckHastings any thought?

…2_remove-edge-renumber-map

seunghwak · 2024-11-27T02:44:16Z

cpp/src/sampling/neighbor_sampling_impl.hpp

@@ -105,6 +107,29 @@ neighbor_sample_impl(raft::handle_t const& handle,
  graph_view_t<vertex_t, edge_t, false, multi_gpu> modified_graph_view = graph_view;
  edge_masks_vector.reserve(num_edge_types);

+  label_t num_unique_labels = 0;
+
+  std::optional<rmm::device_uvector<label_t>> cp_starting_vertex_labels{std::nullopt};


So, it seems like you are not using this array after computing num_unique_labels. You can define this variable inside the if statement below.

seunghwak · 2024-11-27T02:48:52Z

cpp/src/sampling/neighbor_sampling_impl.hpp

-      if (edge_types) { (*level_result_edge_type_vectors).push_back(std::move(*edge_types)); }
-      if (labels) { (*level_result_label_vectors).push_back(std::move(*labels)); }
+      level_result_src.resize(
+        std::reduce(level_sizes_edge_types.begin(), level_sizes_edge_types.end()),


Have you tested this with more than one hops? You're defining level_sizes_edge_types outside the for (size_t hop = 0; hop < num_hops; ++hop) loop. In hop, this vector will includes the sizes from hop 0. Better minimize the scope of variables, and you may better define this inside the outermost loop.

And can't this just be

auto old_size = level_result_src.size(); <-old_size for copying. level_result_src.resizse(old_size + srcs.size())

? Should we really create level_sizes_edge_types?

No longer needed since I am now using old_size

seunghwak · 2024-11-27T02:51:09Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+        handle.get_stream());
+      level_result_dst.resize(
+        std::reduce(level_sizes_edge_types.begin(), level_sizes_edge_types.end()),
+        handle.get_stream());


level_result_dst.resize(level_result_src.size(), handle.get_stream());

using old_size + srcs.size()

cpp/src/sampling/neighbor_sampling_impl.hpp

seunghwak · 2024-11-27T02:54:56Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+                   std::reduce(level_sizes_edge_types.begin(), level_sizes_edge_types.end() - 1),
+                 dsts.begin(),
+                 srcs.size(),
+                 handle.get_stream());


Similar for all the copies and resizes here.

Also note that frequent resizes can be expensive (when # edge types is large). In this case, we may better create a large vector after iterating over the entire set of edge types.

Resize is basically create a new array and copying the current elements. This copy part can become quite costly if we are resizing many times.

Did the same for all copies

seunghwak · 2024-11-27T02:56:36Z

cpp/src/sampling/neighbor_sampling_impl.hpp

@@ -339,7 +457,7 @@ neighbor_sample_impl(raft::handle_t const& handle,
  if (return_hops) {
    result_hops   = rmm::device_uvector<int32_t>(result_size, handle.get_stream());
    output_offset = 0;
-    for (size_t i = 0; i < fan_out.size(); ++i) {
+    for (size_t i = 0; i < num_hops; ++i) {  // FIXME: replace this by the number of hops


Is this comment still relevant?

No longer relevant

seunghwak · 2024-11-27T03:03:51Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+    thrust::inclusive_scan(handle.get_thrust_policy(),
+                           result_offsets->begin() + 1,
+                           result_offsets->end(),
+                           result_offsets->begin() + 1);


I guess result_offsets[0] can hold a garbage value here.

seunghwak · 2024-11-27T03:09:11Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+                                                                 std::move(result_labels),
+                                                                 label_to_output_comm_rank);
+
+  if (result_labels && (result_offsets->size() != num_unique_labels + 1)) {


I am not sure the code in this if block is correct.

num_unique_labels is computed using starting_vertex_labels. starting vertex labels may include only a subset of the global labels.

Think about what will happen if unique labels are 0, 7, 13. Do you think this code will still work?

I am not sure I understand this edge case. Aren't we specifying the labels for only the starting vertices? And starting vertex labels offsets is being passed from the CAPI hence the labels are contiguous.

By global do you mean the multi-gpu case?

Think about the case label_to_output_comm_rank is defined.

In the multi-GPU case,

The unique labels in starting_vertex_labels and the labels mapped to this GPU (in label_to_output_comm_rank) may not coincide (I don't see any documentation about this).

What happens if the unique labels in starting_vertex_labels are 0, 7, 13 and the labels mapped to this GPU are 1, 4, 5?

Do you think this code will still work?

alexbarghi-nv

👍

seunghwak · 2024-12-02T21:53:33Z

cpp/src/sampling/neighbor_sampling_impl.hpp

@@ -105,6 +107,29 @@ neighbor_sample_impl(raft::handle_t const& handle,
  graph_view_t<vertex_t, edge_t, false, multi_gpu> modified_graph_view = graph_view;
  edge_masks_vector.reserve(num_edge_types);

+  label_t num_unique_labels = 0;
+
+  std::optional<rmm::device_uvector<label_t>> cp_starting_vertex_labels{std::nullopt};


We'd better reduce the scope of this variable.

You are using num_unique_labels or cp_starting_vertex_labels only after invoking shuffle_and_organize_output.

The input vertex labels are const and won't be modified in this function (even though you're updating starting_vertices to point different arrays).

No reason to pre-allocate memory here and also define this variable way before it is actually used. This goes against minimizing memory footprint & code readability.

seunghwak · 2024-12-02T21:57:39Z

cpp/src/sampling/neighbor_sampling_impl.hpp


      if (num_edge_types > 1) { modified_graph_view.clear_edge_mask(); }
    }

+    level_sizes.push_back(level_result_src.size());
+    level_result_src_vectors.push_back(std::move(level_result_src));
+    level_result_dst_vectors.push_back(std::move(level_result_dst));


After calling std::move, the variable is in valid but arbitrary state. You should not use re-use the moved-out variable assuming that it's size is reset to 0. And to minimize the scope of per-level (per-hop) variables, we should better define these variables inside the for (auto hop = 0; hop < num_hops; hop++) { ... }.

The input vertex labels are const and won't be modified in this function (even though you're updating starting_vertices to point different arrays).

As you mentioned, vertex labels are constant hence it can't be sorted and I need the initial labels downstream. I reduced the scope of its copy

seunghwak · 2024-12-02T21:59:08Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+                         std::move(result_edge_types),
+                         std::move(result_hops),
+                         std::move(result_labels),
+                         std::move(result_offsets));


result_offsets here stores result label offsets, right? Shouldn't we better rename to result_label_offsets?

Right. It is more descriptive

…2_remove-edge-renumber-map

seunghwak

LGTM except for minor cosmetic issues.

seunghwak · 2024-12-03T21:14:55Z

cpp/src/sampling/neighbor_sampling_impl.hpp

+  auto num_hops = ((fan_out.size() % num_edge_types) == 0)
+                    ? (fan_out.size() / num_edge_types)
+                    : ((fan_out.size() / num_edge_types) + 1);
+


We may better define this before defining level_result_src_vectors and level_result_dst_vectors. So, we can define all level_resutl_..._vectors in one code block.

seunghwak · 2024-12-03T21:19:20Z

cpp/src/sampling/neighbor_sampling_impl.hpp

@@ -252,11 +337,6 @@ neighbor_sample_impl(raft::handle_t const& handle,

    starting_vertices =
      raft::device_span<vertex_t const>(frontier_vertices.data(), frontier_vertices.size());


I assume we don't need this code anymore. We are directly creating a span from frontier_vertices.

Should I initialize frontier_vertices to starting_vertices?

ChuckHastings · 2024-12-03T23:21:38Z

/merge

jnke2016 added 2 commits November 19, 2024 14:57

homogeneous neighborhood sampling doesn't support edge renumber_map a…

95c5ba2

…long with offsets

update docstrings

af465f9

jnke2016 requested a review from a team as a code owner November 19, 2024 23:05

github-actions bot added the python label Nov 19, 2024

jnke2016 requested review from rlratzel and alexbarghi-nv November 19, 2024 23:06

jnke2016 changed the title ~~Branch 24.12 remove edge renumber map~~ Remove edge renumber map from the homogeneous sampling API Nov 19, 2024

alexbarghi-nv assigned jnke2016 Nov 20, 2024

fix bug in heterogeneous renumbering

61a0926

jnke2016 requested a review from a team as a code owner November 20, 2024 21:03

github-actions bot added the cuGraph label Nov 20, 2024

fix style

707cf3d

jnke2016 requested a review from ChuckHastings November 20, 2024 22:12

ChuckHastings approved these changes Nov 20, 2024

View reviewed changes

ChuckHastings added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 20, 2024

ChuckHastings added this to the 24.12 milestone Nov 20, 2024

rlratzel approved these changes Nov 21, 2024

View reviewed changes

jnke2016 added 3 commits November 21, 2024 16:48

handle case where there is no sampling result prior to renumbering

ef848e9

expose 'label_type_hop_offsets'

535df5d

fix style

3501a45

seunghwak reviewed Nov 22, 2024

View reviewed changes

jnke2016 added 8 commits November 21, 2024 20:36

remove debug print and properly compute the 'result_offsets'

288b8fc

update docstrings

9f459de

directly return the result

fc0426d

fix illegal memory access

d997656

Merge remote-tracking branch 'upstream/branch-24.12' into branch-24.1…

b7405e0

…2_remove-edge-renumber-map

rename variable for consistency

fc70811

fix style

463f12b

rename variable for consistency

3767b94

jnke2016 added 4 commits November 26, 2024 17:14

properly handle sampling with multiple edge types

b760a61

properly compute the number of hops for heterogeneous renumbering

580d3e9

fix style

529955a

Merge remote-tracking branch 'upstream/branch-24.12' into branch-24.1…

c3a58a0

…2_remove-edge-renumber-map

seunghwak reviewed Nov 27, 2024

View reviewed changes

jnke2016 added 4 commits November 26, 2024 19:21

update docstrings

bf17251

simplify code and re-order statements

88b35ba

fix style

f8c576a

update docstrings

9648d69

alexbarghi-nv approved these changes Nov 27, 2024

View reviewed changes

jnke2016 added 5 commits November 30, 2024 09:00

fix bug when creating struct

9d84558

fix style

a7a224c

add missing arguments

d64dc66

update label list if some are missing from the result

bcfc99c

fix style

19c37a8

alexbarghi-nv mentioned this pull request Dec 2, 2024

[FEA] Heterogeneous Distributed Sampling #4795

Open

seunghwak reviewed Dec 2, 2024

View reviewed changes

jnke2016 added 7 commits December 2, 2024 14:29

properly handle the case where hop is 0

3a0bf57

reduce variable score and reorder instructions

08a37cf

Merge remote-tracking branch 'upstream/branch-24.12' into branch-24.1…

6a61d9b

…2_remove-edge-renumber-map

remove unnecessary code

f23df9a

fix style

2797559

undo changes

1c70abc

remove unsued variables

6ca87be

seunghwak approved these changes Dec 3, 2024

View reviewed changes

jnke2016 added 3 commits December 3, 2024 14:35

undo changes

62b3b69

remove unsed variables

a737fbc

fix style

c434eae

rapids-bot bot merged commit 50a11b1 into rapidsai:branch-24.12 Dec 4, 2024
73 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove edge renumber map from the homogeneous sampling API #4775

Remove edge renumber map from the homogeneous sampling API #4775

jnke2016 commented Nov 19, 2024 •

edited

Loading

seunghwak Nov 22, 2024

seunghwak Nov 22, 2024

jnke2016 Nov 22, 2024

seunghwak Nov 27, 2024

seunghwak Nov 27, 2024

seunghwak Nov 27, 2024

jnke2016 Nov 27, 2024

seunghwak Nov 27, 2024

jnke2016 Nov 27, 2024

seunghwak Nov 27, 2024

seunghwak Nov 27, 2024

jnke2016 Nov 27, 2024

seunghwak Nov 27, 2024

jnke2016 Nov 27, 2024

seunghwak Nov 27, 2024

seunghwak Nov 27, 2024

jnke2016 Nov 27, 2024

jnke2016 Dec 2, 2024

seunghwak Dec 2, 2024

alexbarghi-nv left a comment

seunghwak Dec 2, 2024

seunghwak Dec 2, 2024

jnke2016 Dec 3, 2024

seunghwak Dec 2, 2024

jnke2016 Dec 3, 2024

seunghwak left a comment

seunghwak Dec 3, 2024

seunghwak Dec 3, 2024

jnke2016 Dec 3, 2024

ChuckHastings commented Dec 3, 2024

		@@ -252,11 +337,6 @@ neighbor_sample_impl(raft::handle_t const& handle,

		starting_vertices =
		raft::device_span<vertex_t const>(frontier_vertices.data(), frontier_vertices.size());

Remove edge renumber map from the homogeneous sampling API #4775

Remove edge renumber map from the homogeneous sampling API #4775

Conversation

jnke2016 commented Nov 19, 2024 • edited 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

alexbarghi-nv left a 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

seunghwak left a 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

ChuckHastings commented Dec 3, 2024

jnke2016 commented Nov 19, 2024 •

edited

Loading