Optimize check for vertex existence #4966

jnke2016 · 2025-03-11T13:56:55Z

This PR leverages the CAPI to optimize the check for vertex existence which can lead to +100x speedup compared to the cudf based version as it can be noticed from the performance figure below

closes #4956

rlratzel · 2025-03-11T18:53:10Z

@jnke2016 does this close #4956 by itself or are there other changes to the Python layer needed?

seunghwak

Is this PR ready for review?

I see just debug print statements and commented out code. If the PR is not ready for review, you may better create a draft PR (this also saves CI resources).

seunghwak · 2025-03-11T19:10:39Z

cpp/src/c_api/graph_functions.cpp

@@ -132,6 +132,7 @@ struct two_hop_neighbors_functor : public cugraph::c_api::abstract_functor {
            bool multi_gpu>
  void operator()()
  {
+    printf("\nin two_hop_neighbors \n");


Please delete debug print statements.

…number is False

…4_optimize-vertex-existance-check

seunghwak · 2025-03-17T16:12:44Z

cpp/src/c_api/graph_functions.cpp

+      rmm::device_uvector<vertex_t> vertex_array(1, handle_.get_stream());
+
+      cugraph::detail::sequence_fill(
+        handle_.get_stream(), vertex_array.data(), vertex_array.size(), vertex_t(vertex_));
+
+      if constexpr (multi_gpu) {
+        vertex_array = cugraph::shuffle_ext_vertices(handle_, std::move(vertex_array));
+      }
+
+      cugraph::renumber_ext_vertices<vertex_t, multi_gpu>(
+        handle_,
+        vertex_array.data(),
+        vertex_array.size(),
+        number_map->data(),
+        graph_view.local_vertex_partition_range_first(),
+        graph_view.local_vertex_partition_range_last(),
+        do_expensive_check_);


I assume the same vertex_t value is passed to every GPU in multi-GPU, right? Or are you assuming that each GPU can call this function with different vertex_ values?

If the former is the case, you don't need to really shuffle vertices, just need to call compute_gpu_id_from_ext_vertex_t to find the owning GPU. The owning GPU performs the check and broadcast the check result.

seunghwak · 2025-03-17T16:29:01Z

cpp/src/c_api/graph_sg.cpp

+        raft::copy<vertex_t>(
+          vertices.data(), edgelist_srcs.data(), edgelist_srcs.size(), handle_.get_stream());
+
+        cugraph::detail::sort_ints(handle_.get_stream(),
+                                   raft::device_span<vertex_t>{vertices.data(), vertices.size()});
+
+        size_t unique_vertices_size = cugraph::detail::unique_ints(
+          handle_.get_stream(), raft::device_span<vertex_t>{vertices.data(), vertices.size()});
+
+        vertices.resize(unique_vertices_size + edgelist_dsts.size(), handle_.get_stream());
+
+        raft::copy<vertex_t>(vertices.data() + unique_vertices_size,
+                             edgelist_dsts.data(),
+                             edgelist_dsts.size(),
+                             handle_.get_stream());
+
+        cugraph::detail::sort_ints(handle_.get_stream(),
+                                   raft::device_span<vertex_t>{vertices.data(), vertices.size()});
+
+        unique_vertices_size = cugraph::detail::unique_ints(
+          handle_.get_stream(), raft::device_span<vertex_t>{vertices.data(), vertices.size()});
+
+        vertices.resize(unique_vertices_size, handle_.get_stream());


To reduce memory footprint, you can sort & unique sources & destinations separately and merge instead of sort & unique sources, then copy all the destinations, and sort & unique.

You may further cut the memory footprint by first performing hash based group by (https://github.com/rapidsai/cugraph/blob/branch-25.04/cpp/include/cugraph/utilities/shuffle_comm.cuh#L761 with mem_frugal_threshold set), but this might be little bit of an overkill and you may defer this till this actually becomes a bottlenck (especially considering that renumber = false is here mainly for historical/debugging reasons).

If you just want to put working simple implementation here (as this won't be a case actual users will heavily use), just copy all sources & destinations to a single array and sort & unique.

And don't forget to call shrink_to_fit() to actually release memory after resize.

To reduce memory footprint, you can sort & unique sources & destinations separately and

Right. I now sort & unique sources & destinations separately. I also call shrink_to_fit.

You may further cut the memory footprint by first performing hash based group by ...

Right, we can add this optimization if this becomes a bottleneck which might not be for now. And as you mention, the case this code is targeting is for historical/debugging reasons

cpp/src/c_api/graph_sg.cpp

ChuckHastings · 2025-03-17T18:15:16Z

cpp/include/cugraph_c/graph_functions.h

+ */
+cugraph_error_code_t cugraph_has_vertex(const cugraph_resource_handle_t* handle,
+                                        cugraph_graph_t* graph,
+                                        const int vertex,


Can't use an int here. A vertex can be int32_t or int64_t.

For SSSP, we pass in a size_t and cast it to either int32_t or int64_t inside the C API implementation.

ChuckHastings · 2025-03-17T18:15:57Z

cpp/src/c_api/graph_functions.cpp

+struct has_vertex_functor : public cugraph::c_api::abstract_functor {
+  raft::handle_t const& handle_{};
+  cugraph::c_api::cugraph_graph_t* graph_{nullptr};
+  const int vertex_{};


Can't be an int.

ChuckHastings · 2025-03-17T18:16:22Z

cpp/src/c_api/graph_functions.cpp

+
+  has_vertex_functor(::cugraph_resource_handle_t const* handle,
+                     ::cugraph_graph_t* graph,
+                     const int vertex,


Can't be an int.

ChuckHastings · 2025-03-17T18:18:53Z

python/pylibcugraph/pylibcugraph/_cugraph_c/graph_functions.pxd

+    cdef cugraph_error_code_t cugraph_has_vertex(
+        const cugraph_resource_handle_t* handle,
+        const cugraph_graph_t* graph,
+        const int vertex,


Can't be an int

rlratzel

Looks good, thanks!

I just have a few minor requests.

rlratzel · 2025-03-20T05:49:46Z

python/pylibcugraph/pylibcugraph/bfs.pyx

@@ -145,6 +147,10 @@ def bfs(ResourceHandle handle, _GPUGraph graph,

    assert_CAI_type(sources, "sources")

+    # Check if sources are valid
+    for v in sources.values_host:
+        if not pylibcugraph.has_vertex(handle, graph, v, do_expensive_check):


If you just import has_vertex from pylibcugraph, you can change this:

Suggested change

if not pylibcugraph.has_vertex(handle, graph, v, do_expensive_check):

if not has_vertex(handle, graph, v, do_expensive_check):

rlratzel · 2025-03-20T05:50:58Z

python/pylibcugraph/pylibcugraph/bfs.pyx

@@ -20,6 +20,7 @@ from libc.stdint cimport uintptr_t
 from libc.stdint cimport int32_t
 from libc.limits cimport INT_MAX

+import pylibcugraph


You can remove this if you import has_vertex directly from pylibcugraph, which you're doing below.

rlratzel · 2025-03-20T05:54:58Z

python/pylibcugraph/pylibcugraph/has_vertex.pyx

+
+
+    return True if result else False


I would only have one blank line here.
Also, will this work instead?

Suggested change

return True if result else False

return bool(result)

rlratzel · 2025-03-20T05:58:22Z

python/pylibcugraph/pylibcugraph/has_vertex.pyx

+from pylibcugraph._cugraph_c.array cimport (
+    cugraph_type_erased_device_array_view_t,
+    cugraph_type_erased_device_array_view_free,
+)


I don't see these being used here, can they be removed?

rlratzel · 2025-03-20T05:59:15Z

python/pylibcugraph/pylibcugraph/has_vertex.pyx

+    copy_to_cupy_array,
+    create_cugraph_type_erased_device_array_view_from_py_obj


I don't see these being used here, can they be removed?

seunghwak · 2025-03-21T23:20:58Z

cpp/include/cugraph/detail/utility_wrappers.hpp

+/**
+ * @ingroup utility_wrappers_cpp
+ * @brief    Update a value in a device span to 0 if it matches the target_value or 1
+ *
+ * @tparam      value_t      type of the value to operate on. Must be either int32_t or int64_t.
+ *
+ * @param[out]  values       device span to update
+ * @param[in]   target_value        value to be querried
+ * @param[in]   stream_view  stream view
+ *
+ */
+template <typename value_t>
+void transform_binary(raft::device_span<value_t> values,
+                      value_t target_value,
+                      raft::handle_t const& handle);


This doesn't look generic enough to be included as a utility function.

I think we at least need a better name for this if you need this to call thrust function from .cpp file.

Better store/return results in bool array than using value_t.

Something like

rmm::device_uvector<bool> elementwise_not_equal( raft::device_span<value_t> values, value_t compare, rmm::cuda_stream_view const& stream_view);

or transform_not_equal or transform_compare_not_equal? I won't sure about the best name but transform_binary doesn't match very well with what this function does.

Just FYI: I asked this to chatgpt and got this.

A good name for this function should align with STL naming conventions, which are often verb-based and descriptive of their operation. Some well-known STL functions that perform element-wise transformations or checks include std::transform, std::count_if, and std::any_of. Since your function checks whether each element differs from a given value and returns a boolean result for each, a name that follows STL style could be: Suggested names: mismatch_mask – Inspired by std::mismatch, emphasizing that it creates a mask of mismatches. not_equal_mask – Mirrors std::not_equal_to, clearly indicating the operation. transform_not_equal – Follows the std::transform pattern, emphasizing element-wise transformation. compare_not_equal – Explicit about the comparison operation. Recommended choice: not_equal_mask This name aligns well with STL conventions, is concise, and clearly communicates that the function generates a mask indicating inequality.

Interesting. I might go for chatgpt next time I am trying to find a name for my thrust-like function. I chose transform_not_equal

Yeah... you may try cursor chatting as well. Haven't compared but in theory, it can better consider the other code in cugraph codebase.

seunghwak · 2025-03-21T23:22:48Z

cpp/src/c_api/graph_functions.cpp

+      /*
+      if constexpr (multi_gpu) {
+        vertices = cugraph::shuffle_ext_vertices(handle_, std::move(vertices));
+      }
+      */


Delete dead code.

seunghwak · 2025-03-21T23:27:44Z

cpp/src/c_api/graph_functions.cpp

+      cugraph::detail::transform_binary(
+        raft::device_span<vertex_t>{vertices.data(), vertices.size()},
+        cugraph::invalid_vertex_id<vertex_t>::value,
+        handle_.get_stream());


I think the comparison result should better be stored in rmm::device_uvector<bool>

I created a rmm::device_uvector<bool> to store the result of the comparison.

seunghwak · 2025-03-22T01:43:56Z

cpp/src/c_api/graph_sg.cpp

+
+        vertices.resize(unique_edgelist_srcs_size + unique_edgelist_dsts_size,
+                        handle_.get_stream());
+        vertices.shrink_to_fit(handle_.get_stream());


This shrink_to_fit is unnecessary. You are not shrinking a vector here. You just increased the size of the vector.

Right this is addressed

cpp/src/c_api/graph_sg.cpp

seunghwak · 2025-03-22T01:54:25Z

cpp/src/c_api/graph_sg.cpp

+          return;
+        }
+
+        *number_map = std::move(vertices);


I guess this unnecessary. number_map already holds consecutive integers starting from 0.

seunghwak · 2025-03-24T16:21:36Z

cpp/include/cugraph/detail/utility_wrappers.hpp

+void transform_not_equal(raft::device_span<value_t> values,
+                         raft::device_span<bool> result,
+                         value_t compare,
+                         raft::handle_t const& handle);


Our convention is to pass handle as the first input argument (and stream as the last).

cpp/src/c_api/graph_sg.cpp

seunghwak

LGTM (except for unnecessary empty lines).

seunghwak · 2025-03-25T22:18:52Z

cpp/src/c_api/graph_sg.cpp

@@ -292,11 +294,34 @@ struct create_graph_functor : public cugraph::c_api::abstract_functor {
      if (renumber_) {
        *number_map = std::move(new_number_map.value());
      } else {
+


This empty line is unnecessary.

seunghwak · 2025-03-25T22:19:18Z

cpp/src/c_api/graph_sg.cpp

+
+

These empty lines are unnecessary.

seunghwak · 2025-03-25T22:19:55Z

cpp/src/c_api/graph_sg.cpp

+
+

No need for 3 empty lines. 1 is sufficient.

rlratzel

Recent changes look good but I have a question.

rlratzel · 2025-03-26T19:56:51Z

python/pylibcugraph/pylibcugraph/has_vertex.pyx

+    cdef cugraph_type_erased_device_array_view_t* \
+        result_view_ptr = \
+            cugraph_type_erased_device_array_view(
+                result_ptr)
+
+    cupy_has_vertex = copy_to_cupy_array(c_resource_handle_ptr, result_view_ptr)

-    return True if result else False
+    return cupy_has_vertex


Why was this copy operation added, and are we no longer returning a bool from this function as implied in the docstring? Also, do the benchmarks in the PR description include this change?

Talked to @jnke2016 offline: reason for returning the array is to facilitate passing/querying multiple vertices and getting individual bools back for each. This also helps facilitate MG use cases. The benchmarks were not impacted significantly with this change.
@jnke2016 is going to update the docstring and example now that the return value is different.

rlratzel · 2025-03-26T22:01:30Z

/merge

add method checking vertex existance

8ea7e35

jnke2016 requested a review from a team as a code owner March 11, 2025 13:56

github-actions bot added the cuGraph label Mar 11, 2025

rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 11, 2025

seunghwak reviewed Mar 11, 2025

View reviewed changes

jnke2016 added 7 commits March 11, 2025 19:27

Add check ensuring that the vertices are numbered consecutively if re…

83cb71f

…number is False

remove duplicated vertices

dff0729

add method checking for vertex existance

8b16e6d

add method to check vertex existence

49052d4

add method to check vertex existence in the PLC API

575e2e6

remove slow check

80a9255

add vertex check

32a1f0a

jnke2016 requested review from a team as code owners March 17, 2025 14:33

github-actions bot added CMake python labels Mar 17, 2025

jnke2016 added 5 commits March 17, 2025 07:49

remove debug print

10f5f2f

remove debug print

d5ed0ed

remove outdated script

253e0c9

Merge remote-tracking branch 'upstream/branch-25.04' into brnach-25.0…

d230cbc

…4_optimize-vertex-existance-check

fix style

9f9bf49

jnke2016 requested a review from ChuckHastings March 17, 2025 15:11

seunghwak reviewed Mar 17, 2025

View reviewed changes

ChuckHastings reviewed Mar 17, 2025

View reviewed changes

rlratzel requested changes Mar 20, 2025

View reviewed changes

jnke2016 added 2 commits March 20, 2025 17:16

update API to check the existance of a list of vertices

96135c5

fix style

050b4ac

jnke2016 added 9 commits March 21, 2025 07:15

update vertex check for BFS and SSSP

33681bd

update function API

09aeeb1

add method to retrieve vertex type from the graph

1da9244

fix style

e7dfadb

fix style

01f7ccf

update docstrings

737fb90

cut memory footprint

e821ffb

remove debug print

31495e2

fix style

06501be

seunghwak reviewed Mar 22, 2025

View reviewed changes

jnke2016 added 3 commits March 24, 2025 04:45

rename transform funtion, update docstring and API

15f0757

clean code

3c8d3be

fix style

896899f

seunghwak reviewed Mar 24, 2025

View reviewed changes

ChuckHastings assigned jnke2016 Mar 24, 2025

jnke2016 added 2 commits March 25, 2025 13:47

update check

35e0a9b

pass stream_view instead of handle

b4536fe

seunghwak approved these changes Mar 25, 2025

View reviewed changes

jnke2016 added 5 commits March 25, 2025 15:44

fix module error

9108ad6

fix style

8bcf086

fix style

9a65c65

store vertex type for MG graphs

d3f682c

fix style

fc57a8f

rlratzel added this to the 25.04 milestone Mar 26, 2025

ChuckHastings approved these changes Mar 26, 2025

View reviewed changes

rlratzel reviewed Mar 26, 2025

View reviewed changes

update docstrings and add docstring example

a7cf3de

rlratzel approved these changes Mar 26, 2025

View reviewed changes

rapids-bot bot merged commit 96e275f into rapidsai:branch-25.04 Mar 27, 2025
81 checks passed

	if not pylibcugraph.has_vertex(handle, graph, v, do_expensive_check):
	if not has_vertex(handle, graph, v, do_expensive_check):

		copy_to_cupy_array,
		create_cugraph_type_erased_device_array_view_from_py_obj

Optimize check for vertex existence #4966

Optimize check for vertex existence #4966

Uh oh!

Conversation

jnke2016 commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlratzel commented Mar 11, 2025

Uh oh!

seunghwak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlratzel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seunghwak Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seunghwak left a comment

Choose a reason for hiding this comment

jnke2016 commented Mar 11, 2025 •

edited

Loading

seunghwak Mar 22, 2025 •

edited

Loading