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

Interop between CAGRA and HNSW #3252

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

divyegala
Copy link
Contributor

Depends on #3084

@divyegala
Copy link
Contributor Author

@mdouze I addressed your first batch of reviews. Could you re-review when it is convenient for you?

Copy link
Contributor

@mdouze mdouze left a comment

Choose a reason for hiding this comment

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

What is the level of parity between GPU search and CPU search? The search function on CPU does not have access to the Cagra search parameters.

Could you write a few python tests taking inspiration from

https://github.com/facebookresearch/faiss/blob/main/faiss/gpu/test/test_contrib_gpu.py

I would like to see:

  • build and search on GPU

  • index_gpu_to_cpu on a Cagra graph

  • serialization / deserialization

  • search on CPU

Thanks!

@@ -46,6 +46,8 @@ project(faiss
LANGUAGES ${FAISS_LANGUAGES})
include(GNUInstallDirs)

set(CMAKE_INSTALL_PREFIX "$ENV{CONDA_PREFIX}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we want to keep this as Faiss is not necessarily compiled in a conda env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. This was just for development purposes - I will remove it.

@divyegala
Copy link
Contributor Author

@mdouze thanks for your review! Btw, the C++ tests cover all the tests you mentioned except serialization. Nonetheless, I will definitely add python tests per your suggestion.

@divyegala
Copy link
Contributor Author

@mdouze I seem to be unable to build faiss C++ tests after merging some recent commits into my branch. This is the error I get, can you help? I have tried deleting build/cache and building again:

/home/nfs/dgala/faiss/build/tests/faiss_test: symbol lookup error: /home/nfs/dgala/faiss/build/tests/faiss_test: undefined symbol: _ZN5faiss15TimeoutCallback5resetEd

@Di-Is
Copy link

Di-Is commented May 21, 2024

Hi @divyegala!

Can you share the steps to reproduce the error?
I have built working branch in my environment and no error occurred.

@divyegala
Copy link
Contributor Author

@Di-Is I figured out what the issue is. I build FAISS in a conda environment, and re-configuring FAISS CMake does not overwrite the FAISS header files that get copied the very first time when I configure FAISS CMake. As a consequence, any time there's changes to an API or new APIs are added, the header files in the conda environment become stale.

@divyegala
Copy link
Contributor Author

@mdouze I have added Python tests, could you review when possible?

@mdouze
Copy link
Contributor

mdouze commented May 30, 2024

Thanks! Importing to the internal repo.

@facebook-github-bot
Copy link
Contributor

@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

None yet

4 participants