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.

Sign up for GitHub

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

Jump to bottom

Add option to clone RAFT even if it is in the environment #4217

Merged
merged 15 commits into from
Sep 29, 2021
Merged

Add option to clone RAFT even if it is in the environment #4217

merged 15 commits into from
Sep 29, 2021

Conversation

dantegd
Copy link
Member

@dantegd dantegd commented Sep 21, 2021 •
edited
Loading

This PR addresses a few issues:

  • libcumlprims also depends on RAFT so it installs the RAFT headers as part of its own conda package. This made it so that CPM in CI was not cloning RAFT, which made it so that warnings as errors was not being applied to RAFT code, so code that is not covered by RAFT's own C++ tests let warnings trickle in into merged PRs.
  • This makes it so that CPM_raft_source is always used even if there is a RAFT in the conda include folders.
  • Allows users to force CPM to download RAFT even if they have a RAFT in their system already, hopefully alleviating potential folder conflicts that can happen during development

@dantegd dantegd added 2 - In Progress Currenty a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 21, 2021
@dantegd dantegd changed the title Add option to clone RAFT even if it is in the envirionment Add option to clone RAFT even if it is in the environment Sep 21, 2021
@dantegd dantegd marked this pull request as ready for review September 21, 2021 20:38
@dantegd dantegd requested review from a team as code owners September 21, 2021 20:38
@dantegd
Copy link
Member Author

dantegd commented Sep 22, 2021

rerun tests

wphicks
wphicks reviewed Sep 27, 2021
View reviewed changes
cpp/src/fil/fil.cu Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Sep 27, 2021

@gpucibot merge

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Sep 28, 2021
@dantegd dantegd requested a review from a team as a code owner September 28, 2021 02:17
@dantegd
Copy link
Member Author

dantegd commented Sep 28, 2021

rerun tests

@dantegd
Copy link
Member Author

dantegd commented Sep 28, 2021

rerun tests

@dantegd dantegd mentioned this pull request Sep 28, 2021
Merged
cjnolet
cjnolet approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM

@levsnv
Copy link
Contributor

levsnv commented Sep 29, 2021 

cuml.test.test_nearest_neighbors.test_knn_separate_index_search[braycurtis-35-3-500-dataframe]
cuml.test.test_nearest_neighbors.test_knn_separate_index_search[braycurtis-35-3-500-ndarray]
cuml.test.test_nearest_neighbors.test_knn_graph[chebyshev-35-2-5-10-dataframe-connectivity-cupy-True]
... (more test_knn_graph tests)

17:55:01 E ValueError: Expected n_neighbors <= n_samples, but n_samples = 10, n_neighbors = 35

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@8cc833f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #4217   +/-   ##
===============================================
  Coverage                ?   86.06%           
===============================================
  Files                   ?      231           
  Lines                   ?    18656           
  Branches                ?        0           
===============================================
  Hits                    ?    16057           
  Misses                  ?     2599           
  Partials                ?        0
Flag Coverage Δ
dask 47.04% <0.00%> (?)
non-dask 78.73% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out 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 8cc833f...40c95b6. Read the comment docs.

@dantegd
Copy link
Member Author

dantegd commented Sep 29, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0b5b10f into rapidsai:branch-21.10 Sep 29, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@dantegd
Add option to clone RAFT even if it is in the environment (rapidsai#4217
955b355 
)

This PR addresses a few issues:

- `libcumlprims` also depends on RAFT so it installs the RAFT headers as part of its own conda package. This made it so that CPM in CI was not cloning RAFT, which made it so that warnings as errors was not being applied to RAFT code, so code that is not covered by RAFT's own C++ tests let warnings trickle in into merged PRs.
- This makes it so that `CPM_raft_source` is always used even if there is a RAFT in the conda include folders.
- Allows users to force CPM to download RAFT even if they have a RAFT in their system already, hopefully alleviating potential folder conflicts that can happen during development

Authors:
  - Dante Gama Dessavre (https://github.com/dantegd)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai#4217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wphicks wphicks wphicks left review comments

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
2 - In Progress Currenty a work in progress CMake CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dantegd @cjnolet @levsnv @codecov-commenter @wphicks