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

add package/faiss #25272

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add package/faiss #25272

wants to merge 2 commits into from

Conversation

simshi
Copy link

@simshi simshi commented Sep 15, 2024

Summary

Add new recipe: faiss (https://github.com/facebookresearch/faiss)

Motivation

Faiss is a library for efficient similarity search and clustering of dense vectors.

Details

Limitations

  1. Because there is no available CUDAToolkit/system and OpenMP/system, consumers must express dependencies on those dependencies by themself. Please refer to test_package/CMakeLists.txt as example.
    • Solved cuda dependency, add a cmake build module file, by @valgur
  2. Faiss prefers MKL, but no conan package, now, using OpenBLAS without an option to choose.
  3. OpenBLAS recipe lacks of options like USE_OPENMP, NUM_THREADS, faiss openmp code calls into openblas without openmp supporting may cause issues.

Requires


@CLAassistant
Copy link

CLAassistant commented Sep 15, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.


def requirements(self):
self.requires("openblas/0.3.27")
self.test_requires("gtest/1.12.1")
Copy link
Contributor

Choose a reason for hiding this comment

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

self.test_requires() be placed under build_requirements() instead.

Besides that, following the value of the tools.build:skip_test config option on CCI is discouraged as it can lead to unexpected test failures for consumers and CCI maintainers try to avoid the additional potential burden from bug reports related to these.

Comment on lines 79 to 88
if not (
self.options.with_opt_generic or
self.options.get_safe("with_opt_avx2", False) or
self.options.get_safe("with_opt_avx512", False)
):
raise ConanInvalidConfiguration("""Must configure at least one of following optimizations:
- generic: no optimization
- avx2: with x86 AVX2 instruction set
- avx512 (only availible >=1.8.0): with x86 AVX512 instruction set
""")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest keeping the recipe slightly simpler and always enabling FAISS_WITH_OPT_GENERIC for non-debug builds instead. What do you think?

- avx512 (only availible >=1.8.0): with x86 AVX512 instruction set
""")
if not self.dependencies["openblas"].options.get_safe("use_openmp"):
self.output.warning("Prefers openbals with use_openmp")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.output.warning("Prefers openbals with use_openmp")
self.output.warning("-o openblas/*:use_openmp is recommended but is not available.")

- target_compile_options(faiss_avx2 PRIVATE $<$<COMPILE_LANGUAGE:CXX>:/arch:AVX2>)
- # we need bigobj for the swig wrapper
- add_compile_options(/bigobj)
+find_package(OpenMP REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 4 to 5
"1.7.4":
folder: all
Copy link
Contributor

Choose a reason for hiding this comment

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

I would only keep the latest version unless you specifically need the older one for some reason.

@valgur
Copy link
Contributor

valgur commented Sep 19, 2024

Since v1.8.0, faiss has a separated gpu library, linked to cpu lib by whole-archive, but conan itself has no solution for it

The libtorch recipe had a similar issue and has a workaround for it: #24759. Probably useful here as well.

Copy link
Contributor

@valgur valgur left a comment

Choose a reason for hiding this comment

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

Needs some work, but looks good overall. Thank you!

@valgur
Copy link
Contributor

valgur commented Sep 19, 2024

I simplified the recipe a bit by avoiding most of the patching, improved OpenMP and CUDA support and filled in a few other gaps on my personal fork: https://github.com/valgur/conan-center-index/blob/dev/recipes/faiss/all/conanfile.py

@simshi
Copy link
Author

simshi commented Sep 20, 2024

Thank you, that's great!

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Failure in build 3 (8a03b4b815f1a2542bd2a700f39c930878f9f669):

  • faiss/1.8.0:
    Error running command conan info faiss/1.8.0@#7d1189f594ffd764b3a019ad57a090d1 --json {jsonName} --dry-build -pr {profileName}:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=5
    os=Linux
    [options]
    faiss:shared=False
    
    ...
    ERROR: Failed requirement 'openmp/system' from 'faiss/1.8.0'
    ERROR: Unable to find 'openmp/system' in remotes
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.


Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

Failure in build 2 (8a03b4b815f1a2542bd2a700f39c930878f9f669):

  • faiss/1.8.0:
    Error running command conan graph info --requires faiss/1.8.0@#7d1189f594ffd764b3a019ad57a090d1 --build faiss/1.8.0@#7d1189f594ffd764b3a019ad57a090d1 -f json -pr:h /home/conan/workspace/prod-v2/bsr/87548/cefde/profile_linux_11_libstdcpp11_17_gcc_release_64.-shared-False.txt -pr:b /home/conan/workspace/prod-v2/bsr/87548/cefde/profile_linux_11_libstdcpp11_17_gcc_release_64..txt:
    ======== Computing dependency graph ========
    openblas/0.3.27: Not found in local cache, looking in remotes...
    openblas/0.3.27: Checking remote: conan-center
    openblas/0.3.27: Downloaded recipe revision c16c3dbdf45e4a4ff56f8ada8631a2f3
    openmp/system: Not found in local cache, looking in remotes...
    openmp/system: Checking remote: conan-center
    openmp/system: Checking remote: c3i_PR-v2-25272
    Graph root
        cli
    Requirements
        faiss/1.8.0#7d1189f594ffd764b3a019ad57a090d1 - Cache
        openblas/0.3.27#c16c3dbdf45e4a4ff56f8ada8631a2f3 - Downloaded (conan-center)
    ERROR: Package 'openmp/system' not resolved: Unable to find 'openmp/system' in remotes.
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@valgur
Copy link
Contributor

valgur commented Sep 20, 2024

Opened a PR for OpenMP support in OpenBLAS: #25344. It's already included in my personal fork as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants