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

cuGraph-PyG Loader Improvements #3795

Merged
merged 32 commits into from
Sep 25, 2023
Merged

cuGraph-PyG Loader Improvements #3795

merged 32 commits into from
Sep 25, 2023

Conversation

alexbarghi-nv
Copy link
Member

@alexbarghi-nv alexbarghi-nv commented Aug 15, 2023 •
edited
Loading

Consolidates various speed improvements tested while running performance benchmarks. Avoids copying batch data, removes redundant data loading code, simplifies and improves de-offsetting, even though that is now being bypassed entirely for homogeneous graphs. Removes extra host to device copy. Properly flips the src/dst columns in the returned HeteroData minibatch objects, avoid exposing this to the end user.

I've confirmed this cuts the MFG time by a factor of 4.

Closes #3807

@alexbarghi-nv alexbarghi-nv self-assigned this Aug 15, 2023
@alexbarghi-nv alexbarghi-nv added this to the 23.10 milestone Aug 15, 2023
@alexbarghi-nv alexbarghi-nv added improvement Improvement / enhancement to an existing function breaking Breaking change labels Aug 16, 2023
@alexbarghi-nv alexbarghi-nv marked this pull request as ready for review August 16, 2023 23:04
@alexbarghi-nv alexbarghi-nv requested a review from a team as a code owner August 16, 2023 23:04
VibhuJawa
VibhuJawa reviewed Aug 25, 2023
View reviewed changes
Copy link
Member

@VibhuJawa VibhuJawa left a comment

Choose a reason for hiding this comment

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

The changes look good to me, just have asked for a bunch of clarifications.

python/cugraph-pyg/cugraph_pyg/data/cugraph_store.py Outdated Show resolved Hide resolved
python/cugraph-pyg/cugraph_pyg/loader/cugraph_node_loader.py Outdated Show resolved Hide resolved
python/cugraph-pyg/cugraph_pyg/sampler/cugraph_sampler.py Show resolved Hide resolved
python/cugraph-pyg/cugraph_pyg/sampler/cugraph_sampler.py Outdated Show resolved Hide resolved
@alexbarghi-nv @VibhuJawa
Persist on host memory
19f66d0 
Co-authored-by: Vibhu Jawa <[email protected]>
@alexbarghi-nv
Copy link
Member Author

Based on feedback from @VibhuJawa , this PR is going back to draft while I work on improvements

@alexbarghi-nv alexbarghi-nv marked this pull request as draft August 25, 2023 15:16
@alexbarghi-nv alexbarghi-nv marked this pull request as ready for review August 30, 2023 14:16
VibhuJawa
VibhuJawa suggested changes Aug 30, 2023
View reviewed changes
Copy link
Member

@VibhuJawa VibhuJawa left a comment

Choose a reason for hiding this comment

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

Code changes mostly look good, requested some changes to improve code clarity and readability.

python/cugraph-pyg/cugraph_pyg/data/cugraph_store.py Outdated Show resolved Hide resolved
python/cugraph-pyg/cugraph_pyg/loader/cugraph_node_loader.py Outdated Show resolved Hide resolved
VibhuJawa
VibhuJawa approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@VibhuJawa VibhuJawa left a comment

Choose a reason for hiding this comment

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

LGTM

rlratzel
rlratzel reviewed Sep 21, 2023
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.

I scanned it and only found two things that jumped out to me. Vibhu and Tingyu would be able to provide a more thorough review.

python/cugraph-pyg/cugraph_pyg/data/cugraph_store.py Outdated Show resolved Hide resolved
python/cugraph-pyg/cugraph_pyg/sampler/cugraph_sampler.py Show resolved Hide resolved
acostadon
acostadon approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@acostadon acostadon left a comment

Choose a reason for hiding this comment

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

Looks great as much as I understood. Just saw aplace where I wasn't sure what wasn't working due to the fixme

@BradReesWork
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit fe17abc into rapidsai:branch-23.10 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlratzel rlratzel rlratzel left review comments

@acostadon acostadon acostadon approved these changes

@BradReesWork BradReesWork BradReesWork approved these changes

@VibhuJawa VibhuJawa VibhuJawa approved these changes

Assignees

@alexbarghi-nv alexbarghi-nv

Labels
breaking Breaking change improvement Improvement / enhancement to an existing function
Projects
None yet
Milestone
23.10
Development

Successfully merging this pull request may close these issues.

cuGraph-PyG Should Test Training in CI
5 participants
@alexbarghi-nv @BradReesWork @rlratzel @VibhuJawa @acostadon