-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[faiss] Add new port #13825
[faiss] Add new port #13825
Conversation
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Thanks @NancyLi1013. I applied your suggested changes. |
You should also update the hash value. |
@NancyLi1013 It happened what I suspected to happen. The older version 1.6.3 doesn't compile, because they did not have CMake build files back then. I suggest to revert back to the proposed commit from above. Since then also many bug fixes have gone into the code, so we should really try to be on the latest version in the master branch. |
Hi @ahojnnes I checked the upstream. There is another new release, we might update it to 1.7.4. |
@NancyLi1013 I updated to v1.6.4. It should work now. |
Thanks for your quick update. |
@NancyLi1013 I have tested the package for a while already. It works fine. One remaining open question is how to best manage the dependency against lapack and openblas, which both typically compile with special instruction sets. The current package configuration of faiss here compiles against lapack/openblas statically, which means that the produced package is not redistributable to another machine. Not sure what the best way is to manage this problem in vcpkg. Typically, you would always want to use an optimized lapack/blas implementation for your specific machine. Any suggestions? |
Hi @ahojnnes I noticed that other ports such as |
@NancyLi1013 It seems like it is all fine. I also checked other libraries depending on lapack and openblas. I think we can get this PR merged as is. I tested this quite extensively on Windows & Linux over the last weeks. |
LGTM now, thanks for your PR @ahojnnes. |
Thanks for your contribution! |
This PR adds FAISS as a new port. This resolves issue #1405.