Replace faiss bfKnn #1202

benfred · 2023-01-27T19:32:47Z

Replace faiss bfKnn with code that leverages our pairwise_distance api and select_k api - by tiling over the inputs.

This lets us remove faiss as a dependency

Closes #798
Closes #1159

Replace faiss bfKnn with code that leverages our pairwise_distance api and select_k api - by computing tiling over the inputs. This lets us remove faiss as a dependency

cjnolet

PR is looking great! I'm just giving some intial feedback so far based on a quick scan through the files. I'll do another pass tomorrow.

cpp/include/raft/spatial/knn/detail/knn_brute_force_faiss.cuh

docs/source/build.md

cjnolet · 2023-01-30T22:23:00Z

docs/source/build.md

 | RAFT_STATIC_LINK_LIBRARIES | ON, OFF | ON | Build static link libraries instead of shared libraries |
 | DETECT_CONDA_ENV | ON, OFF | ON | Enable detection of conda environment for dependencies |
 | NVTX | ON, OFF | OFF | Enable NVTX Markers |
 | CUDA_ENABLE_KERNELINFO | ON, OFF | OFF | Enables `kernelinfo` in nvcc. This is useful for `compute-sanitizer` |
 | CUDA_ENABLE_LINEINFO  | ON, OFF | OFF | Enable the -lineinfo option for nvcc |
 | CUDA_STATIC_RUNTIME | ON, OFF | OFF | Statically link the CUDA runtime |

-Currently, shared libraries are provided for the `libraft-nn` and `libraft-distance` components. The `libraft-nn` component depends upon [FAISS](https://github.com/facebookresearch/faiss) and the `RAFT_ENABLE_NN_DEPENDENCIES` option will build it from source if it is not already installed.
+Currently, shared libraries are provided for the `libraft-nn` and `libraft-distance` components.


Next step (after this PR and after the release) will be merging libraft-nn and libraft-distance into a single libraft.so. The only reason it was separated was to isolate the FAISS dependency as it's the only run-time dependency that's not in the CUDA toolkit.

codecov-commenter · 2023-02-07T07:21:36Z

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (8445aed) compared to base (bd0d86b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##           branch-23.04    #1202   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58

Impacted Files	Coverage Δ
python/pylibraft/pylibraft/cluster/__init__.py	`100.00% <100.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.

benfred · 2023-02-11T06:46:36Z

The remaining build time issues seem to be in the RBC code .

It look like

raft/cpp/include/raft/spatial/knn/detail/ball_cover.cuh

Line 185 in a572397

brute_force_knn_impl<value_int, value_idx>(handle,

and

raft/cpp/test/neighbors/ball_cover.cu

Line 120 in a572397

raft::spatial::knn::detail::brute_force_knn_impl<uint32_t, int64_t>(handle,

access the detail brute_force_knn_api, and the specializations are on the public api.

cjnolet

LGTM. I think as a follow-on we should also make a note (maybe a github issue so we don't forget?) to profile the stream pool bits w/ nsight systems just to make sure it's actually parallelizing the way we think it is. Otherwise, I think we've already discussed the other follow-ons (like augmenting this so the HDBSCAN reachability computation can use it.

cjnolet · 2023-03-15T19:37:55Z

cpp/include/raft/neighbors/detail/knn_brute_force.cuh

+          metric == raft::distance::DistanceType::LpUnexpanded) {
+        float p = 0.5;  // standard l2
+        if (metric == raft::distance::DistanceType::LpUnexpanded) p = 1.0 / metricArg;
+        raft::linalg::unaryOp<float>(


BTW- just as a note. The new raft::linalg::map primitive allows us to do this really nicely (it's now being invoked directly by unary_op, binary_op, and ternary_op).

cjnolet · 2023-03-15T19:38:24Z

cpp/include/raft/neighbors/detail/knn_brute_force.cuh

+        default:
+          // Create a new handle with the current stream from the stream pool
+          raft::device_resources stream_pool_handle(handle);
+          raft::resource::set_cuda_stream(stream_pool_handle, stream);


I'm going to add a copy overload to the handle that accepts a stream to we can make this easier.

cpp/include/raft/core/resource/cublas_handle.hpp

Try to figure out if failures are in transpose code, or in tiled_knn code

Trying to resolve stream failures here

cjnolet · 2023-03-17T17:24:59Z

/merge

This depends on #1202 so those changes are also included here. This should also not be merged until that PR is merged. Authors: - Corey J. Nolet (https://github.com/cjnolet) - Ben Frederickson (https://github.com/benfred) Approvers: - Ben Frederickson (https://github.com/benfred) - Vyas Ramasubramani (https://github.com/vyasr) - Ray Douglass (https://github.com/raydouglass) URL: #1340

…#1333) Now that we no longer need to rely on the FAISS dependency, this PR: 1. Consolidates the `libraft-distance` and `libraft-nn` targets into a single `libraft` artifact 2. Introduces a new `raft::compiled` target to link against the new `libraft.so` binary. Removes `raft::distance`and `raft::nn` targets. 3. Consolidates `RAFT_DISTANCE_COMPILED` and `RAFT_NN_COMPILED` pre-processor vars into a single `RAFT_COMPILED` (to match similar pattern implementated by `spdlog`) 4. Consolidates `specializations.cuh` headers 5. Updates all docs, scripts, and build infra This change has been a long time coming and is intended to be a 23.04 feature. This is further going to require updates to several projects downstream. Here's a checklist to track that progress: - [x] offline announcement - [x] cuml rapidsai/cuml#5272 - [x] cugraph rapidsai/cugraph#3348 - [x] cuopt rapidsai/cuopt#1023 - [x] cugraph-ops rapidsai/cugraph-ops#429 This PR depended on #1340 (removing FAISS from the build) and on #1202 (replacing the FAISS bfknn w/ our own), both of which have been merged. Closes #824 Authors: - Corey J. Nolet (https://github.com/cjnolet) - Ben Frederickson (https://github.com/benfred) Approvers: - Sevag H (https://github.com/sevagh) - Divye Gala (https://github.com/divyegala) - Ben Frederickson (https://github.com/benfred) - Vyas Ramasubramani (https://github.com/vyasr) URL: #1333

benfred added 2 commits January 27, 2023 11:03

Replace faiss bfKnn

57cfa20

Replace faiss bfKnn with code that leverages our pairwise_distance api and select_k api - by computing tiling over the inputs. This lets us remove faiss as a dependency

Merge branch 'branch-23.02' into bfknn

984c641

benfred added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 27, 2023

benfred requested review from a team as code owners January 27, 2023 19:32

benfred marked this pull request as draft January 27, 2023 19:32

github-actions bot added CMake cpp labels Jan 27, 2023

benfred and others added 6 commits January 27, 2023 11:36

fix merge

805abc7

Merge branch 'branch-23.02' into bfknn

3e21478

Fix bug with col_tiles < K

74bd44f

Merge branch 'bfknn' of github.com:benfred/raft into bfknn

c69054d

Include metric_arg in bfknn

1d9581b

speedup compile times

b4cf88c

cjnolet requested changes Jan 30, 2023

View reviewed changes

cjnolet assigned benfred Jan 30, 2023

benfred added 3 commits January 31, 2023 14:09

Merge branch 'branch-23.02' into bfknn

98ffb70

Suggestions from code review

5442d31

fixes

0f5d206

benfred changed the base branch from branch-23.02 to branch-23.04 February 1, 2023 21:13

benfred added 3 commits February 3, 2023 15:46

Merge branch 'branch-23.04' into bfknn

cb2b750

use pairwise_distance specialization to speed up compile times

e870eb3

Merge branch 'branch-23.04' into bfknn

cd84397

benfred and others added 3 commits February 7, 2023 10:26

Use distance specializations

8445aed

Merge branch 'branch-23.04' into bfknn

e87633a

Merge branch 'branch-23.04' into bfknn

52bf729

move faiss_select into raft::neighbors namespace

e8f9c55

github-actions bot added the ci label Mar 14, 2023

This was referenced Mar 14, 2023

Removing FAISS from build #1340

Merged

Consolidate pre-compiled specializations into single libraft binary #1333

Merged

benfred and others added 5 commits March 14, 2023 13:03

move knn_merge parts to its own file

9f211a0

Merge branch 'branch-23.04' into bfknn

9cbac3c

Merge remote-tracking branch 'origin/branch-23.04' into bfknn

0667526

Use stream pool

9593ae1

Merge branch 'bfknn' of github.com:benfred/raft into bfknn

76d2b19

cjnolet approved these changes Mar 15, 2023

View reviewed changes

cjnolet reviewed Mar 15, 2023

View reviewed changes

raydouglass approved these changes Mar 15, 2023

View reviewed changes

benfred added 3 commits March 15, 2023 13:39

use right handle for transpose

97753b0

set blas stream

a534538

error handling

237b7e1

cjnolet reviewed Mar 15, 2023

View reviewed changes

cpp/include/raft/core/resource/cublas_handle.hpp Outdated Show resolved Hide resolved

benfred added 8 commits March 15, 2023 20:34

try to isolate stream failure

07290bc

Try to figure out if failures are in transpose code, or in tiled_knn code

Move transpose code out of loop

2671a0e

Trying to resolve stream failures here

fix

7e0bb9b

try transpose inside streampool again

b4c3284

one more try with cublasSetStream

92d82db

Merge branch 'branch-23.04' into bfknn

c84e560

fix

21a1953

Merge branch 'bfknn' of github.com:benfred/raft into bfknn

80fb76c

rapids-bot bot merged commit 1a3c028 into rapidsai:branch-23.04 Mar 17, 2023

benfred deleted the bfknn branch March 17, 2023 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace faiss bfKnn #1202

Replace faiss bfKnn #1202

benfred commented Jan 27, 2023 •

edited by cjnolet

Loading

cjnolet left a comment

cjnolet Jan 30, 2023

codecov-commenter commented Feb 7, 2023 •

edited

Loading

benfred commented Feb 11, 2023

cjnolet left a comment

cjnolet Mar 15, 2023

cjnolet Mar 15, 2023

cjnolet commented Mar 17, 2023

Replace faiss bfKnn #1202

Replace faiss bfKnn #1202

Conversation

benfred commented Jan 27, 2023 • edited by cjnolet Loading

cjnolet left a comment

Choose a reason for hiding this comment

cjnolet Jan 30, 2023

Choose a reason for hiding this comment

codecov-commenter commented Feb 7, 2023 • edited Loading

Codecov Report

benfred commented Feb 11, 2023

cjnolet left a comment

Choose a reason for hiding this comment

cjnolet Mar 15, 2023

Choose a reason for hiding this comment

cjnolet Mar 15, 2023

Choose a reason for hiding this comment

cjnolet commented Mar 17, 2023

benfred commented Jan 27, 2023 •

edited by cjnolet

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading