Skip to content

[Faiss] Fix dependencies#18780

Merged
vicroms merged 11 commits intomicrosoft:masterfrom
bobuc-havok:faiss-dependencies
Jul 16, 2021
Merged

[Faiss] Fix dependencies#18780
vicroms merged 11 commits intomicrosoft:masterfrom
bobuc-havok:faiss-dependencies

Conversation

@bobuc-havok
Copy link
Copy Markdown
Contributor

Describe the pull request

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@NancyLi1013 NancyLi1013 self-assigned this Jul 5, 2021
@NancyLi1013 NancyLi1013 changed the title Faiss dependencies [Faiss] Fix dependencies Jul 5, 2021
@NancyLi1013 NancyLi1013 added the category:port-bug The issue is with a library, which is something the port should already support label Jul 5, 2021
Comment thread ports/faiss/vcpkg.json Outdated
Comment thread ports/lapack-reference/portfile.cmake
@NancyLi1013
Copy link
Copy Markdown
Contributor

faiss failed on x64-windows with the following errors:

/pdb:faiss\faiss.pdb /dll /version:0.0 /machine:x64 /nologo /debug /INCREMENTAL /DEF:faiss\CMakeFiles\faiss.dir\.\exports.def lapack.lib D:\installed\x64-windows\debug\lib\openblas.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:faiss\CMakeFiles\faiss.dir/intermediate.manifest faiss\CMakeFiles\faiss.dir/manifest.res" failed (exit code 1104) with the following output:
LINK : fatal error LNK1104: cannot open file 'lapack.lib'
ninja: build stopped: subcommand failed.

@bobuc-havok
Copy link
Copy Markdown
Contributor Author

@NancyLi1013 yup, saw that build break, it went green now :)

@bobuc-havok bobuc-havok requested a review from NancyLi1013 July 8, 2021 11:24
@NancyLi1013
Copy link
Copy Markdown
Contributor

NancyLi1013 commented Jul 9, 2021

LGTM now, have you tested the feature gpu?

@bobuc-havok
Copy link
Copy Markdown
Contributor Author

@NancyLi1013 good point, I added a small fix to the port file for gpu. That said faiss[gpu] does not build for me locally, regardless of any changes. I'm getting nvcc compiler errors such as:

faiss/gpu/utils/Tensor.cuh(366): error: argument list for template "T::canUseIndexType" is missing
faiss/gpu/utils/PtxUtils.cuh(25): error: asm operand type size(4) does not match type/size implied by constraint 'l'

There are others. In terms of CMake targets, the gpu feature should be fine, as the dependency is declared as:

find_package(CUDAToolkit REQUIRED)
target_link_libraries(faiss PRIVATE CUDA::cudart CUDA::cublas)

so it should not generate absolute paths. I'm not sure if the CI builds test faiss[gpu], but in case the failure to compile the cuda code is more general than on my machine, I think it should be split out from this PR / issue.

@NancyLi1013
Copy link
Copy Markdown
Contributor

NancyLi1013 commented Jul 12, 2021

gpu is not default feature, so it will not be tested on CI. Thanks for your fix and information. Now can you build faiss[gpu]:x64-windows on your local machine?

@bobuc-havok
Copy link
Copy Markdown
Contributor Author

@NancyLi1013 no I cannot build faiss[gpu]:x64-windows locally, I'm getting the nvcc compiler errors mentioned above. It might be because I don't have an NVidia graphics card on my machine, not sure.

@NancyLi1013
Copy link
Copy Markdown
Contributor

I was blocked by the failure on my local with master branch.

CMake Error at CMakeLists.txt:26 (enable_language):
  No CMAKE_CUDA_COMPILER could be found.

Have you tried with master branch on your machine? Does it work? If you have the same problem with the changes on this PR, it might not be the new issue introduced by this PR.

@bobuc-havok
Copy link
Copy Markdown
Contributor Author

@NancyLi1013 yup, I am getting the same cuda compiler not found error on master. Seems to be common, as other ports have fixes for it. It looks like you can't get to compiling cuda on master until you make changes and fix the cuda compiler like I have in the portfile in this PR. I don't think my changes broke the faiss[cuda] - I think it was simply broken from the start and not tested on CI.

@NancyLi1013 NancyLi1013 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jul 16, 2021
@NancyLi1013
Copy link
Copy Markdown
Contributor

Thanks for your efforts and patience. @bucurb

@vicroms vicroms merged commit 0f05959 into microsoft:master Jul 16, 2021
@vicroms
Copy link
Copy Markdown
Member

vicroms commented Jul 16, 2021

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-bug The issue is with a library, which is something the port should already support info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[faiss] The target files on x64-linux contain absolute paths

3 participants