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

Remote Graph Wrappers for cuGraph-Service #2821

Merged
merged 36 commits into from
Oct 27, 2022
Merged

Remote Graph Wrappers for cuGraph-Service #2821

merged 36 commits into from
Oct 27, 2022

Conversation

alexbarghi-nv
Copy link
Member

@alexbarghi-nv alexbarghi-nv commented Oct 18, 2022 •
edited
Loading

Resolves https://github.com/rapidsai/graph_dl/issues/76
Merge after #2804

This PR defines a remote interface for Graph and PropertyGraph so they can be easily substituted in a client application that does not use cugraph directly. For PropertyGraph, this PR also implements most of the functionality of a native PropertyGraph.

Note: this PR does not include the upcoming "dispatch" updates for running cugraph algorithms or subgraph extraction, which are covered under https://github.com/rapidsai/graph_dl/issues/80

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@alexbarghi-nv alexbarghi-nv added feature request New feature or request non-breaking Non-breaking change labels Oct 18, 2022
@alexbarghi-nv alexbarghi-nv added this to the 22.12 milestone Oct 18, 2022
@alexbarghi-nv alexbarghi-nv self-assigned this Oct 18, 2022
@alexbarghi-nv alexbarghi-nv marked this pull request as ready for review October 18, 2022 19:40
@alexbarghi-nv alexbarghi-nv requested a review from a team as a code owner October 18, 2022 19:40
@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022 •
edited
Loading

Codecov Report

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

❗ Current head 0af0e27 differs from pull request most recent head 8f28820. Consider uploading reports for the commit 8f28820 to get more accurate results

Additional details and impacted files 
@@               Coverage Diff               @@
##             branch-22.12    #2821   +/-   ##
===============================================
  Coverage                ?   62.61%           
===============================================
  Files                   ?      114           
  Lines                   ?     6395           
  Branches                ?        0           
===============================================
  Hits                    ?     4004           
  Misses                  ?     2391           
  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.

VibhuJawa
VibhuJawa suggested changes Oct 21, 2022
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.

Thanks for working on this @alexbarghi-nv , I can see how this can be very useful. Overall approach looks good but have raised some questions.

python/cugraph_service/cugraph_service_client/remote_graph.py Show resolved Hide resolved
python/cugraph_service/cugraph_service_client/remote_graph.py Outdated Show resolved Hide resolved
python/cugraph_service/cugraph_service_client/remote_graph.py Outdated Show resolved Hide resolved
python/cugraph_service/cugraph_service_client/remote_graph.py Outdated Show resolved Hide resolved
python/cugraph_service/cugraph_service_client/remote_graph.py Outdated Show resolved Hide resolved
python/cugraph_service/cugraph_service_server/cugraph_handler.py Show resolved Hide resolved
VibhuJawa
VibhuJawa suggested changes Oct 25, 2022
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.

Have requested small changes, looks good other-wise

python/cugraph/cugraph/structure/property_graph.py Outdated Show resolved Hide resolved
python/cugraph_service/cugraph_service_client/remote_graph.py Outdated Show resolved Hide resolved
@VibhuJawa
Copy link
Member

@alexbarghi-nv is this good to merge ?

@alexbarghi-nv
Copy link
Member Author

This is good to merge.

@VibhuJawa
Copy link
Member

@gpucibot merge

@VibhuJawa
Copy link
Member

@rlratzel , Can you merge this please, i dont think i have privileges.

@rlratzel
Copy link
Contributor

rlratzel commented Oct 27, 2022 •
edited
Loading

@gpucibot merge

1 similar comment
@rlratzel
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 96d04f0 into rapidsai:branch-22.12 Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VibhuJawa VibhuJawa VibhuJawa requested changes

@jnke2016 jnke2016 jnke2016 approved these changes

@rlratzel rlratzel rlratzel approved these changes

Assignees

@alexbarghi-nv alexbarghi-nv

Labels
feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Milestone
22.12
Development

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