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

[FEA] Remove FAISS dependency, inherit other common dependencies from raft #1863

Merged
merged 74 commits into from
Jan 23, 2022

Conversation

trxcllnt
Copy link
Collaborator

@trxcllnt trxcllnt commented Sep 30, 2021

Originally this PR was about building FAISS shared libs via CPM, but @rlratzel mentioned cuGraph doesn't need FAISS anymore, and it'd be better if we remove it.

If we need FAISS again in the future, we can add faiss::faiss back to target_link_libraries without needing extra CPM configuration as it will be available via raft.

Edit: Removed more dependencies that we inherit from raft and/or rmm. Depends on rapidsai/raft#345.

Edit 2: I think the CUDA 11.0 thrust issue will be solved by rapidsai/rapids-cmake#98

@trxcllnt trxcllnt requested a review from a team as a code owner September 30, 2021 20:49
@trxcllnt trxcllnt changed the base branch from branch-21.12 to branch-21.10 September 30, 2021 20:49
@trxcllnt trxcllnt added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.02@f4b5f3d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.02    #1863   +/-   ##
===============================================
  Coverage                ?   70.75%           
===============================================
  Files                   ?      142           
  Lines                   ?     8861           
  Branches                ?        0           
===============================================
  Hits                    ?     6270           
  Misses                  ?     2591           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b5f3d...bb815aa. Read the comment docs.

@BradReesWork
Copy link
Member

@trxcllnt is this really a hotfix for 21.10?

@ajschmidt8
Copy link
Member

rerun tests

cpp/cmake/thirdparty/get_faiss.cmake Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_faiss.cmake Outdated Show resolved Hide resolved
@BradReesWork BradReesWork changed the base branch from branch-21.10 to branch-21.12 October 4, 2021 16:04
@BradReesWork BradReesWork added this to the 21.12 milestone Oct 4, 2021
@trxcllnt trxcllnt changed the title [FEA] Add option to build faiss shared libs [FEA] Remove FAISS dependency Oct 6, 2021
@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Oct 6, 2021

rerun tests

@trxcllnt
Copy link
Collaborator Author

rerun tests

4 similar comments
@trxcllnt
Copy link
Collaborator Author

rerun tests

@trxcllnt
Copy link
Collaborator Author

rerun tests

@trxcllnt
Copy link
Collaborator Author

rerun tests

@trxcllnt
Copy link
Collaborator Author

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Dec 16, 2021

We recently merged the PR to raft that establishes multiple output targets- specifically, there is now a libraft-nn package which depends kn FAISS (and which cugraph no longer depends on directly). Does this help at all alleviate the FAISS issue on cugraph?

@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Jan 6, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jan 11, 2022

@trxcllnt, not sure if it helps but I opened #2009 to fully remove the faiss dependency from cugraph and implement @robertmaynard's recent changes to the raft build.

@trxcllnt
Copy link
Collaborator Author

rerun tests

1 similar comment
@trxcllnt
Copy link
Collaborator Author

rerun tests

@trxcllnt
Copy link
Collaborator Author

This PR has been updated to use rapidsai/raft#428

@trxcllnt
Copy link
Collaborator Author

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jan 19, 2022

rerun tests

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Jan 20, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Jan 20, 2022

rerun tests

1 similar comment
@trxcllnt
Copy link
Collaborator Author

rerun tests

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit bae28f7 into rapidsai:branch-22.02 Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants