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

suppress expansion of unused raft spectral templates #2739

Merged
merged 12 commits into from
Sep 27, 2022

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Sep 27, 2022

Closes #2678

raft updated some k-means logic and dramatically increased the compile time of our spectral clustering implementation. This PR adds an include that will suppress the expansion of templates that we are not using.

@cjnolet cjnolet requested review from a team as code owners September 27, 2022 13:23
@cjnolet cjnolet changed the title Trying a couple things for Chuck's RAFT templates branch [DO NOT MERGE] Trying a couple things for Chuck's RAFT templates branch Sep 27, 2022
@BradReesWork BradReesWork added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 27, 2022
@BradReesWork
Copy link
Member

added labels just so CI will run

@ChuckHastings ChuckHastings changed the title [DO NOT MERGE] Trying a couple things for Chuck's RAFT templates branch suppress expansion of unused raft spectral templates Sep 27, 2022
@ChuckHastings ChuckHastings added this to the 22.10 milestone Sep 27, 2022
@codecov-commenter
Copy link

Codecov Report

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

❗ Current head 4c2132d differs from pull request most recent head ce916dc. Consider uploading reports for the commit ce916dc to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10    #2739   +/-   ##
===============================================
  Coverage                ?   60.04%           
===============================================
  Files                   ?      111           
  Lines                   ?     6184           
  Branches                ?        0           
===============================================
  Hits                    ?     3713           
  Misses                  ?     2471           
  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.

@ChuckHastings
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 35337cf into rapidsai:branch-22.10 Sep 27, 2022
Comment on lines +50 to +53
# We need to call get_raft due to cuML asking for raft::nn and
# raft::distance targets
# see issue https://github.com/rapidsai/cuml/issues/4843
include(../../cpp/cmake/thirdparty/get_raft.cmake)
Copy link
Contributor

Choose a reason for hiding this comment

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

@cjnolet good find. The underlying issue here will be fixed by rapidsai/rapids-cmake#154.

@vyasr
Copy link
Contributor

vyasr commented Sep 27, 2022

For future reference the issue this PR fixes that caused #2707 to fail is that rapids-cmake's export commands don't support COMPONENTS, so the distance component requirement of libcugraph isn't being propagated to the pylibcugraph and cugraph packages. Once rapidsai/rapids-cmake#154 is merged we should be able to get rid of the extra get_raft.cmake includes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spectral_clustering build is suspiciously slow and contributes to CI build timeouts
7 participants