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] Bring up cugraph_dgl_repo #2896

Merged
merged 19 commits into from
Nov 15, 2022
Merged

[REVIEW] Bring up cugraph_dgl_repo #2896

merged 19 commits into from
Nov 15, 2022

Conversation

VibhuJawa
Copy link
Member

@VibhuJawa VibhuJawa commented Nov 8, 2022 •
edited
Loading

This PR resolves https://github.com/rapidsai/graph_dl/issues/92

  • Add code
  • Add tests
  • Add readme

@VibhuJawa VibhuJawa added non-breaking Non-breaking change feature request New feature or request labels Nov 8, 2022
@VibhuJawa VibhuJawa changed the title [WIP]Bring up cugraph_dgl_repo [REVIEW] Bring up cugraph_dgl_repo Nov 8, 2022
@VibhuJawa VibhuJawa marked this pull request as ready for review November 8, 2022 18:49
@VibhuJawa VibhuJawa requested a review from a team as a code owner November 8, 2022 18:49
@VibhuJawa VibhuJawa changed the title [REVIEW] Bring up cugraph_dgl_repo [WIP] Bring up cugraph_dgl_repo Nov 8, 2022
@VibhuJawa VibhuJawa changed the title [WIP] Bring up cugraph_dgl_repo [REVIEW] Bring up cugraph_dgl_repo Nov 8, 2022
alexbarghi-nv
alexbarghi-nv approved these changes Nov 9, 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.

👍

@VibhuJawa VibhuJawa requested a review from a team as a code owner November 10, 2022 19:56
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.12@e7e4f90). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@               Coverage Diff               @@
##             branch-22.12    #2896   +/-   ##
===============================================
  Coverage                ?   60.81%           
===============================================
  Files                   ?      122           
  Lines                   ?     6891           
  Branches                ?        0           
===============================================
  Hits                    ?     4191           
  Misses                  ?     2700           
  Partials                ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rlratzel
rlratzel requested changes Nov 13, 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.

I didn't see any showstoppers, but I did have several questions and minor requests. I like that the test coverage seems good and that they are easy to follow.

python/cugraph-dgl/cugraph_dgl/cugraph_storage.py Outdated
Comment on lines 32 to 36


Read the user guide chapter :ref:`guide-`CuGraphStorage` for an in-depth
explanation about its usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: redundant blank lines

python/cugraph-dgl/cugraph_dgl/cugraph_storage.py Outdated Show resolved Hide resolved
python/cugraph-dgl/cugraph_dgl/cugraph_storage.py Outdated Show resolved Hide resolved
python/cugraph-dgl/cugraph_dgl/cugraph_storage.py Outdated Show resolved Hide resolved
python/cugraph-dgl/cugraph_dgl/cugraph_storage.py Outdated Show resolved Hide resolved
python/cugraph-dgl/cugraph_dgl/cugraph_storage.py Outdated
num_nodes_dict: dict,
single_gpu: bool = True,
cugraph_service_client=None,
device_id=0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout this file too are cases of some args with type hints, some without. Should we be enforcing in reviews that these be present on new code, or would this be a FIXME type of situation for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added all the type-hints that are possible without adding extra imports. The missing ones are now dgl objects and dataframe objects that depends on the underlying library (for dgl bakcend, it can be pytorch, tensorflow,keras) and for dataframe can be (cuDF,dask_cuDF, pandas).

Copy link
Member Author

Choose a reason for hiding this comment

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

Have added issue: #2922 to track it.

python/cugraph-dgl/cugraph_dgl/utils/test_utils.py Outdated Show resolved Hide resolved
python/cugraph-dgl/cugraph_dgl/utils/test_utils.py Outdated Show resolved Hide resolved
python/cugraph-dgl/setup.py Outdated Show resolved Hide resolved
python/cugraph-dgl/tests/test_cugraph_storage.py Outdated Show resolved Hide resolved
VibhuJawa and others added 2 commits November 14, 2022 09:38
@VibhuJawa @rlratzel
Apply suggestions from code review
d51be34 
Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>
@VibhuJawa
Added type-hints whereever possible
7bd825f
@VibhuJawa VibhuJawa mentioned this pull request Nov 14, 2022
Closed
2 tasks
rlratzel
rlratzel approved these changes Nov 14, 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.

LGTM, thanks.

@rlratzel
Copy link
Contributor

@gpucibot merge

@VibhuJawa VibhuJawa added the DO NOT MERGE Hold off on merging; see PR for details label Nov 15, 2022
@VibhuJawa VibhuJawa added 5 - Ready to Merge and removed DO NOT MERGE Hold off on merging; see PR for details labels Nov 15, 2022
@rapids-bot rapids-bot bot merged commit ee3195a into rapidsai:branch-22.12 Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlratzel rlratzel rlratzel approved these changes

@raydouglass raydouglass raydouglass approved these changes

@sevagh sevagh sevagh approved these changes

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

Assignees
No one assigned
Labels
feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

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