support heterogenous fanout type #4608

jnke2016 · 2024-08-13T15:54:13Z

closes #4589
closes #4591

ChuckHastings

Some thoughts on changing the API a bit.

ChuckHastings · 2024-08-13T19:19:42Z

cpp/include/cugraph/sampling_functions.hpp

  raft::random::RngState& rng_state,
  bool return_hops,
  bool with_replacement                           = true,
  prior_sources_behavior_t prior_sources_behavior = prior_sources_behavior_t::DEFAULT,
  bool dedupe_sources                             = false,
  bool do_expensive_check                         = false);

+#if 0
+/* FIXME:
+ There are two options to support heterogeneous fanout


Here's another option to explore.

Create a new function called neighbor_sample. Create it off of the biased sampling API, but with the following changes:

the biases become optional instead of required. Then it can do either uniform or biased in the same call just by whether the biases are included or not

the fanout and heterogeneous fanout as you have defined. Or we might explore using std::variant, where it would either take host_span or tuple of host span and make the right choice internally

Move the rng_state parameter to be right after the handle (before the graph_view). This feels like a better standard place for the parameter.

We can then mark the existing uniform_neighbor_sample and biased_neighbor_sample as deprecated. When we implement, the internal C++ implementation can just call the new neighbor_sample with the parameters properly configured. This makes it a non-breaking change (eventually we'll drop the old functions), but still keeps the code reuse increased.

Thoughts @seunghwak ?

the biases become optional instead of required. Then it can do either uniform or biased in the same call just by whether the biases are included or not

=> In this case, we may update the existing non-heterogeneous fanout type sampling functions as well. i.e. combine the uniform & biased sampling functions. Not sure about the optimal balancing point between creating too many functions vs creating a function with too many input parameters.

Yeah... I guess we should avoid creating a too busy function (one function handling all different types of sampling based on the input arguments excessively using std::variant & std::optional) but we should also avoid creating too many functions... Not sure what's the optimal balancing point...

In theory, adding new parameters exponentially increase code complexity (too handle all possible combinations of optional parameters), we should better create separate functions. If supporting an additional optional parameter requires only a minor change in the API and implementation, we may create one generic function (or we may create one complex function that handles all different options in the detail namespace and multiple public functions calling this if this helps in reducing code replication).

cpp/include/cugraph_c/sampling_algorithms.h

ChuckHastings · 2024-08-13T19:30:38Z

cpp/include/cugraph_c/sampling_algorithms.h

@@ -368,6 +410,7 @@ cugraph_error_code_t cugraph_uniform_neighbor_sample(
  const cugraph_type_erased_device_array_view_t* label_to_comm_rank,
  const cugraph_type_erased_device_array_view_t* label_offsets,
  const cugraph_type_erased_host_array_view_t* fan_out,
+  const cugraph_sample_heterogeneous_fanout_t* heterogeneous_fanout,


Perhaps we take the same approach here. Create a new C API function called neighbor_sample, following the biased function definition. Add this parameter. Deprecate the other functions. In the implementation we can just check for nullptr (NULL).

ChuckHastings · 2024-08-13T20:01:01Z

cpp/src/sampling/neighbor_sampling_impl.hpp

@@ -150,7 +173,7 @@ neighbor_sample_impl(

  std::vector<size_t> level_sizes{};
  int32_t hop{0};
-  for (auto&& k_level : fan_out) {
+  for (auto&& k_level : (*fan_out)) {


This isn't actually sufficient yet... but I'm more worried about the API right now.

This loop will need, in the case of heterogeneous sampling, to have 2 levels of for loop. An outer loop iterating by hop and an inner loop iterating by type.

I'd be inclined to add a setup loop that iterates over the types and generates the masks - and perhaps identifies the maximum number of hops to drive the outer loop. You'll need to get k_level from the right type/hop combination... so this for construct won't work at all, it will need to look different.

Right I only added it for it to compile. I will revisit this approach once we lock the API's interface. It is only supporting non heterogeneous type for now

ChuckHastings · 2024-08-13T20:01:23Z

cpp/src/sampling/neighbor_sampling_impl.hpp

@@ -192,7 +215,7 @@ neighbor_sample_impl(
    if (labels) { (*level_result_label_vectors).push_back(std::move(*labels)); }

    ++hop;
-    if (hop < fan_out.size()) {
+    if (hop < (*fan_out).size()) {


fan_out size will (potentially) vary by type.

Right I only added it for it to compile. I will revisit this approach once we lock the API's interface. It is only supporting non heterogeneous type for now

ChuckHastings · 2024-08-13T20:03:15Z

python/cugraph/cugraph/dask/sampling/uniform_neighbor_sample.py

+        # FIXME: Add expensive check to ensure all dict values are lists
+        # Convert to a tuple of sequence (edge type size and fanout values)
+        edge_type_size = []
+        [edge_type_size.append(len(s)) for s in list(fanout_vals.values())]


Does this iterate over the edge types in the dictionary in order? We need to make sure that this is constructed with edge type 0 first, followed by edge type 1, etc.

Right. I converted the heterogeneous fanout type to a sorted ordered dictionary.

ChuckHastings · 2024-08-13T20:03:56Z

python/cugraph/cugraph/dask/sampling/uniform_neighbor_sample.py

+        edge_type_size = []
+        [edge_type_size.append(len(s)) for s in list(fanout_vals.values())]
+        edge_type_fanout_vals = list(chain.from_iterable(list(fanout_vals.values())))
+        fanout_vals = (


Per my earlier suggestions, I think we want this to be a CSR structure, so converting from a list of sizes to a list of offsets is perhaps best done here.

We changed this back to a dense structure... so I think this code isn't right.

This still seems wrong to me. If you want to support fanout_vals as a dictionary I think we need to convert it to a dense array to get the right values. Do you have a python test for this path that we can verify?

ChuckHastings · 2024-08-13T20:04:22Z

python/cugraph/cugraph/sampling/uniform_neighbor_sample.py

@@ -314,8 +316,21 @@ def uniform_neighbor_sample(
        fanout_vals = fanout_vals.get().astype("int32")
    elif isinstance(fanout_vals, cudf.Series):
        fanout_vals = fanout_vals.values_host.astype("int32")
+    elif isinstance(fanout_vals, dict):


Same comments as above

…olidate neighborhood sampling functions

cpp/include/cugraph/sampling_functions.hpp

cpp/include/cugraph_c/sampling_algorithms.h

cpp/src/c_api/neighbor_sampling.cpp

…0_support_heterogeneous_fanout

alexbarghi-nv

This PR adds deprecated features, which I don't think we should be doing. Those should be omitted from the new APIs in PLC.

We also need to expose the edge renumber map to Python so we can recover the original edge ids, allowing us to select the correct values from the feature tensor.

python/pylibcugraph/pylibcugraph/heterogeneous_biased_neighbor_sample.pyx

alexbarghi-nv · 2024-11-14T23:22:41Z

python/pylibcugraph/pylibcugraph/heterogeneous_uniform_neighbor_sample.pyx

+                                          num_edge_types,
+                                          bool_t with_replacement,
+                                          bool_t do_expensive_check,
+                                          with_edge_properties=False,


Again, this was deprecated in uniform_neighbor_sample and I think we should just leave it out.

python/pylibcugraph/pylibcugraph/homogeneous_biased_neighbor_sample.pyx

alexbarghi-nv · 2024-11-14T23:23:54Z

python/pylibcugraph/pylibcugraph/homogeneous_uniform_neighbor_sample.pyx

+                                          h_fan_out,
+                                          bool_t with_replacement,
+                                          bool_t do_expensive_check,
+                                          with_edge_properties=False,


Same here; we don't need this flag and it is deprecated in uniform_neighbor_sample.

python/pylibcugraph/pylibcugraph/homogeneous_uniform_neighbor_sample.pyx

python/pylibcugraph/pylibcugraph/heterogeneous_biased_neighbor_sample.pyx

cpp/src/c_api/neighbor_sampling.cpp

…rray

…0_support_heterogeneous_fanout

alexbarghi-nv

Could we shorten the function names for getting the edge renumber map and offsets? I think it's self-explanatory that they have been renumbered and sorted.

cpp/include/cugraph_c/sampling_algorithms.h

alexbarghi-nv

These too

python/pylibcugraph/pylibcugraph/internal_types/sampling_result.pyx

alexbarghi-nv

👍

alexbarghi-nv · 2024-11-16T20:25:43Z

/merge

rlratzel

LGTM, but I did notice a couple minor typos in the new examples, but I'll approve now with the assumption those will be fixed.

rlratzel · 2024-11-18T15:40:08Z

python/pylibcugraph/pylibcugraph/homogeneous_biased_neighbor_sample.pyx

+    >>> sampling_results = pylibcugraph.homogeneous_biased_neighbor_sample(
+    ...         resource_handle, G, start_vertices, starting_vertex_label_offsets,
+    ...         h_fan_out, False, True)
+    >>> >>> sampling_results


Minor copy/paste error?

Suggested change

>>> >>> sampling_results

>>> sampling_results

rlratzel · 2024-11-18T15:40:31Z

python/pylibcugraph/pylibcugraph/homogeneous_uniform_neighbor_sample.pyx

+    >>> sampling_results = pylibcugraph.homogeneous_uniform_neighbor_sample(
+    ...         resource_handle, G, start_vertices, starting_vertex_label_offsets,
+    ...         h_fan_out, False, True)
+    >>> >>> sampling_results


closes rapidsai#4589 closes rapidsai#4591 Authors: - Joseph Nke (https://github.com/jnke2016) - Chuck Hastings (https://github.com/ChuckHastings) Approvers: - Seunghwa Kang (https://github.com/seunghwak) - Chuck Hastings (https://github.com/ChuckHastings) - Alex Barghi (https://github.com/alexbarghi-nv) - Rick Ratzel (https://github.com/rlratzel) URL: rapidsai#4608

support heterogenous fanout type

0adb2fd

github-actions bot added cuGraph python labels Aug 13, 2024

jnke2016 requested review from ChuckHastings and alexbarghi-nv August 13, 2024 17:51

jnke2016 added 2 commits August 13, 2024 11:01

remove unusued code

bb5a3e2

fix style

10fa86d

ChuckHastings reviewed Aug 13, 2024

View reviewed changes

jnke2016 added 3 commits August 20, 2024 04:15

create one API for both uniform and biased neighborhood sampling

f904350

use the same function for both uniform and biased nieghborhood sampling

1fc32c3

add support for heterogenous fanout support at the plc layer and cons…

8fc21f8

…olidate neighborhood sampling functions

github-actions bot added the CMake label Aug 20, 2024

jnke2016 added 11 commits August 20, 2024 09:06

remove outdated codes

01a57f3

add flag differentiating between biased and uniform sampling

3a6aeb2

update docstrings and rename variable

d2f6467

rename variable

5d25155

create new tuple type

80f8b86

remove unnecessary check

50e0fc5

add constructor converting from array_view_t to array_t

9f455bf

leverage new constructor and remove unnecessary code

d114534

ensure edge types are ordered in increasing order

cf4a3ae

update docstrings

bc87b50

update docstrings

3013684

ChuckHastings reviewed Aug 21, 2024

View reviewed changes

jnke2016 added 6 commits August 22, 2024 12:30

undo changes to uniform neighbor sample

d6b6234

undo changes to uniform neighbor sample

068b0a3

update docstrings

6920f65

re-order arguments

760c5cd

remove outdated comments

1e0ef27

add arguments and type check

de79620

jnke2016 added 11 commits November 12, 2024 09:17

update docstrings

3e03324

fix typo and remove check

6d50df8

reorder instructions

ed6d532

add docstring examples

8a3a774

fix style

57ba1e8

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

1011095

…0_support_heterogeneous_fanout

update docstrings

b827ac7

add more docstring example

27eb500

add type chec, remove outdated docstrings

c6ea067

fix style

db468b6

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

d866235

…0_support_heterogeneous_fanout

alexbarghi-nv requested changes Nov 14, 2024

View reviewed changes

jnke2016 added 8 commits November 14, 2024 18:55

add functions exposing the edge renumber map along with its offsets a…

978281e

…rray

add FIXME

cd68019

expose edge renumber map along with its offsets to the PLC API

ccfadc9

fix style

dc203dd

update docstrings example

69afe17

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

fa44832

…0_support_heterogeneous_fanout

remove outdated arguments

3d9b526

fix style

9edd3ae

alexbarghi-nv requested changes Nov 15, 2024

View reviewed changes

cpp/include/cugraph_c/sampling_algorithms.h Outdated Show resolved Hide resolved

cpp/include/cugraph_c/sampling_algorithms.h Outdated Show resolved Hide resolved

alexbarghi-nv requested changes Nov 15, 2024

View reviewed changes

python/pylibcugraph/pylibcugraph/internal_types/sampling_result.pyx Outdated Show resolved Hide resolved

python/pylibcugraph/pylibcugraph/internal_types/sampling_result.pyx Outdated Show resolved Hide resolved

alexbarghi-nv mentioned this pull request Nov 15, 2024

Support Heterogeneous Renumbering in the C/PLC APIs #4764

Closed

jnke2016 added 2 commits November 15, 2024 08:37

rename methods

c8b5875

fix style

65a0225

alexbarghi-nv approved these changes Nov 15, 2024

View reviewed changes

rlratzel approved these changes Nov 18, 2024

View reviewed changes

rapids-bot bot merged commit a51cb17 into rapidsai:branch-24.12 Nov 18, 2024
86 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support heterogenous fanout type #4608

support heterogenous fanout type #4608

jnke2016 commented Aug 13, 2024 •

edited

Loading

ChuckHastings left a comment

ChuckHastings Aug 13, 2024

seunghwak Aug 14, 2024 •

edited

Loading

seunghwak Aug 14, 2024

seunghwak Aug 14, 2024

ChuckHastings Aug 13, 2024

ChuckHastings Aug 13, 2024

jnke2016 Aug 21, 2024 •

edited

Loading

ChuckHastings Aug 13, 2024

jnke2016 Aug 21, 2024

ChuckHastings Aug 13, 2024

jnke2016 Aug 21, 2024

ChuckHastings Aug 13, 2024

ChuckHastings Sep 26, 2024

ChuckHastings Oct 25, 2024

ChuckHastings Aug 13, 2024

alexbarghi-nv left a comment

alexbarghi-nv Nov 14, 2024

jnke2016 Nov 15, 2024

alexbarghi-nv Nov 14, 2024

jnke2016 Nov 15, 2024

alexbarghi-nv left a comment

alexbarghi-nv left a comment

alexbarghi-nv left a comment

alexbarghi-nv commented Nov 16, 2024

rlratzel left a comment

rlratzel Nov 18, 2024

rlratzel Nov 18, 2024

support heterogenous fanout type #4608

support heterogenous fanout type #4608

Conversation

jnke2016 commented Aug 13, 2024 • edited Loading

ChuckHastings left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seunghwak Aug 14, 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

jnke2016 Aug 21, 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

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

alexbarghi-nv left a comment

Choose a reason for hiding this comment

alexbarghi-nv left a comment

Choose a reason for hiding this comment

alexbarghi-nv left a comment

Choose a reason for hiding this comment

alexbarghi-nv commented Nov 16, 2024

rlratzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnke2016 commented Aug 13, 2024 •

edited

Loading

seunghwak Aug 14, 2024 •

edited

Loading

jnke2016 Aug 21, 2024 •

edited

Loading