[REVIEW]Add remote storage support #2859

VibhuJawa · 2022-10-28T00:36:26Z

Add remote CuGraphRemoteStore support to enable using of graph service with DGL .

Tests:

This PR depends upon:
#2855
#2850

…to graph creation extensions, changed how unloading extensions work to unload by unique module name, make numpy and cupy optional modules in the client, updated Value and ValueWrapper to handle doubles and lists (recursive), added tests for new extension changes.

…one, added test for extension modules returning results to client GPUs (code not done yet).

…2-cugraph_service_extension_enhancements

…(), updated tests, added test for upcoming ability to load extensions via import paths.

…kage path (eg. import foo.bar.baz), updated test.

…ns, added tests for error conditions, added benchmark for call_extension test.

…2-cugraph_service_extension_enhancements

…raph into add_remore_storage_support

VibhuJawa · 2022-11-03T19:19:30Z

rerun tests

codecov-commenter · 2022-11-03T21:07:22Z

Codecov Report

Base: 62.76% // Head: 60.75% // Decreases project coverage by -2.00% ⚠️

Coverage data is based on head (5235e46) compared to base (5c15341).
Patch coverage: 35.53% of modified lines in pull request are covered.

❗ Current head 5235e46 differs from pull request most recent head bf49417. Consider uploading reports for the commit bf49417 to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12    #2859      +/-   ##
================================================
- Coverage         62.76%   60.75%   -2.01%     
================================================
  Files               117      122       +5     
  Lines              6451     6824     +373     
================================================
+ Hits               4049     4146      +97     
- Misses             2402     2678     +276

Impacted Files	Coverage Δ
.../gnn/dgl_extensions/service_extensions/add_data.py	`0.00% <0.00%> (ø)`
.../gnn/dgl_extensions/service_extensions/sampling.py	`0.00% <0.00%> (ø)`
...ugraph/gnn/dgl_extensions/cugraph_service_store.py	`17.79% <17.79%> (ø)`
...thon/cugraph/cugraph/dask/sampling/random_walks.py	`22.91% <22.91%> (ø)`
...graph/cugraph/gnn/dgl_extensions/utils/add_data.py	`25.00% <25.00%> (-62.50%)`	⬇️
...raph/cugraph/gnn/dgl_extensions/feature_storage.py	`48.78% <39.34%> (-30.71%)`	⬇️
...ugraph/cugraph/gnn/dgl_extensions/cugraph_store.py	`84.24% <55.55%> (-4.65%)`	⬇️
...graph/cugraph/gnn/dgl_extensions/utils/sampling.py	`87.17% <85.71%> (-0.44%)`	⬇️
...ph/cugraph/gnn/dgl_extensions/utils/feature_map.py	`87.50% <87.50%> (ø)`
python/cugraph/cugraph/sampling/random_walks.py	`92.42% <92.15%> (-4.13%)`	⬇️
... and 4 more

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

Mostly minor things, looks fine to me otherwise.

python/cugraph/cugraph/gnn/dgl_extensions/cugraph_service_store.py

rlratzel · 2022-11-04T21:01:25Z

python/cugraph/cugraph/gnn/dgl_extensions/cugraph_service_store.py

+                        "are more than one node types."
+                    )
+                )
+            ntype = ntypes[0]


Where does ntype get used after this line?

Wow, Yup, this was a bug. I now pass it down here.

cugraph/python/cugraph/cugraph/gnn/dgl_extensions/feature_storage.py

Lines 146 to 151 in d24ab52

result = self.pg.get_vertex_data(

vertex_ids=indices, columns=self.columns, types=self.types_to_fetch

)

else:

result = self.pg.get_edge_data(

edge_ids=indices, columns=self.columns, types=self.types_to_fetch

We previously had a bug in PG which got fixed by this #2523 but i missed linking it so this slipped through.

I have added tests here:

cugraph/python/cugraph/cugraph/tests/test_dgl_extension_graph_store.py

Lines 391 to 423 in d24ab52

def test_get_node_storage_ntypes():

node_ser = cudf.Series([1, 2, 3])

feat_ser = cudf.Series([1.0, 1.0, 1.0])

df = cudf.DataFrame({"node_ids": node_ser, "feat": feat_ser})

pg = PropertyGraph()

gs = CuGraphStore(pg, backend_lib="cupy")

gs.add_node_data(df, "node_ids", ntype="nt.a")

node_ser = cudf.Series([4, 5, 6])

feat_ser = cudf.Series([2.0, 2.0, 2.0])

df = cudf.DataFrame({"node_ids": node_ser, "feat": feat_ser})

gs.add_node_data(df, "node_ids", ntype="nt.b")

# All indices from a single ntype

output_ar = gs.get_node_storage(key="feat", ntype="nt.a").fetch([1, 2, 3])

cp.testing.assert_array_equal(cp.asarray([1, 1, 1], dtype=cp.float32), output_ar)

# Indices from other ntype are ignored

output_ar = gs.get_node_storage(key="feat", ntype="nt.b").fetch([1, 2, 5])

cp.testing.assert_array_equal(cp.asarray([2.0], dtype=cp.float32), output_ar)

def test_get_edge_storage_gs(dataset1_CuGraphStore):

etype = "('user', 'relationship', 'user')"

fs = dataset1_CuGraphStore.get_edge_storage("relationships_k", etype)

relationship_t = fs.fetch([6, 7, 8], device="cuda")

relationships_df = create_df_from_dataset(

dataset1["relationships"][0], dataset1["relationships"][1]

)

cudf_ar = relationships_df["relationship_type"].iloc[[0, 1, 2]].values

assert cp.allclose(cudf_ar, relationship_t)

rlratzel · 2022-11-04T21:02:21Z

python/cugraph/cugraph/gnn/dgl_extensions/cugraph_service_store.py

+                    )
+                )
+
+            etype = etypes[0]


Where does etype get used after this line?

Added tests here, similar to ntype.

cugraph/python/cugraph/cugraph/tests/test_dgl_extension_graph_store.py

Lines 413 to 423 in d24ab52

def test_get_edge_storage_gs(dataset1_CuGraphStore):

etype = "('user', 'relationship', 'user')"

fs = dataset1_CuGraphStore.get_edge_storage("relationships_k", etype)

relationship_t = fs.fetch([6, 7, 8], device="cuda")

relationships_df = create_df_from_dataset(

dataset1["relationships"][0], dataset1["relationships"][1]

)

cudf_ar = relationships_df["relationship_type"].iloc[[0, 1, 2]].values

assert cp.allclose(cudf_ar, relationship_t)

python/cugraph/cugraph/gnn/dgl_extensions/cugraph_service_store.py

…e.py Co-authored-by: Rick Ratzel <[email protected]>

…Jawa/cugraph into add_remore_storage_support

VibhuJawa · 2022-11-07T18:11:30Z

@rlratzel , All reviews have been addressed. Please give this another look

python/cugraph/cugraph/gnn/__init__.py

alexbarghi-nv · 2022-11-08T14:40:11Z

python/cugraph/cugraph/gnn/dgl_extensions/cugraph_service_store.py

+
+        # TODO: Cant send dlpack or cupy arrays or numpys arrays
+        # through  extensions
+        # Ask Rick


Should create an issue for this and mention it here, remove "ask Rick"

Resolved with bf49417

alexbarghi-nv · 2022-11-08T14:44:33Z

python/cugraph_service/cugraph_service_client/remote_graph.py

        edge_data = self.__client.get_graph_edge_data(
-            id_or_ids=edge_ids or -1,
+            id_or_ids=ids,


Reason for this change? There is no functional change here

This throws an error when you have array like objects . I dont like or when being used with objects that can be array like.

import numpy as np np.asarray([1,2]) or -1

Traceback (most recent call last): File "main.py", line 2, in <module> np.asarray([1,2]) or -1 ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

python/cugraph/cugraph/tests/test_dgl_extension_remote_wrappers.py

…s.py Co-authored-by: Alex Barghi <[email protected]>

…Jawa/cugraph into add_remore_storage_support

VibhuJawa · 2022-11-08T17:17:05Z

@alexbarghi-nv , @rlratzel , Addressed all requested reviews, let me know.

alexbarghi-nv

👍

rlratzel · 2022-11-09T04:59:59Z

@gpucibot merge

rlratzel and others added 30 commits October 25, 2022 23:39

Fixed tests broken by refactorings, removed dead code.

2572ed0

Remove remove_pg_dependency_from_cugraph_store.py

203da90

Added tests for extension modules that access the server and return N…

b19fab1

…one, added test for extension modules returning results to client GPUs (code not done yet).

Merge remote-tracking branch 'upstream/branch-22.12' into branch-22.1…

e897caf

…2-cugraph_service_extension_enhancements

Remove remove_pg_dependency_from_cugraph_store.py

51fb38f

Move code to helper functions to make them callable from extensions

d528c0c

Fix typos

a18becd

Added copyrights

3b6ef59

Moved common stuff to RemoteStorage

4cfa293

Address Reviews

f1a3318

Added ucx-py infra for support result_device param for call_extension…

067c4e9

…(), updated tests, added test for upcoming ability to load extensions via import paths.

Added code to support loading extensions using a python module or pac…

ac59971

…kage path (eg. import foo.bar.baz), updated test.

Updated docstring for load_extensions()

b4b489e

Added Remote Storage Support

943034f

Added better error handling for APIs when using device transfer optio…

82871c3

…ns, added tests for error conditions, added benchmark for call_extension test.

Merge remote-tracking branch 'upstream/branch-22.12' into branch-22.1…

42256e9

…2-cugraph_service_extension_enhancements

Remove remove_pg_dependency_from_cugraph_store.py

b0ffa45

Remove remove_pg_dependency_from_cugraph_store.py

4df76f0

Move code to helper functions to make them callable from extensions

a941a8a

Fix typos

696fbc7

Added copyrights

8f9cfab

Moved common stuff to RemoteStorage

d73bef4

Address Reviews

e899c89

Added Remote Storage Support

206e0be

Add cugraph_service_store

0afbb9b

fix merge conflicts

508f87a

Add RemotePropertyGraph and RemoteMGPropertyGraph

8426656

Merge commit 'refs/pull/2850/head' of https://github.com/rapidsai/cug…

bca4154

…raph into add_remore_storage_support

Added working feature storage and add parquet data

5e902e8

VibhuJawa added 2 commits October 28, 2022 10:50

add_remore_storage_support

133c0d0

Add LocalStore

e91dc1f

VibhuJawa mentioned this pull request Oct 31, 2022

[FEA]Add Remote Storage support to DGL GraphStore rapidsai/dgl#17

Draft

Added working graph service

c0fab52

VibhuJawa changed the title ~~[WIP]Add remote storage support~~ [REVIEW]Add remote storage support Nov 1, 2022

VibhuJawa marked this pull request as ready for review November 1, 2022 15:37

VibhuJawa added 3 commits November 4, 2022 10:59

Merge branch 'branch-22.12' into add_remore_storage_support

e55be6d

Style Fixes

ce470bb

Skipped pytest for now

be2d7ca

BradReesWork requested review from rlratzel and alexbarghi-nv November 4, 2022 18:24

rlratzel reviewed Nov 4, 2022

View reviewed changes

VibhuJawa and others added 5 commits November 6, 2022 16:35

Update python/cugraph/cugraph/gnn/dgl_extensions/cugraph_service_stor…

55ff270

…e.py Co-authored-by: Rick Ratzel <[email protected]>

Address Reviews

56a4350

Merge branch 'add_remore_storage_support' of https://github.com/Vibhu…

5b36168

…Jawa/cugraph into add_remore_storage_support

Addresed using ntype and etype

d24ab52

fix typing issue

d7f4ace

alexbarghi-nv requested changes Nov 8, 2022

View reviewed changes

VibhuJawa and others added 4 commits November 8, 2022 08:56

Update python/cugraph/cugraph/tests/test_dgl_extension_remote_wrapper…

00795b4

…s.py Co-authored-by: Alex Barghi <[email protected]>

remove relative imports

fdd39d3

Merge branch 'add_remore_storage_support' of https://github.com/Vibhu…

35872e6

…Jawa/cugraph into add_remore_storage_support

link cugraph/issues/2863

bf49417

alexbarghi-nv approved these changes Nov 8, 2022

View reviewed changes

rlratzel approved these changes Nov 9, 2022

View reviewed changes

rapids-bot bot merged commit 4bdc242 into rapidsai:branch-22.12 Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW]Add remote storage support #2859

[REVIEW]Add remote storage support #2859

VibhuJawa commented Oct 28, 2022 •

edited

Loading

VibhuJawa commented Nov 3, 2022

codecov-commenter commented Nov 3, 2022 •

edited

Loading

rlratzel left a comment

rlratzel Nov 4, 2022

VibhuJawa Nov 7, 2022 •

edited

Loading

rlratzel Nov 4, 2022

VibhuJawa Nov 7, 2022

VibhuJawa commented Nov 7, 2022

alexbarghi-nv Nov 8, 2022

VibhuJawa Nov 8, 2022

alexbarghi-nv Nov 8, 2022

VibhuJawa Nov 8, 2022

VibhuJawa commented Nov 8, 2022

alexbarghi-nv left a comment

rlratzel commented Nov 9, 2022

	result = self.pg.get_vertex_data(
	vertex_ids=indices, columns=self.columns, types=self.types_to_fetch
	)
	else:
	result = self.pg.get_edge_data(
	edge_ids=indices, columns=self.columns, types=self.types_to_fetch

	def test_get_node_storage_ntypes():
	node_ser = cudf.Series([1, 2, 3])
	feat_ser = cudf.Series([1.0, 1.0, 1.0])
	df = cudf.DataFrame({"node_ids": node_ser, "feat": feat_ser})
	pg = PropertyGraph()
	gs = CuGraphStore(pg, backend_lib="cupy")
	gs.add_node_data(df, "node_ids", ntype="nt.a")

	node_ser = cudf.Series([4, 5, 6])
	feat_ser = cudf.Series([2.0, 2.0, 2.0])
	df = cudf.DataFrame({"node_ids": node_ser, "feat": feat_ser})
	gs.add_node_data(df, "node_ids", ntype="nt.b")

	# All indices from a single ntype
	output_ar = gs.get_node_storage(key="feat", ntype="nt.a").fetch([1, 2, 3])
	cp.testing.assert_array_equal(cp.asarray([1, 1, 1], dtype=cp.float32), output_ar)

	# Indices from other ntype are ignored
	output_ar = gs.get_node_storage(key="feat", ntype="nt.b").fetch([1, 2, 5])
	cp.testing.assert_array_equal(cp.asarray([2.0], dtype=cp.float32), output_ar)


	def test_get_edge_storage_gs(dataset1_CuGraphStore):
	etype = "('user', 'relationship', 'user')"
	fs = dataset1_CuGraphStore.get_edge_storage("relationships_k", etype)
	relationship_t = fs.fetch([6, 7, 8], device="cuda")

	relationships_df = create_df_from_dataset(
	dataset1["relationships"][0], dataset1["relationships"][1]
	)
	cudf_ar = relationships_df["relationship_type"].iloc[[0, 1, 2]].values

	assert cp.allclose(cudf_ar, relationship_t)

[REVIEW]Add remote storage support #2859

[REVIEW]Add remote storage support #2859

Conversation

VibhuJawa commented Oct 28, 2022 • edited Loading

VibhuJawa commented Nov 3, 2022

codecov-commenter commented Nov 3, 2022 • edited Loading

Codecov Report

rlratzel left a comment

Choose a reason for hiding this comment

rlratzel Nov 4, 2022

Choose a reason for hiding this comment

VibhuJawa Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

rlratzel Nov 4, 2022

Choose a reason for hiding this comment

VibhuJawa Nov 7, 2022

Choose a reason for hiding this comment

VibhuJawa commented Nov 7, 2022

alexbarghi-nv Nov 8, 2022

Choose a reason for hiding this comment

VibhuJawa Nov 8, 2022

Choose a reason for hiding this comment

alexbarghi-nv Nov 8, 2022

Choose a reason for hiding this comment

VibhuJawa Nov 8, 2022

Choose a reason for hiding this comment

VibhuJawa commented Nov 8, 2022

alexbarghi-nv left a comment

Choose a reason for hiding this comment

rlratzel commented Nov 9, 2022

VibhuJawa commented Oct 28, 2022 •

edited

Loading

codecov-commenter commented Nov 3, 2022 •

edited

Loading

VibhuJawa Nov 7, 2022 •

edited

Loading