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

[REVIEW] BUG Move subcomms init outside of individual algorithm functions #1196

Merged

Conversation

rlratzel
Copy link
Contributor

@rlratzel rlratzel commented Oct 7, 2020

This PR moves the subcomms init outside of the individual algorithm functions and up to the function a user calls to initialize comms. This allows the same subcomm configuration to be used across algorithms and also allows users to customize the call to optimize based on GPU configuration.

Without this, subcomms will be repeatedly created/initialized for each algo call, resulting in problems.

closes #1065

(thanks to @Iroy30 for commits 71d4a0b and 79f131c)

@rlratzel rlratzel requested review from a team as code owners October 7, 2020 16:19
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@rlratzel rlratzel changed the title [WIP][skip-ci] BUG Move subcomms init outside of individual algorithm functions [REVIEW] BUG Move subcomms init outside of individual algorithm functions Oct 7, 2020
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

Besides the comment about documentation, this looks good to me. Thank you for the quick fix!

python/cugraph/comms/comms.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

Much cleaner!

…on of get_n_workers() call, but still need FIXME for that addressed.
@BradReesWork BradReesWork added this to the 0.16 milestone Oct 8, 2020
@rlratzel
Copy link
Contributor Author

rlratzel commented Oct 9, 2020

rerun tests

@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

❗ No coverage uploaded for pull request base (branch-0.16@ae5d0f2). Click here to learn what that means.
The diff coverage is 27.08%.

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.16    #1196   +/-   ##
==============================================
  Coverage               ?   57.28%           
==============================================
  Files                  ?       61           
  Lines                  ?     2500           
  Branches               ?        0           
==============================================
  Hits                   ?     1432           
  Misses                 ?     1068           
  Partials               ?        0           
Impacted Files Coverage Δ
python/cugraph/dask/community/louvain.py 31.81% <ø> (ø)
python/cugraph/comms/comms.py 35.36% <25.00%> (ø)
python/cugraph/structure/shuffle.py 10.86% <50.00%> (ø)
python/cugraph/community/subgraph_extraction.py 95.00% <0.00%> (ø)
python/cugraph/utilities/nx_factory.py 91.66% <0.00%> (ø)
python/cugraph/link_analysis/__init__.py 100.00% <0.00%> (ø)
python/cugraph/components/__init__.py 100.00% <0.00%> (ø)
python/cugraph/dask/community/__init__.py 100.00% <0.00%> (ø)
python/cugraph/bsp/traversal/__init__.py 100.00% <0.00%> (ø)
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae5d0f2...69051f6. Read the comment docs.

@BradReesWork BradReesWork merged commit b14c458 into rapidsai:branch-0.16 Oct 9, 2020
@rlratzel rlratzel deleted the branch-0.16-movesubcomminit branch December 9, 2020 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] sub-communicator initialization for 2D partitioning support
8 participants