Refactor MG neighborhood sampling and add SG implementation #2285

jnke2016 · 2022-05-18T20:41:21Z

A new implementation of neighborhood sampling meeting the new C API was merged.

This PR

Moves the old implementation to the dir proto
Refactor MG neighborhood sampling bindings based on the new implementation
Add bindings for SG neighborhood sampling
Refactor tests

DO NOT MERGE YET: PENDING fix for missing vertices in start_list
closes #2272

codecov-commenter · 2022-05-18T23:13:11Z

Codecov Report

Merging #2285 (1af8e7b) into branch-22.06 (38be932) will decrease coverage by 7.78%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06    #2285      +/-   ##
================================================
- Coverage         70.82%   63.03%   -7.79%     
================================================
  Files               170      104      -66     
  Lines             11036     4805    -6231     
================================================
- Hits               7816     3029    -4787     
+ Misses             3220     1776    -1444

Impacted Files	Coverage Δ
python/cugraph/cugraph/__init__.py	`100.00% <ø> (ø)`
python/cugraph/cugraph/centrality/__init__.py	`100.00% <ø> (ø)`
...graph/cugraph/centrality/betweenness_centrality.py	`89.65% <ø> (ø)`
...on/cugraph/cugraph/centrality/degree_centrality.py	`81.81% <ø> (ø)`
...graph/cugraph/centrality/eigenvector_centrality.py	`93.93% <ø> (ø)`
...thon/cugraph/cugraph/centrality/katz_centrality.py	`89.79% <ø> (+0.32%)`	⬆️
python/cugraph/cugraph/community/egonet.py	`97.36% <ø> (ø)`
...ython/cugraph/cugraph/community/ktruss_subgraph.py	`88.23% <ø> (+2.94%)`	⬆️
python/cugraph/cugraph/community/leiden.py	`100.00% <ø> (+7.69%)`	⬆️
python/cugraph/cugraph/community/louvain.py	`100.00% <ø> (+7.69%)`	⬆️
... and 139 more

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 861cef0...1af8e7b. Read the comment docs.

…quest properly

…6-fea_neighborhood_sampling

…changes

betochimas

I tried to create a deprecation warning wrapper, but found the deprecation message you used worked better for my PRs, so I appreciated that addition here. Overall, the PR LGTM, I only had a few minor comments

betochimas · 2022-05-24T21:19:26Z

python/cugraph/cugraph/dask/__init__.py

@@ -19,3 +19,6 @@
 from .community.louvain import louvain
 from .centrality.katz_centrality import katz_centrality
 from .components.connectivity import weakly_connected_components
+from .sampling.uniform_neighbor_sample import uniform_neighbor_sample
+# FIXME: This call is deprecated and will be removed next release
+from .sampling.neighborhood_sampling import neighborhood_sampling


If you make a FIXME for the deprecation warning of the old neighborhood_sampling, then this only lets the developers of cuGraph know that it's going away to be replaced by uniform_neighbor_sample. Wouldn't it be better to wrap neighborhood_sampling with a deprecation warning similar to some pylibcugraph calls?

I did add a warning in pylibcugraph/neighborhood_sampling and this FIXME is indeed for the cuGraph developers

betochimas · 2022-05-24T21:27:30Z

python/pylibcugraph/cufile.log

@@ -0,0 +1,3 @@
+ 24-05-2022 11:57:57:955 [pid=3446 tid=3446] ERROR  cufio-drv:632 nvidia-fs.ko driver not loaded


This file should not be left in, I can relate though.

Yep. I already have it removed locally but I am yet to push the changes

betochimas · 2022-05-24T22:46:46Z

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

+import dask_cudf
+import cudf
+
+from pylibcugraph.experimental import (


This is super minor thing, but all of these except for uniform_neighbor_sample can be taken out of the experimental namespace

betochimas · 2022-05-24T23:00:02Z

python/pylibcugraph/pylibcugraph/uniform_neighbor_sample.pyx

+    cupy_sources = copy_to_cupy_array(c_resource_handle_ptr, src_ptr)
+    cupy_destinations = copy_to_cupy_array(c_resource_handle_ptr, dst_ptr)
+    cupy_indices = copy_to_cupy_array_ids(c_resource_handle_ptr, index_ptr)
+    #print("indices are \n", cupy_indices)


Here's a few print debug lines left over

betochimas · 2022-05-24T23:01:47Z

python/cugraph/cugraph/sampling/uniform_neighbor_sample.py

+import cudf
+
+
+def uniform_neighbor_sample(G,


Might want to add both this SG and the MG uniform_neighbor_sample to the API docs

rlratzel

Looks good, and a lot of the feedback I have is already posted by others. I just have one request below.

rlratzel · 2022-05-26T19:32:11Z

python/cugraph/cugraph/experimental/dask/__init__.py

+from cugraph.dask.sampling.uniform_neighbor_sample import \
+    uniform_neighbor_sample
+uniform_neighbor_sample = \
+    experimental_warning_wrapper(uniform_neighbor_sample)


We can immediately promote this to stable too, which means we'll be replacing the current MG neighborhood sampling algo with this one (a breaking change). We think this is okay though, since this was mainly added for DGL and the DGL effort is just starting to use cugraph MG neighbor sampling (IOW, we don't think this will actually break anyone).
Of course, since the old version was released in 22.04, there's no way to guarantee a community member isn't using it, but we still think the chance is small enough not to warrant the extra expense of maintaining two versions to support our normal deprecation process. I'll add a "breaking" label to this PR though too.

I agree. It didn't make it into the 22.04 release html docs, so a community member who used it had to have followed development closely.

…ts in cugraph instead of pylibcugraph

betochimas

The tests LGTM, everything else looked good before, so I think this can be merged soon

BradReesWork · 2022-06-01T20:42:39Z

@gpucibot merge

Joseph Nke added 2 commits May 18, 2022 13:27

move the current implementation of mg neighborhood sampling to proto

98fd1b2

remove experimental prefix

063e443

jnke2016 requested a review from a team as a code owner May 18, 2022 20:41

refactor mg neighborhood sampling bindings

e6ed994

ChuckHastings and others added 2 commits May 19, 2022 01:47

add and test mechanism for creating graph with edge index as weight

4581645

Merge mechanism for creating graph with edge index as weight

57680f6

jnke2016 requested a review from a team as a code owner May 19, 2022 14:25

ChuckHastings and others added 6 commits May 19, 2022 13:56

rename create*_with_ids to create*_with_edge_ids

16cea30

rename create*_with_ids to create*_with_edge_ids from Chuck

2fd99b5

update python bindings to create graph with edge index as weight

3f90963

fix bug in MG case... cugraph_ops function doesn't handle an empty re…

d460193

…quest properly

merge bug fix in MG case by Chuck

c7d0a11

Merge remote-tracking branch 'upstream/branch-22.06' into branch-22.0…

e6210a2

…6-fea_neighborhood_sampling

rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python labels May 23, 2022

BradReesWork added this to the 22.06 milestone May 23, 2022

BradReesWork requested review from rlratzel, ChuckHastings, seunghwak and betochimas May 23, 2022 19:13

ChuckHastings approved these changes May 23, 2022

View reviewed changes

seunghwak approved these changes May 23, 2022

View reviewed changes

Joseph Nke added 5 commits May 23, 2022 23:12

add bindings for SG uniform_neighbor_sample

3b76a49

Merge remote-tracking branch 'upstream/branch-22.06' into branch-22.0…

e3b5fe4

…6-fea_neighborhood_sampling

remove debug print

9933120

update pylibcugraph uniform_neighbor_sample tests because of the API …

604ab0f

…changes

drop the directory proto

c579261

betochimas reviewed May 25, 2022

View reviewed changes

Joseph Nke added 2 commits May 25, 2022 14:39

enable support for weigths

626a833

remove debug prints, address PR comments

6d91c39

rlratzel requested changes May 26, 2022

View reviewed changes

rlratzel added breaking Breaking change and removed non-breaking Non-breaking change labels May 26, 2022

Joseph Nke and others added 5 commits May 26, 2022 21:15

move uniform_neighbor_sample to stable API, convert edge_ids to weigh…

f038688

…ts in cugraph instead of pylibcugraph

update uniform neighborhood sampling tests

d97ce67

merge latest change and update branch

c9482c2

remove uniform neighbor sample older mechanism

720b05d

add end of line

78f7dd6

betochimas approved these changes May 31, 2022

View reviewed changes

BradReesWork added the 5 - Ready to Merge label May 31, 2022

rlratzel approved these changes Jun 1, 2022

View reviewed changes

jnke2016 and others added 5 commits June 1, 2022 17:59

resolve merge conflict

ce97653

remove merge labels

7fdc09d

remove outdated fixme

b629ca9

remove unused import

8a8f063

add end of line

1af8e7b

rapids-bot bot merged commit addb9d7 into rapidsai:branch-22.06 Jun 1, 2022

jnke2016 deleted the branch-22.06-fea_neighborhood_sampling branch September 24, 2022 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MG neighborhood sampling and add SG implementation #2285

Refactor MG neighborhood sampling and add SG implementation #2285

jnke2016 commented May 18, 2022 •

edited

Loading

codecov-commenter commented May 18, 2022 •

edited

Loading

betochimas left a comment

betochimas May 24, 2022

jnke2016 May 25, 2022

betochimas May 24, 2022

jnke2016 May 25, 2022

betochimas May 24, 2022

jnke2016 May 26, 2022

betochimas May 24, 2022

jnke2016 May 26, 2022

betochimas May 24, 2022

rlratzel left a comment

rlratzel May 26, 2022

alexbarghi-nv May 26, 2022

betochimas May 26, 2022

betochimas left a comment

BradReesWork commented Jun 1, 2022

		@@ -0,0 +1,3 @@
		24-05-2022 11:57:57:955 [pid=3446 tid=3446] ERROR cufio-drv:632 nvidia-fs.ko driver not loaded

Refactor MG neighborhood sampling and add SG implementation #2285

Refactor MG neighborhood sampling and add SG implementation #2285

Conversation

jnke2016 commented May 18, 2022 • edited Loading

codecov-commenter commented May 18, 2022 • edited Loading

Codecov Report

betochimas 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

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

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

Choose a reason for hiding this comment

betochimas left a comment

Choose a reason for hiding this comment

BradReesWork commented Jun 1, 2022

jnke2016 commented May 18, 2022 •

edited

Loading

codecov-commenter commented May 18, 2022 •

edited

Loading