-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Remove conditional import of FAISS for Windows #819
Conversation
5734ad8
to
8d7d9be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a quick test in a Windows VM. Installation of 1.6.3 via pip still seems to crash:
Complete output (38 lines):
running install
running build
running build_ext
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "C:\Users\PietschMalte\AppData\Local\Temp\pip-install-iue3tn7s\faiss-cpu\setup.py", line 87, in <module>
setup(
File "C:\Users\PietschMalte\Miniconda3\lib\site-packages\setuptools\__init__.py", line 144, in setup
return distutils.core.setup(**attrs)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\core.py", line 148, in setup
dist.run_commands()
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\dist.py", line 966, in run_commands
self.run_command(cmd)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\dist.py", line 985, in run_command
cmd_obj.run()
File "C:\Users\PietschMalte\Miniconda3\lib\site-packages\setuptools\command\install.py", line 61, in run
return orig.install.run(self)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\command\install.py", line 545, in run
self.run_command('build')
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\cmd.py", line 313, in run_command
self.distribution.run_command(command)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\dist.py", line 985, in run_command
cmd_obj.run()
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\command\build.py", line 135, in run
self.run_command(cmd_name)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\cmd.py", line 313, in run_command
self.distribution.run_command(command)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\dist.py", line 985, in run_command
cmd_obj.run()
File "C:\Users\PietschMalte\AppData\Local\Temp\pip-install-iue3tn7s\faiss-cpu\setup.py", line 45, in run
build_ext.run(self)
File "C:\Users\PietschMalte\Miniconda3\lib\distutils\command\build_ext.py", line 340, in run
self.build_extensions()
File "C:\Users\PietschMalte\AppData\Local\Temp\pip-install-iue3tn7s\faiss-cpu\setup.py", line 50, in build_extensions
self._remove_flag('-Wstrict-prototypes')
File "C:\Users\PietschMalte\AppData\Local\Temp\pip-install-iue3tn7s\faiss-cpu\setup.py", line 58, in _remove_flag
compiler = self.compiler.compiler
AttributeError: 'MSVCCompiler' object has no attribute 'compiler'
----------------------------------------
1.6.5 and 1.7.0 seem to work (at least installation succeeded).
I would propose to update to faiss 1.7.0 and test if everything in Haystack is still compatible
Actually we can use 1.7.0 which have latest changes. Also soon they will support M1 Mac support |
Sorry I missed reading @tholor comment |
@tanaysoni we'll need FAISS benchmarks here (which are not included yet in the CI runs). I can take care of this |
Benchmarks look okay. mAP is similar. speed is a bit slower but can be due to smaller random fluctuations. I think the benefit of moving to the latest FAISS version is big enough to justify merging. FAISS Benchmarks for this branch:
Last run on master:
|
Since FAISS is now supported on Windows, we can remove the conditional import of
faiss
in theFAISSDocumentDocumentStore
.Resolves #817