Consolidate pre-compiled specializations into single libraft binary
#1333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
This reverts commit 8eaba84. Change didn't seem to build in CI
Also remove metrics processors - since is handled inside PW distance
This reverts commit fe728e9. This is causing incorrect results, just use the faiss select_k call instead
divyegala
reviewed
Mar 18, 2023
cjnolet
added
breaking
sevagh
approved these changes
Mar 20, 2023
divyegala
approved these changes
Mar 20, 2023
ajschmidt8
added a commit
to ajschmidt8/docker
that referenced
this pull request
Mar 20, 2023
Due to the changes in rapidsai/raft#1333, the `libraft` build command in our `devel` containers need to be updated.
benfred
approved these changes
Mar 20, 2023
vyasr
approved these changes
Mar 20, 2023
There was a problem hiding this comment.
I get the sense that I'm missing some context for the reorganization of the template specialization headers, probably because there were dependencies when using FAISS that have now changed without our usage of FAISS, or possibly because they're now shared between the former nn and dist lib files thanks to them being combined. Assuming all of that was done correctly, the build-related changes look correct to me. Some minor questions that aren't worth blocking over.
cpp/src/neighbors/specializations/detail/ball_cover_lowdim_pass_two_3d.cu
Show resolved
Hide resolved
|
/merge |
rapids-bot bot
pushed a commit
to rapidsai/cugraph
that referenced
this pull request
Mar 21, 2023
Pleae note, this should not be merged until rapidsai/raft#1333 is ready to be merged. Authors: - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Chuck Hastings (https://github.com/ChuckHastings) - Sevag H (https://github.com/sevagh) URL: #3348
rapids-bot bot
pushed a commit
to rapidsai/cuml
that referenced
this pull request
Mar 23, 2023
This is intended to be merged in 23.06 w/ this PR: rapidsai/raft#1333. Authors: - Corey J. Nolet (https://github.com/cjnolet) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) - AJ Schmidt (https://github.com/ajschmidt8) URL: #5272
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that we no longer need to rely on the FAISS dependency, this PR:
libraft-distanceandlibraft-nntargets into a singlelibraftartifactraft::compiledtarget to link against the newlibraft.sobinary. Removesraft::distanceandraft::nntargets.RAFT_DISTANCE_COMPILEDandRAFT_NN_COMPILEDpre-processor vars into a singleRAFT_COMPILED(to match similar pattern implementated byspdlog)specializations.cuhheadersThis 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:
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