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.

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

[REVIEW] Graph sage example #2925

Merged
merged 34 commits into from
Mar 28, 2023

Conversation

VibhuJawa
Copy link
Member

@VibhuJawa VibhuJawa commented Nov 15, 2022

This PR depends upon #2924 .

We add Graph-Sage example to claim dgl , cugraph integration.

Waiting on SWIPAT before merging

CC: @TristonC

@VibhuJawa VibhuJawa requested a review from a team as a code owner November 15, 2022 04:08
@VibhuJawa VibhuJawa added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 15, 2022
@VibhuJawa VibhuJawa changed the title Graph sage example [REVIEW] Graph sage example Nov 15, 2022
@VibhuJawa VibhuJawa added the DO NOT MERGE Hold off on merging; see PR for details label Nov 15, 2022
@BradReesWork BradReesWork added this to the 22.12 milestone Nov 15, 2022
@VibhuJawa VibhuJawa changed the base branch from branch-22.12 to branch-23.02 November 15, 2022 18:57
@VibhuJawa VibhuJawa changed the base branch from branch-23.02 to branch-22.12 November 15, 2022 18:57
@VibhuJawa VibhuJawa changed the base branch from branch-22.12 to branch-23.02 November 16, 2022 01:03
@VibhuJawa VibhuJawa changed the base branch from branch-23.02 to branch-22.12 November 16, 2022 01:03
Copy link
Member

@tingyu66 tingyu66 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 have the following comments:

  • I don't think we need to support mode other than cugraph_storage, and the code have assumed to use cugraph_storage in a few places, indicated by calling graph.get_node_storage()
  • Need more explanation around the batch_size and the rmm option to users.

Copy link
Contributor

@wangxiaoyunNV wangxiaoyunNV left a comment

Choose a reason for hiding this comment

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

see my comments in the code

Copy link
Contributor

@wangxiaoyunNV wangxiaoyunNV left a comment

Choose a reason for hiding this comment

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

Please see me comments in the code.

alexbarghi-nv
alexbarghi-nv previously approved these changes Nov 21, 2022
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 like Xiaoyun addressed everything that needed additional review. Approved.

@VibhuJawa VibhuJawa changed the base branch from branch-22.12 to branch-23.02 November 22, 2022 15:47
@VibhuJawa VibhuJawa changed the base branch from branch-23.02 to branch-22.12 November 22, 2022 15:48
@BradReesWork BradReesWork changed the base branch from branch-23.02 to branch-23.04 January 27, 2023 17:43
@BradReesWork BradReesWork dismissed stale reviews from wangxiaoyunNV, tingyu66, and alexbarghi-nv January 27, 2023 17:43

The base branch was changed.

@BradReesWork BradReesWork modified the milestones: 23.02, 23.04 Jan 27, 2023
@kingmesal
Copy link

@VibhuJawa any status updates?

@VibhuJawa VibhuJawa removed the DO NOT MERGE Hold off on merging; see PR for details label Mar 8, 2023
@VibhuJawa
Copy link
Member Author

@VibhuJawa any status updates?

Yeah, we finally have approval to merge this in now :-D . Will update this PR

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.

LGTM, just a couple of minor comments

@VibhuJawa
Copy link
Member Author

@rlratzel , All your reviews have been addressed.

@VibhuJawa
Copy link
Member Author

@tingyu66 , @alexbarghi-nv Waiting on you all's approval too.

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.

👍

Copy link
Member

@tingyu66 tingyu66 left a comment

Choose a reason for hiding this comment

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

👍

@alexbarghi-nv
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit a6bf505 into rapidsai:branch-23.04 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants