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.

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

Adds parameterized benchmarks for uniform_neighbor_sampling, updates benchmarks dir for future additions #3048

Merged

Conversation

rlratzel
Copy link
Contributor

@rlratzel rlratzel commented Dec 6, 2022

closes #3034

This PR updates the top-level benchmarks dir to better organize benchmarks (and future benchmarks) based on package name. Additional parameterized benchmarks for uniform_neighbor_sampling were added as well as re-usable fixtures and pytest param objects and utilities.

Other updates include:

  • Bug fix for MG graphs related to invalid internal column names after renumbering
  • Removed redundant genFixteurParamsProduct and refactored so only one copy is used for both cugraph and PLC
  • Cleaned up some import statements

The "breaking" label is used because the benchmarks dir re-org changes the location of existing benchmarks, which could break scripts that call them.

…est to use specific set of input param combinations.
…fo(), added tests to verify new meta-data is correct.
… with the server, added tests which use/test the new builtin test extensions.
…s (via rapids-pytest-benchmark plugin) easier to read, changes to allow RemoteGraph to use either a server-side Graph or PropertyGraph.
…r LocalCUDACluster for MG service, re-wrote start list generation to work with both SG and MG, added server extension for generating start list, changed scale from 24 to 23 to prevent OOM, added separate function for creating MG graphs, added debug print to show result sizes, fixes to cugraph MG Graph for incorrect column names, added util to cugraph for starting and stopping dask client.
… can be more easily shared, bug fix in benchmark extension testing util.
…arkers to pytest.ini, added pythonpath and test paths to pytest.ini, gave benchmark files unique names so pytest can collect them across different bench dirs.
…ception handler for loading extensions, run server from a temp dir to avoid collisions with 'cugraph' namespace package in benchmarks dir.
@rlratzel rlratzel added improvement Improvement / enhancement to an existing function breaking Breaking change labels Dec 6, 2022
@rlratzel rlratzel added this to the 23.02 milestone Dec 6, 2022
@rlratzel rlratzel requested a review from a team as a code owner December 6, 2022 07:48
@rlratzel rlratzel self-assigned this Dec 6, 2022
@rlratzel rlratzel requested review from a team as code owners December 6, 2022 07:48
Copy link
Member

@alexbarghi-nv alexbarghi-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to looking this over, I was able to use the code in the PR to run cugraph-pyg benchmarks, so it is all good. 👍

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

Base: 58.86% // Head: 58.50% // Decreases project coverage by -0.36% ⚠️

Coverage data is based on head (86cd55d) compared to base (2ae4b61).
Patch coverage: 31.81% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-23.02    #3048      +/-   ##
================================================
- Coverage         58.86%   58.50%   -0.37%     
================================================
  Files               131      133       +2     
  Lines              7848     7895      +47     
================================================
- Hits               4620     4619       -1     
- Misses             3228     3276      +48     
Impacted Files Coverage Δ
...ure/graph_implementation/simpleDistributedGraph.py 14.45% <0.00%> (-0.45%) ⬇️
python/cugraph/cugraph/testing/utils.py 71.42% <ø> (-4.19%) ⬇️
python/pylibcugraph/pylibcugraph/testing/utils.py 17.77% <7.14%> (-12.66%) ⬇️
python/cugraph/cugraph/testing/mg_utils.py 31.03% <31.03%> (ø)
...ugraph/cugraph/dask/structure/mg_property_graph.py 12.14% <33.33%> (+0.05%) ⬆️
python/cugraph/cugraph/structure/property_graph.py 94.95% <100.00%> (+0.02%) ⬆️
python/cugraph/cugraph/utilities/utils.py 71.71% <100.00%> (+1.34%) ⬆️
...thon/pylibcugraph/pylibcugraph/testing/__init__.py 100.00% <100.00%> (ø)

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.

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 a clarification based onMultiGraph, everything else looks good.

graph_props = pylibcugraph.experimental.GraphProperties(
is_symmetric=False, is_multigraph=False)
st = time.time()
G = pylibcugraph.experimental.SGGraph(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
G = pylibcugraph.experimental.SGGraph(
G = pylibcugraph.experimental.MGGraph(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to stay SGGraph since we're not incorporating dask/MPI/etc. anywhere to distribute the data to separate MGGraphs.

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.

Small additon

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.

LGTM

@rlratzel
Copy link
Contributor Author

rerun tests

reason: PG test failures should be fixed from this merged PR.

@rlratzel
Copy link
Contributor Author

from @BradReesWork offline:

FIXME - doc strings should be better
edgelist_df[“src”] - really need those globals/defines for “src” andf “dst” to avoid typos

will address those when a PR for this branch is pushed next.

@rlratzel
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a52dd57 into rapidsai:branch-23.02 Dec 16, 2022
@rlratzel rlratzel deleted the branch-23.02-initial_benchmark_reorg branch September 28, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add benchmarks for uniform_neighbor_sampling for large-scale MG runs
6 participants