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

Making cuco, thrust, and mdspan optional dependencies. #585

Merged
merged 198 commits into from
Apr 19, 2022
Merged

Making cuco, thrust, and mdspan optional dependencies. #585

merged 198 commits into from
Apr 19, 2022

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Mar 23, 2022 •
edited
Loading

In addition to the cuco dependency, the following changes are included:

  1. ability to turn off thrust and mdspan dependencies (rmm is still required)
  2. compiling libraries now defaults to the same setting of BUILD_TESTS (tests are still enabled)
  3. cuco dependency is disabled by default (unless distance component is enabled)
  4. the headers which are safe to expose in public APIs are moved over to core/ directory.

cjnolet added 30 commits March 1, 2022 20:02
@cjnolet
Adding pylibraft
60e7c2a
@cjnolet
Adding distance
54cac54
@cjnolet
More infrastructure stuff
f1b502a
@cjnolet
Updating year
555433b
@cjnolet
Adding missing copyright headers
4b448f5
@cjnolet
More style
f9aa8bc
@cjnolet
Proper formatting
9c72b7a
@cjnolet
Updating setup
2c278ab
@cjnolet
Rename
c021445
@cjnolet
Adding rmm dependency
8de62fe
@cjnolet
Updating gitignore
6e5499f
@cjnolet
Updates
7fc834e
@cjnolet
adding raft_runtime public includes
401c167
@cjnolet
Toy example
cb40df9
@cjnolet
Making raft::runtime the core dependency and having all other targets
a5cea8e 
link to it. Using raft::raft only as internal (build) target
@cjnolet
fixing style
4322d0b
@cjnolet
Almost there...
8ae80ad
@cjnolet
COpyright year
4bfde36
@cjnolet
Updating mdspan copyright
6306b7b
@cjnolet
Fixing doxygen
b06f8e6
@cjnolet
CHanging conda package to libraft-runtime so libraft-headers won't be
f4387b6 
overwritten (this allows downstream projects to continue to use it until
they update
@cjnolet
Update copyrightys
6a92f19
@cjnolet
MOving runtime headers into corresponding directories
831fe81
@cjnolet
Updating style
0fc02ca
@cjnolet
Removing todos
e8ba5a1
@cjnolet
updating readme
4a13200
@cjnolet
Adding docs for the raft_export INSTALL_FILES option
8583386
@cjnolet
Updating build.md
d518ba7
@cjnolet
Merge branch 'branch-22.04' into fea-2204-pylibraft
00e8a69
@cjnolet
Merge branch 'fea-2204-raft_runtime' into fea-2204-pylibraft
54df7de
@robertmaynard
Copy link
Contributor

To build raft by default I had to disable the following tests since they pull in faiss headers, which isn't enabled by default.

  • test/sparse/add.cu
  • test/sparse/convert_coo.cu
  • test/sparse/convert_csr.cu
  • test/sparse/connect_components.cu
  • test/sparse/csr_row_slice.cu
  • test/sparse/csr_to_dense.cu
  • test/sparse/csr_transpose.cu
  • test/sparse/degree.cu
  • test/sparse/dist_coo_spmv.cu
  • test/sparse/distance.cu
  • test/sparse/filter.cu
  • test/sparse/knn.cu
  • test/sparse/knn_graph.cu
  • test/sparse/linkage.cu
  • test/sparse/norm.cu
  • test/sparse/reduce.cu
  • test/sparse/row_op.cu
  • test/sparse/sort.cu
  • test/spatial/knn.cu
  • test/spatial/fused_l2_knn.cu
  • test/spatial/haversine.cu
  • test/spatial/ball_cover.cu
  • test/spatial/epsilon_neighborhood.cu
  • test/spatial/faiss_mr.cu
  • test/spatial/selection.cu
  • test/stats/trustworthiness.cu

@cjnolet
Copy link
Member Author

cjnolet commented Mar 31, 2022

To build raft by default I had to disable the following tests since they pull in faiss headers, which isn't enabled by default.

@robertmaynard most of those files don't include faiss headers, but I do get your point that the tests shouldn't be getting built without FAISS if that's a requirement.

@robertmaynard
Copy link
Contributor

To build raft by default I had to disable the following tests since they pull in faiss headers, which isn't enabled by default.

@robertmaynard most of those files don't include faiss headers, but I do get your point that the tests shouldn't be getting built without FAISS if that's a requirement.

You are correct I over removed tests, I don't know how I captured too many. But here is the full breakdown of what includes faiss / cuco headers.

Requires FAISS directly:

  • test/spatial/fused_l2_knn.cu
  • test/spatial/faiss_mr.cu

Requires FAISS via knn/detail/knn_brute_force_faiss.cuh:

  • test/sparse/knn_graph.cu
  • test/sparse/linkage.cu
  • test/sparse/connect_components.cu
  • test/sparse/csr_transpose.cu
  • test/spatial/knn.cu
  • test/spatial/epsilon_neighborhood.cu
  • test/stats/selection.cu
  • test/stats/trustworthiness.cu

Requires FAISS via haversine_distance.cuh:

  • test/spatial/haversine.cu
  • test/spatial/ball_cover.cu

Requires cuco:

  • test/sparse/dist_coo_spmv.cu
  • test/sparse/distance.cu
  • test/sparse/knn.cu

@cjnolet
Copy link
Member Author

cjnolet commented Apr 1, 2022

rerun tests

robertmaynard
robertmaynard reviewed Apr 1, 2022
View reviewed changes
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member Author

cjnolet commented Apr 2, 2022

rerun tests

divyegala
divyegala reviewed Apr 5, 2022
View reviewed changes
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Some small comments. Should we also have a standard namespace convention for headers in raft/core? There's some variation amongst them at this moment

cpp/include/raft/common/logger.hpp Show resolved Hide resolved
cpp/include/raft/common/nvtx.hpp Show resolved Hide resolved
cpp/include/raft/core/nvtx.hpp Show resolved Hide resolved
docs/source/cuda_cpp.rst Outdated Show resolved Hide resolved
rlratzel
rlratzel reviewed Apr 5, 2022
View reviewed changes
build.sh Show resolved Hide resolved
conda/recipes/libraft_headers/build.sh Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_mdspan.cmake Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_thrust.cmake Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member Author

cjnolet commented Apr 19, 2022

rerun tests

@cjnolet
Copy link
Member Author

cjnolet commented Apr 19, 2022

@gpucibot merge

msadang
msadang approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@msadang msadang left a comment

Choose a reason for hiding this comment

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

Lgtm

@rapids-bot rapids-bot bot merged commit d0601af into rapidsai:branch-22.06 Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divyegala divyegala divyegala left review comments

@rlratzel rlratzel rlratzel left review comments

@msadang msadang msadang approved these changes

@dantegd dantegd dantegd approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

Assignees

@cjnolet cjnolet

Labels
CMake cpp 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.
6 participants
@cjnolet @robertmaynard @dantegd @rlratzel @divyegala @msadang