-
Notifications
You must be signed in to change notification settings - Fork 194
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
Simplify raft component CMake logic, and allow compilation without FAISS #428
Merged
rapids-bot
merged 86 commits into
rapidsai:branch-22.02
from
robertmaynard:refactor_cmake_raft_target_logic
Jan 23, 2022
Merged
Changes from all commits
Commits
Show all changes
86 commits
Select commit
Hold shift + click to select a range
07644ec
add option to build faiss shared libs
trxcllnt f3e1f09
add FAISS::FAISS to the list of global targets
trxcllnt e038644
update cuco
trxcllnt 3fa9c94
create a faissTargets export-set
trxcllnt fc132ef
Merge branch 'fix/build-shared-faiss' into fix/node-rapids-21.10
trxcllnt 85572d0
add faiss to raft-exports
trxcllnt b19a4b6
revert
trxcllnt 6c74570
Merge branch 'fix/build-shared-faiss' into fix/node-rapids-21.10
trxcllnt 14785ff
create a faiss-exports export-set
trxcllnt 25b62e6
mark faiss to be found as part of resolving raft-exports dependencies
trxcllnt 76edf36
create faiss-exports export set
trxcllnt 92851b3
create faiss-exports export set
trxcllnt 8e266ef
export faiss::faiss target to build export set
trxcllnt 9d535fb
Merge branch 'fix/node-rapids-21.10' into fix/build-shared-faiss
trxcllnt 7c990b1
Merge branch 'branch-21.10' of github.com:rapidsai/raft into fix/buil…
trxcllnt 3a20c64
Merge branch 'branch-21.12' of github.com:rapidsai/raft into fix/buil…
trxcllnt 83e3aa6
fix faiss GLOBAL_TARGETS
trxcllnt 2773edc
update rapids-cmake version
trxcllnt 3ff1c4d
fix naming for generated Findfaiss.cmake module so we find conda-inst…
trxcllnt cbb6f2d
ensure faiss::faiss target is available in build and install export sets
trxcllnt 713a872
add gtest to raft-exports, remove dead code
trxcllnt ceb4d8b
update CMake faiss variable name
trxcllnt 1d3ffab
make -v set cmake --log-level=VERBOSE
trxcllnt b48e393
update cuco hash to the commit that uses rapids-cmake v21.12
trxcllnt dc076ae
Merge branch 'branch-21.12' of github.com:rapidsai/raft into fix/buil…
trxcllnt e824384
Merge branch 'branch-21.12' of github.com:rapidsai/raft into fix/buil…
trxcllnt 078e33c
Merge branch 'branch-21.12' of github.com:rapidsai/raft into fix/buil…
trxcllnt 30dafb6
Merge branch 'branch-22.02' of github.com:rapidsai/raft into fix/buil…
trxcllnt aedc6d7
update cuco version
trxcllnt 245ea3d
Merge branch 'branch-22.02' of github.com:rapidsai/raft into fix/buil…
trxcllnt 2b99f1a
Merge branch 'branch-22.02' of github.com:rapidsai/raft into fix/buil…
trxcllnt bb4b7c7
move faiss into separate raft-faiss-exports export set, only include …
trxcllnt e798ab1
check for faiss in raft_FIND_COMPONENTS for build side export set too
trxcllnt 2d5898d
pass EXCLUDE_FROM_ALL in get_faiss.cmake
trxcllnt 2cb3c3a
enable CUDA language in code_string
trxcllnt 23912f5
pass TRUE
trxcllnt fc306f6
drop CUDA from the list of global languages
trxcllnt 61c263a
link raft to faiss if faiss component is requested
trxcllnt 41bd51b
link raft to faiss if faiss component requested
trxcllnt 4a95f2e
Merge branch 'branch-22.02' of github.com:rapidsai/raft into fix/buil…
trxcllnt 082eea9
Merge branch 'branch-22.02' of github.com:rapidsai/raft into fix/buil…
trxcllnt ce8a005
fix FAISS::FAISS -> faiss::faiss
trxcllnt c39685a
Simplify raft component CMake logic, and allow compilation without FAISS
robertmaynard af46a09
Add RAFT_ENABLE_NN_COMPONENT CMake build option
robertmaynard 1e9b9bd
Adding option to turn off buildign all shared libs
cjnolet 7fc712c
Refactor get_faiss to export faiss::faiss build target
trxcllnt a85b085
RAFT_ENABLE_NN_COMPONENT is a dependent option of RAFT_BUILD_SHARED_LIBS
robertmaynard 260a727
Using lowercase faiss target
cjnolet bb4a7f3
Changing RAFT_BUILD_SHARED_LIBS -> RAFT_COMPILE_LIBRARIES
cjnolet 8c02d2c
Passing deps transitively
cjnolet ebb7c1d
Moving faiss back to test target. We need to figure out how to pass
cjnolet c1e9c2d
Another round of cmake cleanups
robertmaynard fef8446
Merge "Add option to build faiss shared libs" into refactor_cmake_raf…
robertmaynard 52e8635
Refactor again now that I understand the libs are an optimization
robertmaynard 293302f
Move `nn` and `distance` to CMake components
robertmaynard 61e5b3c
Make searching for faiss a controllable option
robertmaynard 3701daf
Merge branch 'branch-22.02' into refactor_cmake_raft_target_logic
robertmaynard 60c81a7
fix typo
trxcllnt 55301e7
Merge branch 'branch-22.02' of github.com:rapidsai/raft into refactor…
trxcllnt d20e5ef
add GTest targets to install side of raft-exports export set
trxcllnt 1594f7f
install gtest if not already installed
trxcllnt 1c74204
revert get_gtest.cmake changes
trxcllnt 0bc750b
remove raft_ prefix from global targets list
trxcllnt a3c0370
revert previous change, move target_link_libraries below add_library …
trxcllnt ea209c1
fix _lib name to add prefix
trxcllnt 7093ad5
add missing PKG_ prefix
trxcllnt f613e39
guard creating alias targets when added as a submodule via CPM
trxcllnt b2e0b01
update cuco hash
trxcllnt ad90d9b
Merge branch 'branch-22.02' of github.com:rapidsai/raft into refactor…
trxcllnt 902657c
Replace the use of RMM's CUDA Python bindings with those from CUDA-Py…
shwina d7f0074
Add cuda-python to dev env
shwina c7666ad
Remove older comment about cudaStream_t attribute
shwina 8446e2f
Style
shwina 4de51ed
0 -> cudaSuccess
shwina 9d1bdc5
Add raft_export while rapids-cmake adds a export(COMPONENT) feature
robertmaynard 4f2d14b
Merge remote-tracking branch 'ashwin/replace-rmm-cuda-python-bindings…
cjnolet 1a2c611
raft_export marks components as found
robertmaynard 14f1937
add unprefixed raft_distance_lib and raft_nn_lib to handle both BUILD…
trxcllnt 2d2b23a
Merge branch 'branch-22.02' of github.com:rapidsai/raft into refactor…
trxcllnt cb69067
add versions.json to force Thrust v1.15.0
trxcllnt 47bbfd0
Using cuco branch w/ thrust 1.15
cjnolet 2685783
Reordering cuco and rmm cmake includes
cjnolet 2aa4ea9
Reverting previous changes
cjnolet c51fc3e
add get_thrust.cmake back in to ensure we get Thrust v1.15.0
trxcllnt 93ed2c7
Merge branch 'refactor_cmake_raft_target_logic' of github.com:robertm…
trxcllnt 5670308
revert cuco dependency hash
trxcllnt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a PR w/ cuml to integrate the changes in this PR but it looks like it's trying to build FAISS from source every time now, rather than using the shared lib that's already installed w/ conda.
I also tried removing the explicit call to
get_faiss
and linking of theFAISS::FAISS
targets in cuml but it didn't seem to make a difference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR doesn't change the version of RAFT it is using to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch. I've been building locally w/
CPM_raft_SOURCE
, though. Let me update the PR as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it looks like it's building FAISS from source in CI in cuml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes the name of the searched package to
faiss
( lowercase ) to follow modern CMake patterns, while your cuml PR keeps the existing behavior of usingFAISS
.This for CPM means they are distinctly different projects. When we look at the build logs we see the following:
So we can see that
FAISS
was found locally, andfaiss
is going to be built from source.In general we wouldn't expect RAFT search again for
faiss
but given the mismatch on names it does so.As for why RAFT is building
faiss
from source, the cumlget_raft.cmake
project file currently enablesCPM_DOWNLOAD_ALL
which forces raft and all dependencies to be built from source and not search for a local copy.So what can you do?
faiss
instead ofFAISS
get_raft
in cuml always trying to buildraft
from source. Since this will also stop the benifits ofraft::nn
andraft::distance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've verified (at least locally) that changing the faiss target (and
GLOBAL_TARGETS
for faiss tofaiss::faiss
) works when both with and withoutCPM_raft_SOURCE
.We're working towards item number 2. conda packaging will make that much easier to implement and maintain.
Thanks again, @robertmaynard!