[REVIEW] Allow passing a dict in feat_name for add_edge_data and add_node_data by VibhuJawa · Pull Request #2795 · rapidsai/cugraph

VibhuJawa · 2022-10-12T03:59:59Z

This PR adds support for dict in feat_name for add_edge_data and add_node_data.

Code Cleanup and ensure it lines with DGL
Add tests
Fix a get_feature_storage bug when device is set

…raph into dgl_graphstore_fix

VibhuJawa · 2022-10-13T20:27:22Z

python/cugraph/cugraph/gnn/graph_store.py

+        feat_name=None,
+        contains_vector_features=False,
    ):


Changed to this to keep the API cleaner and intuitive as you would want to provide feat_name together with contains_vector_features

VibhuJawa · 2022-10-13T20:28:34Z

python/cugraph/cugraph/gnn/graph_store.py

                tensor = tensor.to(device)
-            else:
-                return tensor
+        return tensor


Fixed an error here when a user provides device, cant test this as that needs Pytorch installed here.

VibhuJawa · 2022-10-13T20:28:59Z

python/cugraph/cugraph/gnn/graph_store.py

        self.__clear_cached_properties()

-    def get_node_storage(self, feat_name, ntype=None):
+    def get_node_storage(self, key, ntype=None, indices_offset=0):


Changed to key to match DGL.

See : https://github.com/dmlc/dgl/blob/2630d2eb0231cc9de2cb953e49d092ad556e3be9/python/dgl/heterograph.py#L1604-L1605

VibhuJawa · 2022-10-13T20:29:04Z

python/cugraph/cugraph/gnn/graph_store.py

        )

-    def get_edge_storage(self, feat_name, etype=None):
+    def get_edge_storage(self, key, etype=None, indices_offset=0):


Changed to key to match DGL.

See: https://github.com/dmlc/dgl/blob/2630d2eb0231cc9de2cb953e49d092ad556e3be9/python/dgl/heterograph.py#L1608-L1610

VibhuJawa · 2022-10-13T20:32:30Z

python/cugraph/cugraph/gnn/graph_store.py

        self.storage_type = storage_type

        self.from_dlpack = from_dlpack
+        self.indices_offset = indices_offset


Setting offset makes upstream code cleaner

codecov-commenter · 2022-10-13T22:22:42Z

Codecov Report

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

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-22.12    #2795   +/-   ##
===============================================
  Coverage                ?   60.48%           
===============================================
  Files                   ?      111           
  Lines                   ?     6526           
  Branches                ?        0           
===============================================
  Hits                    ?     3947           
  Misses                  ?     2579           
  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 · 2022-10-16T19:41:19Z

@gpucibot rerun tests

rlratzel

Mostly minor things: some questions, suggestions, typos, etc.

python/cugraph/cugraph/gnn/graph_store.py

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

VibhuJawa

Thanks @rlratzel , Have addressed your reviews and added comment on the questions. Let me know what else needs to be done.

VibhuJawa · 2022-10-18T20:26:44Z

@gpucibot merge

VibhuJawa · 2022-10-18T22:59:35Z

@gpucibot merge

rlratzel · 2022-10-19T02:42:40Z

@gpucibot merge

… (#2795) This PR adds support for dict in feat_name for add_edge_data and add_node_data. - [x] Code Cleanup and ensure it lines with DGL - [x] Add tests - [x] Fix a `get_feature_storage` bug when device is set Authors: - Vibhu Jawa (https://github.com/VibhuJawa) Approvers: - Rick Ratzel (https://github.com/rlratzel) URL: rapidsai/cugraph#2795

Working upstream DGL

de7c4d8

VibhuJawa added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 12, 2022

VibhuJawa added 5 commits October 12, 2022 11:38

More cleanup

13f9f9a

Added python/cugraph/cugraph/gnn/graph_store.py

45b9ea3

Merge branch 'branch-22.12' into dgl_graphstore_fix

525bc9b

Added tests for handling dicts

cb235f6

Merge branch 'dgl_graphstore_fix' of https://github.com/VibhuJawa/cug…

5ee39cb

…raph into dgl_graphstore_fix

VibhuJawa changed the title ~~[WIP] Allow passing a dict in feat_name for add_edge_data and add_node_data~~ [REVIEW] Allow passing a dict in feat_name for add_edge_data and add_node_data Oct 13, 2022

VibhuJawa marked this pull request as ready for review October 13, 2022 00:53

VibhuJawa requested a review from a team as a code owner October 13, 2022 00:53

VibhuJawa added 3 commits October 12, 2022 17:59

Formatting using pre-commit

5ba162f

test cases updated to match new API

32ef3ec

Working MG tests

23daf8a

VibhuJawa commented Oct 13, 2022

View reviewed changes

fix docstring order

d0a7e29

rlratzel requested changes Oct 18, 2022

View reviewed changes

VibhuJawa and others added 6 commits October 18, 2022 09:42

Update python/cugraph/cugraph/gnn/graph_store.py

5084247

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

Update python/cugraph/cugraph/gnn/graph_store.py

5d93920

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

Update python/cugraph/cugraph/gnn/graph_store.py

dd5f090

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

Update python/cugraph/cugraph/gnn/graph_store.py

8a778d8

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

Update python/cugraph/cugraph/gnn/graph_store.py

7245ad4

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

Update python/cugraph/cugraph/gnn/graph_store.py

45e257d

Co-authored-by: Rick Ratzel <3039903+rlratzel@users.noreply.github.com>

VibhuJawa commented Oct 18, 2022

View reviewed changes

rlratzel approved these changes Oct 18, 2022

View reviewed changes

fix style errors

81a758e

VibhuJawa mentioned this pull request Oct 18, 2022

[WIP] Fix num vertices error #2824

Closed

This was referenced Oct 18, 2022

[Feature]Add cugraph local storage support to DGL rapidsai/dgl#16

Open

[REVIEW]Fix out of index errors encountered with sampling on out of index samples #2825

Merged

rapids-bot bot merged commit fbd5f20 into rapidsai:branch-22.12 Oct 19, 2022

Conversation

VibhuJawa commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

VibhuJawa Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VibhuJawa commented Oct 16, 2022

Uh oh!

rlratzel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VibhuJawa left a comment

Choose a reason for hiding this comment

Uh oh!

VibhuJawa commented Oct 18, 2022

Uh oh!

VibhuJawa commented Oct 18, 2022

Uh oh!

rlratzel commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VibhuJawa commented Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 13, 2022 •

edited

Loading