Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[REVIEW]Uniform neighbor sample #2450

Merged
merged 20 commits into from
Aug 2, 2022
Merged

[REVIEW]Uniform neighbor sample #2450

merged 20 commits into from
Aug 2, 2022

Conversation

VibhuJawa
Copy link
Member

This PR switches cugraphstore to use uniform neighbor sampling.

Opening this in favor of #2426

@VibhuJawa VibhuJawa requested a review from a team as a code owner July 27, 2022 06:41
@VibhuJawa VibhuJawa added feature request New feature or request improvement Improvement / enhancement to an existing function GNN non-breaking Non-breaking change labels Jul 27, 2022
@VibhuJawa VibhuJawa mentioned this pull request Jul 27, 2022
Closed
@VibhuJawa VibhuJawa removed the feature request New feature or request label Jul 27, 2022
VibhuJawa
VibhuJawa commented Jul 27, 2022
View reviewed changes
python/cugraph/cugraph/gnn/graph_store.py Outdated
source=src_n,
destination=dst_n,
edge_attr="weight",
renumber=False,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need to switch to renumber=True.

See issue: #2446

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renumber=True. still is causing errors. Todo: Raise a MRE without it and comment to the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

legacy_renum_only=True

python/cugraph/cugraph/gnn/graph_store.py Outdated Show resolved Hide resolved
@BradReesWork BradReesWork added this to the 22.08 milestone Jul 27, 2022
rlratzel
rlratzel reviewed Jul 28, 2022
View reviewed changes
Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a comment and a couple of minor suggestions.

python/cugraph/cugraph/gnn/graph_store.py Show resolved Hide resolved
python/cugraph/cugraph/gnn/graph_store.py Show resolved Hide resolved
python/cugraph/cugraph/gnn/graph_store.py Show resolved Hide resolved
alexbarghi-nv
alexbarghi-nv reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

@alexbarghi-nv alexbarghi-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine; just make sure all the subgraphs are being created with legacy_renum_only=True

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@VibhuJawa VibhuJawa changed the base branch from branch-22.08 to branch-22.10 July 29, 2022 17:55
@VibhuJawa VibhuJawa requested review from a team as code owners July 29, 2022 17:55
@VibhuJawa VibhuJawa changed the base branch from branch-22.10 to branch-22.08 July 29, 2022 17:55
alexbarghi-nv
alexbarghi-nv approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@alexbarghi-nv alexbarghi-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022 •
edited
Loading

Codecov Report

Merging #2450 (911b866) into branch-22.08 (1a29434) will increase coverage by 38.11%.
The diff coverage is 91.42%.

@@                Coverage Diff                @@
##           branch-22.08    #2450       +/-   ##
=================================================
+ Coverage         23.10%   61.22%   +38.11%     
=================================================
  Files               106      106               
  Lines              5539     5568       +29     
=================================================
+ Hits               1280     3409     +2129     
+ Misses             4259     2159     -2100
Impacted Files Coverage Δ
python/cugraph/cugraph/gnn/graph_store.py 79.24% <91.42%> (+50.07%) ⬆️
.../cugraph/cugraph/experimental/datasets/__init__.py 100.00% <0.00%> (ø)
python/cugraph/cugraph/dask/comms/comms.py 34.06% <0.00%> (+5.49%) ⬆️
python/cugraph/cugraph/generators/rmat.py 25.96% <0.00%> (+12.50%) ⬆️
python/cugraph/cugraph/dask/traversal/bfs.py 33.33% <0.00%> (+13.33%) ⬆️
python/cugraph/cugraph/testing/utils.py 79.14% <0.00%> (+33.12%) ⬆️
python/cugraph/cugraph/link_prediction/overlap.py 59.09% <0.00%> (+36.36%) ⬆️
python/cugraph/cugraph/linear_assignment/lap.py 71.42% <0.00%> (+42.85%) ⬆️
python/cugraph/cugraph/layout/force_atlas2.py 61.11% <0.00%> (+44.44%) ⬆️
... and 42 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 1a29434...911b866. Read the comment docs.

@VibhuJawa
Copy link
Member Author

@gpucibot rerun tests

@rlratzel
Updated imports to be compatible with latest version of cupy (rapidsa…
93e15c0 
…i#2473)

Updated imports to be compatible with latest version of cupy and also changed corresponding scipy imports for consistency.

Authors:
  - Rick Ratzel (https://github.com/rlratzel)

Approvers:
  - Brad Rees (https://github.com/BradReesWork)

URL: rapidsai#2473
@rlratzel
Copy link
Contributor

rlratzel commented Aug 1, 2022

rerun tests

#2473 was merged which should resolve test failures.

@ajschmidt8 ajschmidt8 removed the request for review from a team August 1, 2022 15:58
@ajschmidt8
Copy link
Member

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

@VibhuJawa
Copy link
Member Author

@gpucibot rerun tests

There might be some weirdness around sampling on different CUDA versions. Testing it. Lets hold of merging even if CI is passing.

@VibhuJawa
Copy link
Member Author

@gpucibot rerun tests

Rerunning cause the gpuCI/cugraph/gpu/cuda/11.2/driver-495/python/3.9/ubuntu18.04 build timed out with below. All other tests passed.

17:37:26 Download error (28) Timeout was reached [https://conda.anaconda.org/rapidsai-nightly/linux-64/dask-cuda-22.08.00a220801-py39_g283a087_32.tar.bz2]
17:37:26 SSL connection timeout

@VibhuJawa VibhuJawa requested review from a team and removed request for a team August 2, 2022 20:29
@rlratzel rlratzel mentioned this pull request Aug 2, 2022
Closed
rlratzel
rlratzel approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests and code look good. I did file this so we can address the FIXMEs ASAP in 22.10

@rlratzel
Copy link
Contributor

rlratzel commented Aug 2, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d50622f into rapidsai:branch-22.08 Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexbarghi-nv alexbarghi-nv alexbarghi-nv approved these changes

@rlratzel rlratzel rlratzel approved these changes

Assignees
No one assigned
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
22.08
Development

Successfully merging this pull request may close these issues.
6 participants
@VibhuJawa @codecov-commenter @rlratzel @ajschmidt8 @alexbarghi-nv @BradReesWork