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] Fix MNMG failures in mg_dgl_extensions #2786

Merged
merged 15 commits into from
Oct 11, 2022
Merged

[REVIEW] Fix MNMG failures in mg_dgl_extensions #2786

merged 15 commits into from
Oct 11, 2022

Conversation

VibhuJawa
Copy link
Member

@VibhuJawa VibhuJawa commented Oct 7, 2022 •
edited
Loading

This PR fixes the below errors that have popped up in MNMG testing.

@VibhuJawa VibhuJawa self-assigned this Oct 7, 2022
@VibhuJawa VibhuJawa added bug Something isn't working non-breaking Non-breaking change labels Oct 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022 •
edited
Loading

Codecov Report

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

Additional details and impacted files 
@@               Coverage Diff               @@
##             branch-22.12    #2786   +/-   ##
===============================================
  Coverage                ?   60.45%           
===============================================
  Files                   ?      111           
  Lines                   ?     6514           
  Branches                ?        0           
===============================================
  Hits                    ?     3938           
  Misses                  ?     2576           
  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 changed the title [WIP] Fix frontier includes out-of-range keys error [WIP] Fix MNMG failures in mg_dgl_extensions Oct 7, 2022
VibhuJawa
VibhuJawa commented Oct 7, 2022
View reviewed changes
python/cugraph/cugraph/dask/structure/mg_property_graph.py Outdated
Comment on lines 488 to 492
if isinstance(vertex_ids, int):
vertex_ids = [vertex_ids]
elif not isinstance(vertex_ids,
(list, slice, self.__series_type)):
vertex_ids = list(vertex_ids)
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 think we should let the underlying library handle it rather than limiting location to these types.

Also, this fails with GPU objects like cupy , cudf.Series , for numpy arrays we pay a cost.

python/cugraph/cugraph/dask/structure/mg_property_graph.py Show resolved Hide resolved
python/cugraph/cugraph/structure/property_graph.py Outdated Show resolved Hide resolved
python/cugraph/cugraph/structure/property_graph.py Outdated Show resolved Hide resolved
@VibhuJawa VibhuJawa changed the title [WIP] Fix MNMG failures in mg_dgl_extensions [REVIEW] Fix MNMG failures in mg_dgl_extensions Oct 8, 2022
@VibhuJawa VibhuJawa marked this pull request as ready for review October 8, 2022 01:28
@VibhuJawa VibhuJawa requested a review from a team as a code owner October 8, 2022 01:28
@BradReesWork BradReesWork added this to the 22.12 milestone Oct 10, 2022
rlratzel
rlratzel requested changes Oct 11, 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.

Thanks for adding the extra test coverage. Just had a few minor suggestions/fixes/typos

python/cugraph/cugraph/gnn/graph_store.py Outdated 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
python/cugraph/cugraph/gnn/graph_store.py Outdated Show resolved Hide resolved
VibhuJawa and others added 2 commits October 11, 2022 10:29
@VibhuJawa @rlratzel
Update python/cugraph/cugraph/gnn/graph_store.py
56b088a 
Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>
@VibhuJawa @rlratzel
Update python/cugraph/cugraph/gnn/graph_store.py
49e42aa 
Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>
@VibhuJawa
Copy link
Member Author

Thanks for adding the extra test coverage. Just had a few minor suggestions/fixes/typos

Thanks for the review Rick, Addressed all of your comments so should be ready for another round of reviews.

@rlratzel
Copy link
Contributor

@gpucibot merge

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

@eriknw eriknw eriknw approved these changes

@rlratzel rlratzel rlratzel approved these changes

@BradReesWork BradReesWork Awaiting requested review from BradReesWork

Assignees

@VibhuJawa VibhuJawa

Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Milestone
22.12
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@VibhuJawa @codecov-commenter @rlratzel @eriknw @BradReesWork