Re-Call Issue Fix with Binary Quantized Vectors #2071

Vikasht34 · 2024-09-08T04:12:21Z

Description

This PR addresses a critical issue that was identified during benchmarking, where the recall performance unexpectedly dropped below 1. The root cause of the issue was traced to two main problems in the quantization and vector handling process:

Bit Packing of Quantized Vector: The quantized vector was not correctly updated during the bit-packing process. Instead, the same vector values were being reused, resulting in incorrect quantization of subsequent vectors.
Vector Transfer to JNI: When transferring vectors to the JNI layer, vectors were passed by reference, causing all the vectors in the VectorTransfer list to reference the same object. This led to unintended behavior where all vectors became identical, severely affecting the recall accuracy.

Testing

Performed Benchmarking with NQ Data Set Results are here s3://disk-based-ann-bq/NQ-1M-768/
Re-Call for One Bit Quantization with 2x Oversampling us 0.94

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.

Signed-off-by: VIKASH TIWARI <[email protected]>

navneet1v

Overall code looks good to me. There are some minor comments. See how you want to resolve it.

Apart from this I would recommend doing 2 things:

Update the description of the PR with details on what are the bugs which was found.
Also add the details on how you tested these, like if you have ran a benchmarks to see the recall add that too.

...java/org/opensearch/knn/quantization/models/quantizationOutput/BinaryQuantizationOutput.java

src/main/java/org/opensearch/knn/index/codec/nativeindex/QuantizationIndexUtils.java

ryanbogan

One minor nit but not blocking. Otherwise, LGTM!

ryanbogan

Looks like there are some merge conflicts that need to be fixed @Vikasht34

Signed-off-by: VIKASH TIWARI <[email protected]>

Signed-off-by: Vikasht34 <[email protected]>

* Re-Call Issue Fix with Binary Quantized Vectors Signed-off-by: VIKASH TIWARI <[email protected]> * Feedback Fix Signed-off-by: VIKASH TIWARI <[email protected]> --------- Signed-off-by: VIKASH TIWARI <[email protected]> Signed-off-by: Vikasht34 <[email protected]> (cherry picked from commit ce735c4)

shatejas · 2024-09-09T17:55:59Z

src/main/java/org/opensearch/knn/quantization/models/quantizationOutput/QuantizationOutput.java

@@ -33,4 +58,11 @@ public interface QuantizationOutput<T> {
     * @return true if the quantized vector is already prepared, false otherwise.
     */
    boolean isPrepared(int vectorLength);


nit: We should be removing this as its a dead code at this point, we can add it back if we ever need it in the future

shatejas · 2024-09-09T17:58:18Z

src/test/java/org/opensearch/knn/quantization/output/BinaryQuantizationOutputTests.java

+        quantizationOutput.prepareQuantizedVector(vectorLength);
+
+        // Assert
+        assertNotNull(quantizationOutput.getQuantizedVector());


Not null isn't tight enough. Can you check if the length is 10 and its all 0?

shatejas · 2024-09-09T18:00:14Z

src/test/java/org/opensearch/knn/quantization/output/BinaryQuantizationOutputTests.java

+    @Before
+    public void setUp() throws Exception {
+        super.setUp();
+        quantizationOutput = new BinaryQuantizationOutput(BITS_PER_COORDINATE);


This member variable seems to be creating interdependency between tests, can we create this for every test please

shatejas · 2024-09-09T18:08:02Z

src/main/java/org/opensearch/knn/index/codec/nativeindex/QuantizationIndexUtils.java

+             * Returns a copy of the quantized vector. This is because of during transfer same vectors was getting
+             * added due to reference.
+             */
+            return indexBuildSetup.getQuantizationOutput().getQuantizedVectorCopy();


Can we update the unit test for this class to make sure we always return a copy? we need a assertNotSame check in the unit test of this class

* Re-Call Issue Fix with Binary Quantized Vectors Signed-off-by: VIKASH TIWARI <[email protected]> * Feedback Fix Signed-off-by: VIKASH TIWARI <[email protected]> --------- Signed-off-by: VIKASH TIWARI <[email protected]> Signed-off-by: Vikasht34 <[email protected]> (cherry picked from commit ce735c4) Co-authored-by: Vikasht34 <[email protected]>

Re-Call Issue Fix with Binary Quantized Vectors

078c522

Signed-off-by: VIKASH TIWARI <[email protected]>

Vikasht34 requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners September 8, 2024 04:12

navneet1v added the skip-changelog label Sep 8, 2024

navneet1v reviewed Sep 8, 2024

View reviewed changes

navneet1v added backport 2.x backport 2.17 labels Sep 8, 2024

Vikasht34 requested a review from navneet1v September 8, 2024 05:17

Vikasht34 force-pushed the FinalMain branch 2 times, most recently from b6e6ed1 to a390c1d Compare September 9, 2024 01:14

ryanbogan reviewed Sep 9, 2024

View reviewed changes

src/main/java/org/opensearch/knn/index/codec/nativeindex/QuantizationIndexUtils.java Outdated Show resolved Hide resolved

ryanbogan previously approved these changes Sep 9, 2024

View reviewed changes

ryanbogan reviewed Sep 9, 2024

View reviewed changes

Vikasht34 dismissed ryanbogan’s stale review via f90a221 September 9, 2024 16:46

ryanbogan previously approved these changes Sep 9, 2024

View reviewed changes

Feedback Fix

ddfa66f

Signed-off-by: VIKASH TIWARI <[email protected]>

Vikasht34 dismissed ryanbogan’s stale review via ddfa66f September 9, 2024 16:47

Vikasht34 force-pushed the FinalMain branch from f90a221 to ddfa66f Compare September 9, 2024 16:47

ryanbogan previously approved these changes Sep 9, 2024

View reviewed changes

Vikasht34 dismissed ryanbogan’s stale review via ddfa66f September 9, 2024 16:52

Vikasht34 force-pushed the FinalMain branch from 9a7b97e to ddfa66f Compare September 9, 2024 16:52

Merge branch 'main' into FinalMain

cb394e0

Signed-off-by: Vikasht34 <[email protected]>

ryanbogan approved these changes Sep 9, 2024

View reviewed changes

navneet1v approved these changes Sep 9, 2024

View reviewed changes

navneet1v merged commit ce735c4 into opensearch-project:main Sep 9, 2024
28 of 29 checks passed

This was referenced Sep 9, 2024

[Backport 2.x] Re-Call Issue Fix with Binary Quantized Vectors #2078

Merged

[Backport 2.17] Re-Call Issue Fix with Binary Quantized Vectors #2079

Merged

shatejas reviewed Sep 9, 2024

View reviewed changes

naveentatikonda pushed a commit that referenced this pull request Sep 9, 2024

Re-Call Issue Fix with Binary Quantized Vectors (#2071) (#2078)

ed5d7d1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-Call Issue Fix with Binary Quantized Vectors #2071

Re-Call Issue Fix with Binary Quantized Vectors #2071

Vikasht34 commented Sep 8, 2024 •

edited

Loading

navneet1v left a comment

ryanbogan left a comment

ryanbogan left a comment

shatejas Sep 9, 2024

shatejas Sep 9, 2024

shatejas Sep 9, 2024

shatejas Sep 9, 2024

Re-Call Issue Fix with Binary Quantized Vectors #2071

Re-Call Issue Fix with Binary Quantized Vectors #2071

Conversation

Vikasht34 commented Sep 8, 2024 • edited Loading

Description

Testing

Check List

navneet1v left a comment

Choose a reason for hiding this comment

ryanbogan left a comment

Choose a reason for hiding this comment

ryanbogan left a comment

Choose a reason for hiding this comment

shatejas Sep 9, 2024

Choose a reason for hiding this comment

shatejas Sep 9, 2024

Choose a reason for hiding this comment

shatejas Sep 9, 2024

Choose a reason for hiding this comment

shatejas Sep 9, 2024

Choose a reason for hiding this comment

Vikasht34 commented Sep 8, 2024 •

edited

Loading