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

[Backport 2.x] Update Faiss engine to allow PQ and HNSW #1076

Merged

Conversation

jmazanec15
Copy link
Member

Backport #1074 to 2.x

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Updates faiss engine to enable hnsw and faiss to work together. For
HNSW, code_size must be equal to 8 (refer to
facebookresearch/faiss#3027). Therefore, the
index description string "HNSW32,PQXxY" does not work. Only "HNSW32,PQX"
ends up working.

Additionally, adds several unit tests and integration tests in order to
validate the functionality.

Signed-off-by: John Mazanec <[email protected]>
@jmazanec15 jmazanec15 force-pushed the backport/backport-1074-to-2.x branch from f0dfb8d to 2439a8a Compare August 31, 2023 03:45
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #1076 (1210cc1) into 2.x (f7b4415) will decrease coverage by 0.04%.
The diff coverage is 74.46%.

@@             Coverage Diff              @@
##                2.x    #1076      +/-   ##
============================================
- Coverage     85.09%   85.06%   -0.04%     
- Complexity     1159     1162       +3     
============================================
  Files           154      154              
  Lines          4730     4753      +23     
  Branches        434      434              
============================================
+ Hits           4025     4043      +18     
- Misses          510      514       +4     
- Partials        195      196       +1     
Files Changed Coverage Δ
...main/java/org/opensearch/knn/index/util/Faiss.java 78.51% <74.46%> (+3.00%) ⬆️

... and 1 file with indirect coverage changes

@jmazanec15
Copy link
Member Author

Mac:

    {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: Model with id=\"test-model-id\" already exists;"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: Model with id=\"test-model-id\" already exists;"},"status":400}

Seems to be a flaky test ID as I did not create one with "test-model-id" in this PR.

@jmazanec15
Copy link
Member Author

Above has an existing flaky GH issue: #888

@jmazanec15 jmazanec15 merged commit d8c5c47 into opensearch-project:2.x Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants