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

faiss 1.7.4 fails to build with swig 4.2.0 #3239

Closed
2 of 3 tasks
pushkarnk opened this issue Feb 4, 2024 · 9 comments
Closed
2 of 3 tasks

faiss 1.7.4 fails to build with swig 4.2.0 #3239

pushkarnk opened this issue Feb 4, 2024 · 9 comments
Assignees
Labels

Comments

@pushkarnk
Copy link

Summary

The Ubuntu and Debian packages of faiss (1.7.4) are failing to build from source when swig 4.2.0 is used. I haven't yet tried a build with swig 4.1.0, but I am pretty sure only swig changed between passing and failing builds.

Platform

Ubuntu/Debian Linux (development release).

Faiss version: release 1.7.4

Installed from: Building from source

Faiss compilation options:

Running on:

  • CPU
  • GPU

Interface:

  • [] C++
  • Python

Reproduction instructions

Its my guess that swig 4.2.0 does not generate a couple of Python bindings required by faiss. I see these error messages:

/<<PKGBUILDDIR>>/build/faiss/python/CMakeFiles/swigfaiss.dir/swigfaissPYTHON_wrap.cxx: In function ‘PyObject* swig_ptr(PyObject*)’:
/<<PKGBUILDDIR>>/build/faiss/python/CMakeFiles/swigfaiss.dir/swigfaissPYTHON_wrap.cxx:4939:41: error: ‘SWIGTYPE_p_unsigned_long_long’ was not declared in this scope; did you mean ‘SWIGTYPE_p_unsigned_long’?
 4939 | return SWIG_NewPointerObj(data, SWIGTYPE_p_unsigned_long_long, 0);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/<<PKGBUILDDIR>>/build/faiss/python/CMakeFiles/swigfaiss.dir/swigfaissPYTHON_wrap.cxx:1136:94: note: in definition of macro ‘SWIG_NewPointerObj’
 1136 | #define SWIG_NewPointerObj(ptr, type, flags) SWIG_Python_NewPointerObj(NULL, ptr, type, flags)
      | ^~~~
/<<PKGBUILDDIR>>/build/faiss/python/CMakeFiles/swigfaiss.dir/swigfaissPYTHON_wrap.cxx:4946:41: error: ‘SWIGTYPE_p_long_long’ was not declared in this scope; did you mean ‘SWIGTYPE_p_long’?
 4946 | return SWIG_NewPointerObj(data, SWIGTYPE_p_long_long, 0);
      | ^~~~~~~~~~~~~~~~~~~~
/<<PKGBUILDDIR>>/build/faiss/python/CMakeFiles/swigfaiss.dir/swigfaissPYTHON_wrap.cxx:1136:94: note: in definition of macro ‘SWIG_NewPointerObj’
 1136 | #define SWIG_NewPointerObj(ptr, type, flags) SWIG_Python_NewPointerObj(NULL, ptr, type, flags)
      | ^~~~
[ 92%] Building CXX object c_api/CMakeFiles/faiss_c.dir/utils/distances_c.cpp.o
[ 92%] Building CXX object tests/CMakeFiles/faiss_test.dir/test_pq_encoding.cpp.o

This was also reported by #3235.

@pushkarnk
Copy link
Author

I haven't had the chance to delve deeper into this, yet. Just putting it up here so that I don't forget it. Any inputs will be appreciated!

@mrzetti
Copy link

mrzetti commented Feb 5, 2024

As commented in the other issue:
Can confirm swig 4.2.0 is the issue.

Building (for gpu at least) worked fine after downgrading to swig 4.1.0.

OS: Ubuntu20.04
Python: 3.11.5

@SixK
Copy link

SixK commented Feb 15, 2024

This appear to me to something similar to this issue:
LudovicRousseau/PyKCS11#107

The problem is probably in this file:
faiss/faiss/python /swigfaiss.swig

code must use a $descriptor() function instead of directly defining variable type.
In PyKCS11 fix has been done like this:
if (SWIG_IsOK(SWIG_ConvertPtr($input, (void **)&vect, $descriptor(vector *), 0)))

instead of using something like that:
if (SWIG_IsOK(SWIG_ConvertPtr($input, (void **)&vect, SWIGTYPE_p_std__vectorT_unsigned_char_std__allocatorT_unsigned_char_t_t *), 0)))

I have no idea of what would be the correct way to fix the problem,
but I guess it could be something like this:
return SWIG_NewPointerObj(data, $descriptor(long long *), 0);

@saisurbehera
Copy link

saisurbehera commented Feb 17, 2024

Confirm, i have the issue too. I am on Centos 7

@zhewang1-intc
Copy link

echo. same issue with swig 4.2.1

@SixK
Copy link

SixK commented Feb 27, 2024

As said, someone has to modify swigfaiss.swig to use $descriptor() function to generate variables types dynamically depending on Swig version.
Actually SWIGTYPE_p_unsigned_long_long only exists with Swig < 4.2.0
$descriptor() will generate the right type for Swig < 4.2.0 and for Swig >= 4.2.0
You just have to guess the right value to put in $descriptor()

for the following error:
/<>/build/faiss/python/CMakeFiles/swigfaiss.dir/swigfaissPYTHON_wrap.cxx:4939:41: error: ‘SWIGTYPE_p_unsigned_long_long’ was not declared in this scope; did you mean ‘SWIGTYPE_p_unsigned_long’?
4939 | return SWIG_NewPointerObj(data, SWIGTYPE_p_unsigned_long_long, 0);
I would try:
return SWIG_NewPointerObj(data, $descriptor(unsigned long), 0)
or
return SWIG_NewPointerObj(data, $descriptor(unsigned long long), 0)
or
return SWIG_NewPointerObj(data, $descriptor(unsigned long *), 0)

Modifying setup.py or Makefile to generate pre-compile C++ files using gcc -E could help to find the right type to use cause you should find something like "#define SWIGTYPE_p_unsigned_long_long unsigned long" in generated file

@navneet1v
Copy link

navneet1v commented Mar 22, 2024

+1, facing the same issue on ubuntu 20.04. Will try and see if reducing swig version help.

For me the issue happens on GPU but not on CPU. On my mac I have swig version 4.2.0 and there is no issue.

@SixK
Copy link

SixK commented Mar 24, 2024

I may have a clue of what's wrong and how to solve the problem.

I probably was wrong with $descriptor() use as it seem's to only apply to structures (I don't really understand what Swig do).

Swig 4.2.x seem's no more compatible with 32bits code: swig/swig@9fb3a49
In this commit they removed 32bits support so they removed SWIGWORDSIZE64.

A quick and dirty fix could be done like this in swigfaiss.swig file, but it would loose compatibility with Swig <4.2:

    if(PyArray_TYPE(ao) == NPY_UINT64) {
        return SWIG_NewPointerObj(data, SWIGTYPE_p_unsigned_long, 0);
    }
    if(PyArray_TYPE(ao) == NPY_INT64) {
        return SWIG_NewPointerObj(data, SWIGTYPE_p_long, 0);
    }

At least with Swig 4.2.0, source code compile and tests (except 1, but it probably failed before) succeed.

To keep compatibility with Swig <4.2 this would probably be better:

    if(PyArray_TYPE(ao) == NPY_UINT64) {
#ifdef SWIGWORDSIZE32
        return SWIG_NewPointerObj(data, SWIGTYPE_p_unsigned_long_long, 0);
#else
        return SWIG_NewPointerObj(data, SWIGTYPE_p_unsigned_long, 0);
#endif
    }
    if(PyArray_TYPE(ao) == NPY_INT64) {
#ifdef SWIGWORDSIZE32
        return SWIG_NewPointerObj(data, SWIGTYPE_p_long_long, 0);
#else
        return SWIG_NewPointerObj(data, SWIGTYPE_p_long, 0);
#endif
}

junjieqi added a commit that referenced this issue Mar 25, 2024
Summary:
Currently, faiss can't build with swig version > 4.2.x. As the #3239 mentioned.  Swig removed the support for 32bit swig/swig@9fb3a49. So SWIGTYPE_p_unsigned_long_long isn't supported any more. In this diff, we are going to remove the unsupported type from Faiss swig.

Test Plan:
STEP 1: create a new conda env
```
conda create --name faiss_swig
conda activate faiss_swig
```

STEP 2: install dependecies from conda-forge
```
conda install -y -q python=3.11 cmake make swig mkl=2023 mkl-devel=2023 numpy scipy pytest gxx_linux-64 sysroot_linux-64=2.28 -c conda-forge
```

STEP 3: CMAKE

```
cmake -B build \                                                                                  [system]
      -DBUILD_TESTING=ON \
      -DBUILD_SHARED_LIBS=ON \
      -DFAISS_ENABLE_GPU=OFF \
      -DFAISS_ENABLE_RAFT=OFF \
      -DFAISS_OPT_LEVEL=avx512 \
      -DFAISS_ENABLE_C_API=ON \
      -DPYTHON_EXECUTABLE=$(which python) \
      -DCMAKE_BUILD_TYPE=Release \
      -DBLA_VENDOR=Intel10_64_dyn \
      -DCMAKE_CUDA_FLAGS="-gencode arch=compute_75,code=sm_75" \
      .
```

STEP 4: build
```
make -C build -j faiss && make -C build -j swigfaiss
```

/var/folders/n5/8sm28y7j7hl8w4xdtl7r_33w0000gn/T/TemporaryItems/NSIRD_screencaptureui_AjUh4J/Screenshot 2024-03-25 at 12.21.25 AM.png
junjieqi added a commit that referenced this issue Mar 26, 2024
Summary:
Currently, faiss can't build with swig version > 4.2.x. As the #3239 mentioned.  Swig removed the support for 32bit swig/swig@9fb3a49. So SWIGTYPE_p_unsigned_long_long isn't supported any more. In this diff, we are going to remove the unsupported type from Faiss swig.

Test Plan:
STEP 1: create a new conda env
```
conda create --name faiss_swig
conda activate faiss_swig
```

STEP 2: install dependecies from conda-forge
```
conda install -y -q python=3.11 cmake make swig mkl=2023 mkl-devel=2023 numpy scipy pytest gxx_linux-64 sysroot_linux-64=2.28 -c conda-forge
```

STEP 3: CMAKE

```
cmake -B build \                                                                                  [system]
      -DBUILD_TESTING=ON \
      -DBUILD_SHARED_LIBS=ON \
      -DFAISS_ENABLE_GPU=OFF \
      -DFAISS_ENABLE_RAFT=OFF \
      -DFAISS_OPT_LEVEL=avx512 \
      -DFAISS_ENABLE_C_API=ON \
      -DPYTHON_EXECUTABLE=$(which python) \
      -DCMAKE_BUILD_TYPE=Release \
      -DBLA_VENDOR=Intel10_64_dyn \
      -DCMAKE_CUDA_FLAGS="-gencode arch=compute_75,code=sm_75" \
      .
```

STEP 4: build
```
make -C build -j faiss && make -C build -j swigfaiss
```

/var/folders/n5/8sm28y7j7hl8w4xdtl7r_33w0000gn/T/TemporaryItems/NSIRD_screencaptureui_AjUh4J/Screenshot 2024-03-25 at 12.21.25 AM.png
facebook-github-bot pushed a commit that referenced this issue Mar 29, 2024
Summary:
Currently, faiss can't build with swig version > 4.2.x. As the #3239 mentioned.  Swig removed the support for 32bit swig/swig@9fb3a49. So SWIGTYPE_p_unsigned_long_long isn't supported any more. In this diff, we are going to remove the unsupported type from Faiss swig.

Pull Request resolved: #3315

Test Plan:
STEP 1: create a new conda env
```
conda create --name faiss_swig
conda activate faiss_swig
```

STEP 2: install dependecies from conda-forge
```
conda install -y -q python=3.11 cmake make swig mkl=2023 mkl-devel=2023 numpy scipy pytest gxx_linux-64 sysroot_linux-64=2.28 -c conda-forge
```

STEP 3: CMAKE

```
cmake -B build \
      -DBUILD_TESTING=ON \
      -DBUILD_SHARED_LIBS=ON \
      -DFAISS_ENABLE_GPU=OFF \
      -DFAISS_ENABLE_RAFT=OFF \
      -DFAISS_OPT_LEVEL=avx512 \
      -DFAISS_ENABLE_C_API=ON \
      -DPYTHON_EXECUTABLE=$(which python) \
      -DCMAKE_BUILD_TYPE=Release \
      -DBLA_VENDOR=Intel10_64_dyn \
      -DCMAKE_CUDA_FLAGS="-gencode arch=compute_75,code=sm_75" \
      .
```

STEP 4: build
```
make -C build -j faiss && make -C build -j swigfaiss
```
<img width="876" alt="Screenshot 2024-03-25 at 12 24 16 AM" src="https://github.com/facebookresearch/faiss/assets/8333898/918f0caf-398a-4361-989f-93ff547cf2b2">

Reviewed By: algoriddle

Differential Revision: D55304004

Pulled By: junjieqi

fbshipit-source-id: e958009dc637aa33b0e1a574a16a846a4abb1525
@junjieqi
Copy link
Contributor

PR merged #3315

@junjieqi junjieqi self-assigned this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants