faiss-split v1.10.0#83
Conversation
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipe/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/23209478862. Examine the logs at this URL for more detail. |
|
Still no support for python 3.13 and numpy 2.0 🥲 |
…nda-forge-pinning 2025.01.28.22.50.30
3cb931a to
05b7e32
Compare
|
Hi! This is the friendly conda-forge automerge bot! I considered the following status checks when analyzing this PR:
Thus the PR was not passing and not merged. |
|
@conda-forge-admin please rerender |
…nda-forge-pinning 2025.03.25.12.23.56
|
Hi! This is the friendly conda-forge automerge bot! Commits were made to this PR after the |
|
@conda-forge-admin please rerender |
…nda-forge-pinning 2025.03.25.21.45.18
|
my commits might be trash. they might need to be force pushed out. |
weiji14
left a comment
There was a problem hiding this comment.
Seems like this is where the action is happening! @h-vetinari, I'd be keen to help move this along a bit more, have opened #95 to add myself to the maintainer list.
| {% set skips = skips + " or (TestIndexProduct and test_accuracy1)" %} | ||
| {% set skips = skips + " or test_wrapped_quantizer_HNSW" %} | ||
| {% endif %} | ||
| # the linux & windows CI agents support AVX2 (OSX doesn't yet), so by default, |
There was a problem hiding this comment.
Testing this locally on Linux (no CUDA build), and hitting into facebookresearch/faiss#3434 (comment). Guessing it might be a numerical precision issue that can be skipped?
| # failing TestExhaustiveSearch.test_query_iterator and TestComponents.test_update_codebooks_with_double | |
| # https://github.com/facebookresearch/faiss/issues/3434 | |
| {% set skips = skips + " or test_query_iterator" %} # [unix] | |
| {% set skips = skips + " or test_update_codebooks_with_double" %} # [unix] | |
| # the linux & windows CI agents support AVX2 (OSX doesn't yet), so by default, |
There was a problem hiding this comment.
Different failing tests now at https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=1487821&view=logs&j=d7c6c07b-672a-5621-e92f-8f41e568f8b9&t=beb7c644-b9b6-55d9-7f79-d332c6a4ed56&l=3925:
=========================== short test summary info ============================
FAILED tests/test_contrib.py::TestPreassigned::test_float - TypeError: Wrong number or type of arguments for overloaded function 'IndexIVF_range_search_preassigned'.
Possible C/C++ prototypes are:
faiss::IndexIVF::range_search_preassigned(faiss::idx_t,float const *,float,faiss::idx_t const *,float const *,faiss::RangeSearchResult *,bool,faiss::IVFSearchParameters const *,faiss::IndexIVFStats *) const
faiss::IndexIVF::range_search_preassigned(faiss::idx_t,float const *,float,faiss::idx_t const *,float const *,faiss::RangeSearchResult *,bool,faiss::IVFSearchParameters const *) const
faiss::IndexIVF::range_search_preassigned(faiss::idx_t,float const *,float,faiss::idx_t const *,float const *,faiss::RangeSearchResult *,bool) const
faiss::IndexIVF::range_search_preassigned(faiss::idx_t,float const *,float,faiss::idx_t const *,float const *,faiss::RangeSearchResult *) const
FAILED tests/test_contrib.py::TestClustering::test_ivf_train_2level - AssertionError: np.int64(55) not less than 53
FAILED tests/test_local_search_quantizer.py::TestComponents::test_update_codebooks_with_double - AssertionError: np.float32(6374.6978) not less than np.float32(6140.4946)
= 3 failed, 887 passed, 1 skipped, 1 deselected, 1 warning in 1090.38s (0:18:10) =
WARNING: Tests failed for faiss-1.10.0-py310h95fd687_0_cpu.conda - moving package to /home/conda/feedstock_root/build_artifacts/broken8746382 to
acac1e3
Compare
Also restrict to cuda_compiler_version != "None" or blas_impl='mkl' for exactness.
| - libgomp # [linux] | ||
| - llvm-openmp # [osx] | ||
| {% if cuda_major >= 12 %} | ||
| - intel-openmp # [win and blas_impl == 'mkl'] |
There was a problem hiding this comment.
Ok, too many segfaults happening on Win + MKL (see #83 and #83) using intel-openmp, but not win + openblas/blis (which are pulling in libgomp).
I'm reading conda-forge/intel_repack-feedstock#115 and thinking whether llvm-openmp should be used instead... Should probably put this under host dependencies of libfaiss also? I'll try with just moving intel-openmp from faiss to libfaiss first, because it seems like libgomp was used there.
There was a problem hiding this comment.
Oo, but the rabbit hole takes me to conda-forge/pytorch-cpu-feedstock#336 which is suggesting that llvm-openmp might be the way to go 🤔
There was a problem hiding this comment.
Hmm ok, so for Windows, CI at commit 24a0c39:
- mkl: built with
intel-openmp, but pulled inllvm-openmpat runtime raised anImportError: DLL load failed while importing _swigfaiss, seems like similar situation to OpenMP follow-ups on windows pytorch-cpu-feedstock#336 (comment) - openblas/blis: somehow had both
vcomp14andllvm-openmppulled in at build time (think it might be using the former?). Thelibfaissbuild seemed to be ok, butfaiss(python) build errored with:
[5/8] Linking CXX shared library _swigfaiss.pyd
FAILED: [code=4294967295] _swigfaiss.pyd swigfaiss.lib
C:\Windows\system32\cmd.exe /C "cd . && D:\bld\faiss-split_1773865256180\_build_env\Library\bin\cmake.exe -E vs_link_dll --msvc-ver=1944 --intdir=CMakeFiles\swigfaiss.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100261~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100261~1.0\x64\mt.exe --manifests -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\link.exe /nologo CMakeFiles\swigfaiss.dir\CMakeFiles\swigfaiss.dir\swigfaissPYTHON_wrap.cxx.obj /out:_swigfaiss.pyd /implib:swigfaiss.lib /pdb:_swigfaiss.pdb /dll /version:0.0 /machine:x64 /LIBPATH:D:/bld/faiss-split_1773865256180/work/build/_libfaiss_stage/lib /INCREMENTAL:NO D:\bld\faiss-split_1773865256180\_h_env\Library\lib\faiss.lib faiss_python_callbacks.lib D:\bld\faiss-split_1773865256180\_h_env\libs\python310.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\link.exe /nologo CMakeFiles\swigfaiss.dir\CMakeFiles\swigfaiss.dir\swigfaissPYTHON_wrap.cxx.obj /out:_swigfaiss.pyd /implib:swigfaiss.lib /pdb:_swigfaiss.pdb /dll /version:0.0 /machine:x64 /LIBPATH:D:/bld/faiss-split_1773865256180/work/build/_libfaiss_stage/lib /INCREMENTAL:NO D:\bld\faiss-split_1773865256180\_h_env\Library\lib\faiss.lib faiss_python_callbacks.lib D:\bld\faiss-split_1773865256180\_h_env\libs\python310.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=2" failed (exit code 1120) with the following output:
Creating library swigfaiss.lib and object swigfaiss.exp
swigfaissPYTHON_wrap.cxx.obj : error LNK2019: unresolved external symbol "public: void __cdecl faiss::HNSW::add_links_starting_from(struct faiss::DistanceComputer &,int,int,float,int,struct omp_lock_t *,struct faiss::VisitedTable &,bool)" (?add_links_starting_from@HNSW@faiss@@QEAAXAEAUDistanceComputer@2@HHMHPEAUomp_lock_t@@AEAUVisitedTable@2@_N@Z) referenced in function _wrap_HNSW_add_links_starting_from__SWIG_0
swigfaissPYTHON_wrap.cxx.obj : error LNK2019: unresolved external symbol "public: void __cdecl faiss::HNSW::add_with_locks(struct faiss::DistanceComputer &,int,int,class std::vector<struct omp_lock_t,class std::allocator<struct omp_lock_t> > &,struct faiss::VisitedTable &,bool)" (?add_with_locks@HNSW@faiss@@QEAAXAEAUDistanceComputer@2@HHAEAV?$vector@Uomp_lock_t@@V?$allocator@Uomp_lock_t@@@std@@@std@@AEAUVisitedTable@2@_N@Z) referenced in function _wrap_HNSW_add_with_locks__SWIG_0
_swigfaiss.pyd : fatal error LNK1120: 2 unresolved externals
I need to get my head around these openmp mutexes somehow...
There was a problem hiding this comment.
The answer is to move everything to llvm-openmp
There was a problem hiding this comment.
The answer is to move everything to
llvm-openmp
Ok, I like simple.
There was a problem hiding this comment.
Hmm, llvm-openmp doesn't seem to be working for blas_impl=blis or openblas on Windows (getting that error above in #83 (comment)). I'll try with libgomp for all Windows builds (mkl/blis/openblas) because that seemed to be working before (on blis/openblas).
Edit: libgomp doesn't work for Win + MKL. Might need to go back to intel-openmp and deal with the segfaults 😭 or consider dropping that build variant for now.
There was a problem hiding this comment.
Stay on llvm-openmp; with the exception of MKL, you should stay on default libblas / liblapack. For MKL, you may need something like
libblas:
- 3.11
liblapack:
- 3.11There was a problem hiding this comment.
Ok, I'll try that in another PR!
See if this fixes the win + mkl segfaults, possibly because libfaiss was built with libgomp and not intel-openmp? Also use llvm-openmp instead of libgomp for win non-mkl variants.
Which might have been failing due to libfaiss built with libgomp while faiss was built with intel-openmp
See if this prevents pulling in vcomp14 for windows mkl or non-mkl builds.
50bd966 to
814fc86
Compare
while keeping llvm-openmp for osx and linux
Re-enable in the future when have more time to debug the segfaults happening with intel-openmp or something else.
…6.03.18.17.42.56 Other tools: - conda-build 25.11.1 - rattler-build 0.60.0 - rattler-build-conda-compat 1.4.6
This reverts commit 05e92b5.
|
Ok, I'm skipping the Windows + MKL cpu builds because there's too many segfaults (see #83 (comment)), so Windows will only have OpenBLAS or BLIS builds for now (and also, no CUDA builds yet). If there's any Windows users who do need these variants, feel free to open an issue! I think there's enough progress on this PR now, so will try to merge tomorrow (once CI passes) and continue on tidying up this recipe in the v1.11.0 (#88) update. |
|
You should have really asked for review before merging this. |
|
Oh sorry, my bad, did something break? I was gonna incrementally apply more changes in 1.11.0, 1.12.0, etc to keep things flowing (see #98). I'll make sure to request reviews going forward. |
|
I did the unusual step of adding you (in #95) without a PR that actually fixes the feedstock out of a surplus of upfront trust, because I appreciate the effort to get this feedstock back on track. But to turn that around and just merge stuff that I've never had a chance to even look at is a pretty offensive move after just having joined a feedstock.
Building per faiss-split-feedstock/recipe/meta.yaml Lines 124 to 125 in 0c419d5
Revert this PR (or at least your changes) completely and try again, then I'll take a look at it. |
It is very likely that the current package version for this feedstock is out of date.
Checklist before merging this PR:
license_fileis packagedInformation about this PR:
@conda-forge-admin,please add bot automergein the title and merge the resulting PR. This command will add our bot automerge feature to your feedstock.bot-rerunlabel to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase@conda-forge-admin, please rerun botin a PR comment to have theconda-forge-adminadd it for you.Pending Dependency Version Updates
Here is a list of all the pending dependency version updates for this repo. Please double check all dependencies before merging.
This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. Feel free to drop us a line if there are any issues! This PR was generated by https://github.com/regro/cf-scripts/actions/runs/13064394991 - please use this URL for debugging.
Closes #90 (CUDA 12.9 rebuild)
Closes #77 (NumPy 2.0 rebuild)
Closes #79 (Python 3.13 rebuild)
Closes #87 (other v1.10.0 attempt)