Skip to content

[FIX] Always add faiss library alias if it's missing#1694

Closed
trxcllnt wants to merge 2 commits intorapidsai:branch-21.08from
trxcllnt:fix/faiss-alias
Closed

[FIX] Always add faiss library alias if it's missing#1694
trxcllnt wants to merge 2 commits intorapidsai:branch-21.08from
trxcllnt:fix/faiss-alias

Conversation

@trxcllnt
Copy link
Collaborator

@trxcllnt trxcllnt commented Jul 3, 2021

Always add the FAISS::FAISS library target alias if it doesn't exist. This can happen if cuML is built and installs FAISS before cuGraph.

rapidsai/cuml#4028

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #1694   +/-   ##
===============================================
  Coverage                ?   59.31%           
===============================================
  Files                   ?       80           
  Lines                   ?     3557           
  Branches                ?        0           
===============================================
  Hits                    ?     2110           
  Misses                  ?     1447           
  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 e2ac467...039c38f. Read the comment docs.

rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Jul 6, 2021
Always add the `FAISS::FAISS` library target alias if it doesn't exist. This can happen if cuGraph is built and installs FAISS before cuML.

rapidsai/cugraph#1694

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)

URL: #4028
@BradReesWork BradReesWork added improvement Improvement / enhancement to an existing function 3 - Ready for Review non-breaking Non-breaking change labels Jul 6, 2021
@BradReesWork BradReesWork added this to the 21.08 milestone Jul 6, 2021
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

Why do we need FAISS?

@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Jul 7, 2021

@ChuckHastings it's a raft dependency, though now that you mention it looks like raft needs this change too. And if raft gets it, we can probably remove cuML's and cuGraph's get_faiss.cmake file.

@BradReesWork
Copy link
Member

BradReesWork commented Jul 9, 2021

@trxcllnt if we can put this in RAFT it would make more sense and solve both graph and ml. Putting this PR on hold until you respond

@BradReesWork BradReesWork added 4 - Waiting on Author DO NOT MERGE Hold off on merging; see PR for details labels Jul 9, 2021
rapids-bot bot pushed a commit that referenced this pull request Jul 9, 2021
…etting UCX install location (#1698)

This PR is a continuation of #1694. Similar to rapidsai/cuml#4015, this PR updates setup.py to:

* Use `library_dirs` instead of `runtime_library_dirs` when linking Cython.
* Allow overriding UCX lib and include dirs via a `UCX_HOME` envvar.
* Link `cudart`, `cusparse`, and `cusolver`.

These are necessary to compile the Cython via `pip` when not inside a conda environment and when UCX is installed to a location other than `/usr` or `/usr/local`.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)

URL: #1698
@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Jul 9, 2021

@BradReesWork The RAFT PR is here: rapidsai/raft#287. This commit was included in PR #1698, so I'll close this one.

@trxcllnt trxcllnt closed this Jul 9, 2021
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Jul 9, 2021
Always add the `FAISS::FAISS` library target alias if it doesn't exist. This can happen if cuML is built and installs FAISS before cuGraph or vice-versa.

Related PRs:
rapidsai/cuml#4028
rapidsai/cugraph#1694

Note: We can probably remove the `get_faiss.cmake` file in cuML and cuGraph since they both should get it from RAFT.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #287
@caryr35 caryr35 modified the milestones: 21.08, 21.10 Aug 4, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Always add the `FAISS::FAISS` library target alias if it doesn't exist. This can happen if cuGraph is built and installs FAISS before cuML.

rapidsai/cugraph#1694

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)

URL: rapidsai#4028
loulankxh pushed a commit to loulankxh/raft that referenced this pull request Oct 14, 2025
Always add the `FAISS::FAISS` library target alias if it doesn't exist. This can happen if cuML is built and installs FAISS before cuGraph or vice-versa.

Related PRs:
rapidsai/cuml#4028
rapidsai/cugraph#1694

Note: We can probably remove the `get_faiss.cmake` file in cuML and cuGraph since they both should get it from RAFT.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO NOT MERGE Hold off on merging; see PR for details 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.

5 participants