Update default engine to FAISS #2221
Conversation
VijayanB
requested review from
heemin32,
navneet1v,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
ryanbogan,
luyuncheng and
shatejas
as code owners
VijayanB
force-pushed
the
update-default-engine
branch
2 times, most recently
from
e2d10a8
to
412d570
Compare
Since faiss supports more features than nmslib, and, we had seen data points that there are more number of vector search users are interesed in faiss, we will be updating default engine to be faiss. This will benefit users who preffered to use defaults while working with vector search. Signed-off-by: Vijayan Balasubramanian <[email protected]>
VijayanB
force-pushed
the
update-default-engine
branch
3 times, most recently
from
96aca39
to
97240f3
Compare
Signed-off-by: Vijayan Balasubramanian <[email protected]>
VijayanB
force-pushed
the
update-default-engine
branch
from
97240f3
to
daf89f6
Compare
| @@ -203,7 +203,7 @@ static KNNMethodContext createKNNMethodContextFromLegacy( | |||
| ? topLevelSpaceType | |||
| : KNNVectorFieldMapperUtil.getSpaceType(indexSettings); | |||
| return new KNNMethodContext( | |||
| KNNEngine.NMSLIB, | |||
There was a problem hiding this comment.
Keep as NMSLIB - nmslib is only legacy support
There was a problem hiding this comment.
maybe add comment to like I should have done haha
| @@ -29,7 +29,7 @@ public enum KNNEngine implements KNNLibrary { | |||
| FAISS(FAISS_NAME, Faiss.INSTANCE), | |||
| LUCENE(LUCENE_NAME, Lucene.INSTANCE); | |||
|
|
|||
| public static final KNNEngine DEFAULT = NMSLIB; | |||
| public static final KNNEngine DEFAULT = FAISS; | |||
There was a problem hiding this comment.
How we are handling the backward compatibility? Also can we validate if we have proper BWC tests to test this default engine change.
There was a problem hiding this comment.
I was just curious to know , why do we need to handle BWC in this case , So i created a Index , without specifying any engine so it picked NMSLIB , so now this information is already there in Cluster State and Serialised and passed to all Nodes, Now During query time , we do get it information from there and it does not depend on the new Default.
| @@ -114,7 +115,7 @@ public void testGetAllEngineFileContexts() throws IOException, ExecutionExceptio | |||
| engineFileContexts = knnIndexShard.getAllEngineFileContexts(searcher.getIndexReader()); | |||
| assertEquals(1, engineFileContexts.size()); | |||
| List<String> paths = engineFileContexts.stream().map(KNNIndexShard.EngineFileContext::getIndexPath).collect(Collectors.toList()); | |||
| assertTrue(paths.get(0).contains("hnsw") || paths.get(0).contains("hnswc")); | |||
| assertTrue(paths.get(0).contains(FAISS_HNSW_EXTENSION)); | |||
There was a problem hiding this comment.
is the compound extension covered?
| @@ -263,29 +263,6 @@ public void testByteVectorDataTypeWithNmslibEngine() { | |||
| assertTrue(ex.getMessage().contains("is not supported for vector data type")); | |||
| } | |||
|
|
|||
| @SneakyThrows | |||
| public void testByteVectorDataTypeWithLegacyFieldMapperKnnIndexSetting() { | |||
There was a problem hiding this comment.
Just checking if this case is covered when engine is explicitly NMSLIB
| @@ -146,7 +146,7 @@ public void testTypeParser_build_fromKnnMethodContext() throws IOException { | |||
| // Check that knnMethodContext takes precedent over both model and legacy | |||
| ModelDao modelDao = mock(ModelDao.class); | |||
|
|
|||
| SpaceType spaceType = SpaceType.COSINESIMIL; | |||
| SpaceType spaceType = SpaceType.DEFAULT; | |||
There was a problem hiding this comment.
Out of curiosity, why this change?
Description
Related Issues
#2163
Check List
--signoff.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.