[LuceneOnFaiss - Part1] Added building blocks for memory optimized search.

[0ctopus13prime]

Description

[1/11] This is the first PR to establish the building blocks for memory optimized search.
At the moment, FAISS is the only engine that supports memory optimized search where loading vectors in demand fashion.

Note that this will be merged into the feature branch first. Unit tests + IT tests will be covered in PR-9 and PR-10.

Related Issues

RFC : #2401

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[navneet1v]

@0ctopus13prime please fix the CIs too

[shatejas]

I am not really sure we should keep adding implementation details to the enum. Shouldn't a simple engine check inside the factory take care of this?

[0ctopus13prime]

I think each Engine either supports or does not support memory_optimized_search mode.
So far, FAISS is the single engine that supports the mode but it can be expanded to other future engines.
From that perspective, it is natural to put factory-getter method in Engine.

What is your concern?

[jmazanec15]

It will be per engine. Ideally, it would be included in something like https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/engine/KNNLibrarySearchContext.java. This can be retrieved via https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/engine/KNNLibrary.java#L121.

[shatejas]

My problem with these enums is that its turning into giant static configuration database, along with a workaround to have dependency injection. Its a maintenance concern- It doesn't seem to be a standard coding pattern and I am not sure its the right way to go

[jmazanec15]

Shouldn't a simple engine check inside the factory take care of this?

I dont think we should branch throughout the code based on engine. This will make engine extendability very difficult and also will created complex branching in the code - this has happened to some degree based in the query.

The purpose of KNNLibrarySearchContext was to let the given engine tell how we are supposed to search. This isnt totally complete, but the direction to kind of further improve this is in #2568. On the indexing/reader setup side, we would need the engines to return a PerFieldKnnVectorFormat that can be added via https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/codec/KNNCodecService.java#L43.

[navneet1v]

@0ctopus13prime merging the thread from here #2581 (comment) too. and also adding my thoughts from the discussion happening on this thread.

@jmazanec15, the idea of engine providing a PerFieldKnnVectorFormat based on the MappedfieldType is a step in right direction. But once we have the KNNVectorFormat which is engine specific then searcher should not come from KNNEngine. It should be job of Reader to find the right searcher for the algorithm.

We moving searcher currently in KNNEngine, I somehow feel a decision we are taking too early. Should we take this decision once we move to PerFieldKnnVectorFormat being returned via KNNEngine?

[0ctopus13prime]

If we could align on having a dedicated FAISS VectorFormat, then will update accordingly in the next rev.
I will add a branch in BasePerFieldKnnVectorsFormat to return FaissVectorFormat returning a reader where it performs vector search on FAISS index for knn fields using FAISS engine.

// In BasePerFieldKnnVectorsFormat
if (engine == FAISS) {
  return new FaissKnnVectorsFormat(...);
}

[jmazanec15]

My concern is that the engine/method will determine if the optimized memory searcher is supported and how its supported. In the current code, Im not sure Im seeing where the branching based on method logic is implemented - for instance, where would we say HNSW fp16 is not supported or IVF is not supported (I might just be missing it). But I think this can get fairly complex, and is why I think that it would be better to go through the engine that determines what functionality is supported.

So we could just add a method in KNNLibrarySearchContext like "getMemoryOptimizedSearcherFactory()". Then, we can retrieve it via knnEngine.getLibrarySearchContext(methodName) (we might need to take additional info like library params like what @owenhalpert is working on). With this, we keep the codec somewhat engine agnostic.

[Vikasht34]

My concern is that the engine/method will determine if the optimized memory searcher is supported and how its supported. In the current code, Im not sure Im seeing where the branching based on method logic is implemented - for instance, where would we say HNSW fp16 is not supported or IVF is not supported (I might just be missing it). But I think this can get fairly complex, and is why I think that it would be better to go through the engine that determines what functionality is supported.

So we could just add a method in KNNLibrarySearchContext like "getMemoryOptimizedSearcherFactory()". Then, we can retrieve it via knnEngine.getLibrarySearchContext(methodName) (we might need to take additional info like library params like what @owenhalpert is working on). With this, we keep the codec somewhat engine agnostic.

This seems reasonable to me !!

[0ctopus13prime]

@0ctopus13prime please fix the CIs too

Hmm.. this PR should not affect anything at all 😅
Let me check, suspecting a flaky test

[navneet1v]

@0ctopus13prime please fix the CIs too

Hmm.. this PR should not affect anything at all 😅 Let me check, suspecting a flaky test

I also think so, but its always better to see build CIs passing and also DCO checks. This will ensure that branch is healthy

[0ctopus13prime]

Please note that once get verbal approval on the implementation, then will ship unit tests in this PR.
Thanks.

[shatejas]

Why not log4j?

[0ctopus13prime]

Slf4j is a general logging framework without tied dependency over a specific logger framework like Log4j, Logback etc. It's a facade for different logging frameworks. When there's a log framework upgrade or change, it won't need any changes

[navneet1v]

let just use what we are using in the plugin to avoid conflicts for future.

[0ctopus13prime]

why it conflicts? opensearch core is using slf4j with log4j

[navneet1v]

When I say conflict, I mean mainly if someone trying add a logger they will be confused whether to use slf4j or log4j. So people will have conflicts in mind what to pick. It mainly about consistency in the code. There is no specific reason from my side, for me its all about consistency in the code.

[0ctopus13prime]

sounds good, will update in the next rev.
before raising a new PR, could you share your thoughts on the code?
If you leave comments, I will factor them into the next PR.

[Vikasht34]

I think we should start using Slf4j and clean code by replacing log4j to Slf4j !! There are couple of benefits we have 1. We are aligning to core and Lucene , Lucene also uses Slf4j , and in future either if core replaces that with any other inline framework !! We get for free !!

[navneet1v]

in that case it should a be part of a separate GH and not scope of this PR.

[0ctopus13prime]

Will update the annotation in the next rev! :)

[0ctopus13prime]

@jmazanec15
For those index types that don't support memory optimized searching, FaissIndex.load (which will be added in the next PR) will throw an exception. In vector reader, it catches it then moving on to the next field.
And in the index settings update event listener, it will make sure to allow it to be turned on ONLY IF all KNN fields have FAISS HNSW. Also the flag check in KnnQueryBuilder will make sure other index types but HNSW will be fallbacked to NativeQuery which will use native shared library to continue the search.