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

Remove Raft deprecated headers #4858

Merged
merged 20 commits into from Sep 8, 2022

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Aug 8, 2022

This should be merged before the Raft PR: rapidsai/raft#753.

cpp/cmake/thirdparty/get_cumlprims_mg.cmake Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_raft.cmake Outdated Show resolved Hide resolved
@lowener lowener marked this pull request as ready for review August 28, 2022 10:17
@lowener lowener requested review from a team as code owners August 28, 2022 10:17
@lowener
Copy link
Contributor Author

lowener commented Aug 28, 2022

rerun tests

@lowener lowener added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 29, 2022
@caryr35 caryr35 added this to PR-WIP in v22.10 Release via automation Aug 29, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v22.10 Release Aug 29, 2022
@dantegd
Copy link
Member

dantegd commented Aug 30, 2022

rerun tests

@github-actions github-actions bot removed the CMake label Aug 31, 2022
@lowener
Copy link
Contributor Author

lowener commented Sep 5, 2022

rerun tests

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.

@lowener your changes look great so far. I ended up going through these files in bulk, grepping the include lines of the files to verify the proper use of hpp and cuh everywhere. I did find a couple files that are still using the incorrect includes (from a recent PR). I'm posing them here since the files aren't actually included in your PR:

These should be using cuh:

cjnolet@gpudev /share/workspace/rapids_projects/cuml ((HEAD detached at lowener/22.10-raft-deprecated)) $ grep -R "#include <raft.*linalg.*hpp" cpp/src
cpp/src/hdbscan/detail/soft_clustering.cuh:#include <raft/linalg/matrix_vector_op.hpp>
cpp/src/hdbscan/detail/soft_clustering.cuh:#include <raft/linalg/norm.hpp>

cjnolet@gpudev /share/workspace/rapids_projects/cuml ((HEAD detached at lowener/22.10-raft-deprecated)) $ grep -R "#include <raft.*csr.*hpp" cpp/src
cpp/src/hdbscan/detail/soft_clustering.cuh:#include <raft/sparse/convert/csr.hpp>

cjnolet@gpudev /share/workspace/rapids_projects/cuml ((HEAD detached at lowener/22.10-raft-deprecated)) $ grep -R "#include <raft.*label.*hpp" cpp/src
cpp/src/hdbscan/condensed_hierarchy.cu:#include <raft/label/classlabels.hpp>
cpp/src/hdbscan/detail/membership.cuh:#include <raft/label/classlabels.hpp>
cpp/src/hdbscan/detail/select.cuh:#include <raft/label/classlabels.hpp>
cpp/src/hdbscan/detail/extract.cuh:#include <raft/label/classlabels.hpp>
cpp/src/hdbscan/detail/soft_clustering.cuh:#include <raft/label/classlabels.hpp>
cpp/src/hdbscan/detail/stabilities.cuh:#include <raft/label/classlabels.hpp>
cpp/src/svm/svc_impl.cuh:#include <raft/label/classlabels.hpp>

cjnolet@gpudev /share/workspace/rapids_projects/cuml ((HEAD detached at lowener/22.10-raft-deprecated)) $ grep -R "#include <raft.*matrix.*hpp" cpp/src
cpp/src/hdbscan/detail/soft_clustering.cuh:#include <raft/linalg/matrix_vector_op.hpp>
cpp/src/hdbscan/detail/soft_clustering.cuh:#include <raft/matrix/math.hpp>

All of the includes from core/ look correct.

@codecov-commenter
Copy link

Codecov Report

Base: 78.02% // Head: 78.07% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (3262230) compared to base (7a0ab85).
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.10    #4858      +/-   ##
================================================
+ Coverage         78.02%   78.07%   +0.05%     
================================================
  Files               180      180              
  Lines             11385    11443      +58     
================================================
+ Hits               8883     8934      +51     
- Misses             2502     2509       +7     
Flag Coverage Δ
dask 46.23% <ø> (+0.01%) ⬆️
non-dask 67.37% <ø> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/common/array.py 95.10% <0.00%> (-2.88%) ⬇️
python/cuml/cluster/__init__.py 100.00% <0.00%> (ø)
python/cuml/metrics/__init__.py 100.00% <0.00%> (ø)
python/cuml/thirdparty_adapters/adapters.py 91.54% <0.00%> (+0.05%) ⬆️
python/cuml/preprocessing/TargetEncoder.py 85.07% <0.00%> (+1.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

@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.

I looked through all the files and didn't see anything further. I also grepped through the files again and didn't see any other missed files. I did notice one very tiny typo, which wasn't the fault of these changes but we could probably fix it while we're already making changes to the offending file. I'm still approving, though, since it's so minor.

wiki/cpp/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
v22.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Sep 8, 2022
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Reviewed the first chunk. Looks good!

@cjnolet
Copy link
Member

cjnolet commented Sep 8, 2022

@gpucibot merge

@cjnolet
Copy link
Member

cjnolet commented Sep 8, 2022

rerun tests

@rapids-bot rapids-bot bot merged commit 6f42ced into rapidsai:branch-22.10 Sep 8, 2022
v22.10 Release automation moved this from PR-Reviewer approved to Done Sep 8, 2022
@lowener lowener deleted the 22.10-raft-deprecated branch September 9, 2022 13:06
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
This should be merged before the Raft PR: rapidsai/raft#753.

Authors:
  - Micka (https://github.com/lowener)

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

URL: rapidsai#4858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CUDA/C++ improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants