[REVIEW] Optimize pg.get_x_data APIs #3086

VibhuJawa · 2022-12-15T23:46:29Z

This PR closes #3026 .
The speed up on cupy arrays is 83x.

mainline:
----------------------- benchmark: 1 tests -----------------------
Name (time in s, mem in bytes)                        Mean  Rounds
------------------------------------------------------------------
bench_get_vector_features_cp_array[128-1000000]     1.0880       1
------------------------------------------------------------------


branch: 
------------------------ benchmark: 1 tests -----------------------
Name (time in ms, mem in bytes)                        Mean  Rounds
-------------------------------------------------------------------
bench_get_vector_features_cp_array[128-1000000]     12.7905       1
-------------------------------------------------------------------

branch: 
------------------------- benchmark: 1 tests -------------------------
Name (time in ms, mem in bytes)                           Mean  Rounds
----------------------------------------------------------------------
bench_get_vector_features_cudf_series[128-1000000]     12.7068       1
----------------------------------------------------------------------


Mainline: 
------------------------- benchmark: 1 tests -------------------------
Name (time in ms, mem in bytes)                           Mean  Rounds
----------------------------------------------------------------------
bench_get_vector_features_cudf_series[128-1000000]     12.6223       1
----------------------------------------------------------------------

python/cugraph/cugraph/dask/structure/mg_property_graph.py

VibhuJawa · 2022-12-15T23:48:53Z

python/cugraph/cugraph/tests/mg/test_mg_property_graph.py

+        assert (
+            df["_VERTEX_"] == df["_VERTEX_"]._constructor(range(start, stop + 1))
+        ).all()


These changes are similar to https://github.com/rapidsai/cugraph/pull/3036/files#r1048542911 .

Unsure how CI passed without them

VibhuJawa · 2022-12-15T23:49:04Z

python/cugraph/cugraph/tests/mg/test_mg_property_graph.py

+            expected = cur._constructor(sorted(x for x, *args in data[key][1]))
+            assert (cur.values == expected.values).all()


These changes are similar to https://github.com/rapidsai/cugraph/pull/3036/files#r1048542911 .

I made similar changes in my PR. We have to be more careful running MG tests until they are included in CI.

VibhuJawa · 2022-12-15T23:49:12Z

python/cugraph/cugraph/tests/mg/test_mg_property_graph.py

+        actual = df[pG.edge_id_col_name]
+        expected = actual._constructor(range(start, stop + 1))
+        assert (actual == expected).all()


These changes are similar to https://github.com/rapidsai/cugraph/pull/3036/files#r1048542911 .

codecov-commenter · 2022-12-16T02:15:06Z

Codecov Report

Base: 58.50% // Head: 57.17% // Decreases project coverage by -1.33% ⚠️

Coverage data is based on head (b7814d3) compared to base (fd8d764).
Patch coverage: 25.00% of modified lines in pull request are covered.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.02    #3086      +/-   ##
================================================
- Coverage         58.50%   57.17%   -1.34%     
================================================
  Files               133      148      +15     
  Lines              7895     9295    +1400     
================================================
+ Hits               4619     5314     +695     
- Misses             3276     3981     +705

Impacted Files	Coverage Δ
...ugraph/cugraph/dask/structure/mg_property_graph.py	`11.97% <0.00%> (-0.18%)`	⬇️
python/cugraph/cugraph/structure/property_graph.py	`94.67% <50.00%> (-0.29%)`	⬇️
...-service/server/cugraph_service_server/__init__.py	`25.00% <0.00%> (ø)`
...aph-service/client/cugraph_service_client/types.py	`74.69% <0.00%> (ø)`
...t/cugraph_service_client/cugraph_service_thrift.py	`68.18% <0.00%> (ø)`
...-service/server/cugraph_service_server/__main__.py	`0.00% <0.00%> (ø)`
...rvice_server/testing/benchmark_server_extension.py	`40.38% <0.00%> (ø)`
...-service/client/cugraph_service_client/__init__.py	`100.00% <0.00%> (ø)`
...-service/client/cugraph_service_client/defaults.py	`100.00% <0.00%> (ø)`
...lient/cugraph_service_client/remote_graph_utils.py	`20.00% <0.00%> (ø)`
... and 10 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.

…m/VibhuJawa/cugraph into optimize_gather_vector_properties

rlratzel

LGTM, but I did have one suggestion.

python/cugraph/cugraph/dask/structure/mg_property_graph.py

Co-authored-by: Rick Ratzel <[email protected]>

alexbarghi-nv · 2022-12-16T22:48:39Z

@VibhuJawa could you clarify - is cudf.Series the preferred datatype to pass in here? So cupy arrays are discouraged?

VibhuJawa · 2022-12-16T23:58:31Z

@VibhuJawa could you clarify - is cudf.Series the preferred datatype to pass in here? So cupy arrays are discouraged?

Nope, You can use anything you want after this PR . Before this PR cupy arrays on SG bonked on SG and MG . I think even cudf.series was a problem for MG previously but i might be wrong

VibhuJawa · 2022-12-19T23:16:00Z

rerun tests

Reason:
The previous failure was because of cuDF issues.

12:20:19 Encountered problems while solving:
12:20:19   - nothing provides libcudf 23.02.00a221216.* needed by cudf-23.02.00a221216-cuda_11_py39_g5e357b4f49_188
12:20:19   - nothing provides libcudf 23.02.00a221216.* needed by cudf-23.02.00a221216-cuda_11_py39_g5e357b4f49_188

VibhuJawa · 2022-12-20T14:33:58Z

@rlratzel / @alexbarghi-nv , Can we merge this in now ? I think all changes have been addressed and CI passes.

alexbarghi-nv · 2022-12-20T14:35:41Z

@rlratzel / @alexbarghi-nv , Can we merge this in now ? I think all changes have been addressed and CI passes.

I've approved, but don't have merge privileges.

alexbarghi-nv · 2023-01-03T15:38:10Z

@gpucibot merge

added benchmarks

769e281

VibhuJawa commented Dec 15, 2022

View reviewed changes

VibhuJawa added non-breaking Non-breaking change Fix labels Dec 15, 2022

VibhuJawa added the improvement Improvement / enhancement to an existing function label Dec 16, 2022

VibhuJawa added 3 commits December 16, 2022 18:41

Merge branch 'branch-23.02' into optimize_gather_vector_properties

9ab9d08

Remove comments

0949d07

Merge branch 'optimize_gather_vector_properties' of https://github.co…

5ea50dc

…m/VibhuJawa/cugraph into optimize_gather_vector_properties

VibhuJawa marked this pull request as ready for review December 16, 2022 14:17

VibhuJawa requested a review from a team as a code owner December 16, 2022 14:17

VibhuJawa changed the title ~~[WIP] Optimize pg.get_x_data APIs~~ [REVIEW] Optimize pg.get_x_data APIs Dec 16, 2022

VibhuJawa changed the title ~~[REVIEW] Optimize pg.get_x_data APIs~~ [WIP] Optimize pg.get_x_data APIs Dec 16, 2022

Update cugraph features

6040a29

VibhuJawa changed the title ~~[WIP] Optimize pg.get_x_data APIs~~ [REVIEW] Optimize pg.get_x_data APIs Dec 16, 2022

BradReesWork requested review from rlratzel and alexbarghi-nv December 16, 2022 17:38

rlratzel reviewed Dec 16, 2022

View reviewed changes

python/cugraph/cugraph/dask/structure/mg_property_graph.py Outdated Show resolved Hide resolved

VibhuJawa and others added 3 commits December 16, 2022 10:54

Update python/cugraph/cugraph/dask/structure/mg_property_graph.py

c6408f5

Co-authored-by: Rick Ratzel <[email protected]>

optimize_gather_vector_properties

63d5177

Update mg_property_graph.py

dcbb7cf

BradReesWork added this to the 23.02 milestone Dec 16, 2022

alexbarghi-nv approved these changes Dec 19, 2022

View reviewed changes

VibhuJawa added 2 commits December 28, 2022 03:25

Merge branch 'branch-23.02' into optimize_gather_vector_properties

6d001c8

Merge branch 'branch-23.02' into optimize_gather_vector_properties

b7814d3

rapids-bot bot merged commit bc2e130 into rapidsai:branch-23.02 Jan 3, 2023

rlratzel approved these changes Jan 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Optimize pg.get_x_data APIs #3086

[REVIEW] Optimize pg.get_x_data APIs #3086

VibhuJawa commented Dec 15, 2022 •

edited

Loading

VibhuJawa Dec 15, 2022

VibhuJawa Dec 15, 2022

alexbarghi-nv Dec 16, 2022

VibhuJawa Dec 15, 2022

codecov-commenter commented Dec 16, 2022 •

edited

Loading

rlratzel left a comment

alexbarghi-nv commented Dec 16, 2022

VibhuJawa commented Dec 16, 2022

VibhuJawa commented Dec 19, 2022

VibhuJawa commented Dec 20, 2022

alexbarghi-nv commented Dec 20, 2022

alexbarghi-nv commented Jan 3, 2023

		expected = cur._constructor(sorted(x for x, *args in data[key][1]))
		assert (cur.values == expected.values).all()

[REVIEW] Optimize pg.get_x_data APIs #3086

[REVIEW] Optimize pg.get_x_data APIs #3086

Conversation

VibhuJawa commented Dec 15, 2022 • edited Loading

VibhuJawa Dec 15, 2022

Choose a reason for hiding this comment

VibhuJawa Dec 15, 2022

Choose a reason for hiding this comment

alexbarghi-nv Dec 16, 2022

Choose a reason for hiding this comment

VibhuJawa Dec 15, 2022

Choose a reason for hiding this comment

codecov-commenter commented Dec 16, 2022 • edited Loading

Codecov Report

rlratzel left a comment

Choose a reason for hiding this comment

alexbarghi-nv commented Dec 16, 2022

VibhuJawa commented Dec 16, 2022

VibhuJawa commented Dec 19, 2022

VibhuJawa commented Dec 20, 2022

alexbarghi-nv commented Dec 20, 2022

alexbarghi-nv commented Jan 3, 2023

VibhuJawa commented Dec 15, 2022 •

edited

Loading

codecov-commenter commented Dec 16, 2022 •

edited

Loading