Deprecate COSINE VectorSimilarity function #13308

Pulkitg64 · 2024-04-15T20:34:11Z

Description

Tries to solve #13281 by deprecating COSINE VectorSimilarity function for Vector Search.

Pulkitg64 · 2024-04-15T20:46:17Z

TestInt8HnswBackwardsCompatibility.java and TestBasicBackwardsCompatibility test cases failing for Lucene versions older than LUCENE_10_0_0. For older Lucene version, we try to read Index directory from resources folder. For example for Lucene-9.10.0, the directory path is located at lucene/backward-codecs/src/test/org/apache/lucene/backward_index/index.9.10.0-cfs.zip.

As per my understanding the segment present at above directory already has KNNVectorField with MAXIMUM_INNER_PRODUCT as vector similarity function and in above test we try to change similarity function to DOT_PRODUCT due to which there is conflict happening when adding more docs to it and hence these test cases failing.

lucene/core/src/java/org/apache/lucene/index/VectorSimilarityFunction.java

jpountz · 2024-04-17T20:47:32Z

Oh okay, that means we need to remove support for COSINE just from indexing side not from searching side?

Correct. Practically, this means keeping the enum constant but marking it as deprecated, and modifying IndexingChain to fail using COSINE on 10.x Lucene indexes.

It would be nice to also add a new file format under lucene/codecs that does cosine similarity, like we just added for hamming distance. This could help with the migration for some users.

Pulkitg64 · 2024-04-18T07:03:33Z

Thanks @jpountz for confirming and suggestions.

uschindler · 2024-04-19T11:47:27Z

lucene/core/src/java/org/apache/lucene/codecs/lucene94/Lucene94FieldInfosFormat.java

@@ -302,7 +301,6 @@ private static VectorSimilarityFunction getDistFunc(IndexInput input, byte b) th
 List.of(
 VectorSimilarityFunction.EUCLIDEAN,
 VectorSimilarityFunction.DOT_PRODUCT,
- VectorSimilarityFunction.COSINE,


Here is exactly the problem. There must be a null entry, so the ordinals in index format stay alive.

++ The reason for the list here is exactly to support and allow such evolution and separation from the enum ordinals. A null here trivially need to a change of List implementation type. And tests need to be carefully updated.

of course, null does not work with default lists :-) Also downstream the code needs to check for a null value.

In any case we can later also go back to a Map<Integer,VectorSimilarity> or to an array.

Thanks @uschindler @ChrisHegarty for the pointers, but IIUC, we cannot add a null entry to VectorSimilarityFunction map for COSINE in these files because we still want to use COSINE similarity function for Lucene versions < 10.0, right? Instead we need to create a separate file for new versions. Please correct me if I am wrong here.

benwtrent

Left a larger review.

I think it would be prudent to first attempt to deprecate and handle internal usage, etc. After that is merged and backported, another PR can be made to fully remove it from main only on writes.

At least, that would make reviewing & testing this simpler :)

benwtrent · 2024-04-22T21:37:44Z

...ckward-codecs/src/test/org/apache/lucene/backward_index/TestBasicBackwardsCompatibility.java

@@ -98,8 +99,14 @@ public class TestBasicBackwardsCompatibility extends BackwardsCompatibilityTestB
 private static final int KNN_VECTOR_MIN_SUPPORTED_VERSION = LUCENE_9_0_0.major;
 private static final String KNN_VECTOR_FIELD = "knn_field";
 private static final FieldType KNN_VECTOR_FIELD_TYPE =
- KnnFloatVectorField.createFieldType(3, VectorSimilarityFunction.COSINE);
+ KnnFloatVectorField.createFieldType(3, VectorSimilarityFunction.DOT_PRODUCT);


I am not 100% sure this will work as simply as this. We will index NEW fields into an index created with the OLD code (see where we call addDoc), meaning you will be trying to index a dot_product knn field into an index with a cosine field.

You will have to handle the index versions from before a certain version (thus on cosine) to ones created after (and thus dot-product).

Make sense.

benwtrent · 2024-04-22T21:38:13Z

...ard-codecs/src/test/org/apache/lucene/backward_index/TestInt8HnswBackwardsCompatibility.java

 private static final float[] KNN_VECTOR = {0.2f, -0.1f, 0.1f};
+ private static final float[] NORMALIZED_KNN_VECTOR = new float[KNN_VECTOR.length];


Same sort of comment as above on versioning and that we index fields into an old index.

benwtrent · 2024-04-22T22:14:14Z

lucene/core/src/java/org/apache/lucene/index/VectorSimilarityFunction.java

@@ -61,24 +60,6 @@ public float compare(byte[] v1, byte[] v2) {
 }
 },

- /**
- * Cosine similarity. NOTE: the preferred way to perform cosine similarity is to normalize all


I think it would be good to deprecate this first, get all the internal paths updated to not use it.

Then the deprecation can be merged and backported. Then work on removing it from main.

Mostly because the deprecation path and internal code usage will all have to occur anyways :)

benwtrent · 2024-04-22T22:15:27Z

lucene/core/src/java/org/apache/lucene/util/quantization/ScalarQuantizedRandomVectorScorer.java

@@ -30,23 +28,6 @@
 public class ScalarQuantizedRandomVectorScorer
 extends RandomVectorScorer.AbstractRandomVectorScorer<byte[]> {

- public static float quantizeQuery(


Even in Lucene 10, we will need to support quantizing a query sent to a Lucene index that uses Cosine.

benwtrent · 2024-04-22T22:19:19Z

lucene/core/src/java/org/apache/lucene/index/VectorSimilarityFunction.java

- return (1 + cosine(v1, v2)) / 2;
- }
- },
-


I think this will get tricky. If you remove this ordinal (I think it would be wise to deprecate first, work through the warnings, etc. get everything passing, etc.), field info & knn vector format readers will need to somehow distinguish between "Max inner product" (whose ordinal decreases to cosine's) and "cosine but in an old index version".

Maybe on write, we adjust the ordinal dynamically, knowing what they are (thus correcting MIP's ordinal on write, allowing for readers to distinguish).

Pulkitg64 · 2024-05-06T11:20:22Z

Thanks @benwtrent for all the feedback.
I have raised another revision just to mark the COSINE function as deprecated. I didn't find any internal usages of this function.
In a followup PR, I will remove COSINE from writes for LUCENE version >=10.0.0

benwtrent · 2024-05-08T12:20:11Z

I didn't find any internal usages of this function.

That doesn't make sense to me. IntelliJ tells me there are over 30 usages of VectorSimilarityFunction.COSINE.

Pulkitg64 · 2024-05-08T13:44:29Z

I didn't find any internal usages of this function.

That doesn't make sense to me. IntelliJ tells me there are over 30 usages of VectorSimilarityFunction.COSINE.

Sorry I think I am missing something, I think we cannot remove COSINE from those places, since we require them in LUCENE version < 10.0. They are mostly being used in FieldInfosFormat, VectorReader, ScalarQuantizedVectorWriter classes.

I think the main task is to first get rid of dependency on enum for VectorSimilarityFunction and instead have some interface to get those functions and support.

benwtrent · 2024-05-08T13:51:25Z

@Pulkitg64 could you deprecate also VectorUtil.cosine? That is technically a public API.

benwtrent · 2024-05-09T13:30:22Z

lucene/CHANGES.txt

+* GITHUB#13281: Mark COSINE VectorSimilarityFunction as deprecated. (Pulkit Gupta)
+


This should be under 9.11 as we will backport the deprecation as a way to warn users that it is going away in 10.0

Yes, Make sense. Will change in next revision
Thanks for mentioning.

ChrisHegarty

LGTM, though you it would be good get another reviewers approval too.

benwtrent requested changes Apr 16, 2024

View reviewed changes

lucene/core/src/java/org/apache/lucene/index/VectorSimilarityFunction.java Outdated Show resolved Hide resolved

uschindler reviewed Apr 19, 2024

View reviewed changes

benwtrent reviewed Apr 22, 2024

View reviewed changes

Mark COSINE Vector Similarity Function as Deprecated

589d1e2

Pulkitg64 force-pushed the cosine-deprecate branch from d500c89 to 589d1e2 Compare May 6, 2024 10:07

tidy fixes

3f23885

Pulkitg64 requested a review from benwtrent May 7, 2024 17:26

Added entry to CHANGES.txt

0c0c35b

Mark VectorUtil cosine function as deprecated

821f775

benwtrent reviewed May 9, 2024

View reviewed changes

Move change from 10.0 to 9.11.0

6ca2e43

ChrisHegarty approved these changes May 16, 2024

View reviewed changes

Pulkitg64 requested a review from benwtrent May 16, 2024 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate COSINE VectorSimilarity function #13308

Deprecate COSINE VectorSimilarity function #13308

Pulkitg64 commented Apr 15, 2024

Pulkitg64 commented Apr 15, 2024 •

edited

jpountz commented Apr 17, 2024

Pulkitg64 commented Apr 18, 2024

uschindler Apr 19, 2024

ChrisHegarty Apr 19, 2024

uschindler Apr 19, 2024

Pulkitg64 Apr 21, 2024 •

edited

benwtrent left a comment

benwtrent Apr 22, 2024

Pulkitg64 Apr 26, 2024

benwtrent Apr 22, 2024

benwtrent Apr 22, 2024

benwtrent Apr 22, 2024

benwtrent Apr 22, 2024

Pulkitg64 commented May 6, 2024

benwtrent commented May 8, 2024

Pulkitg64 commented May 8, 2024

benwtrent commented May 8, 2024

benwtrent May 9, 2024

Pulkitg64 May 9, 2024

Pulkitg64 May 16, 2024

ChrisHegarty left a comment

		private static final float[] KNN_VECTOR = {0.2f, -0.1f, 0.1f};
		private static final float[] NORMALIZED_KNN_VECTOR = new float[KNN_VECTOR.length];

		* GITHUB#13281: Mark COSINE VectorSimilarityFunction as deprecated. (Pulkit Gupta)

Deprecate COSINE VectorSimilarity function #13308

Are you sure you want to change the base?

Deprecate COSINE VectorSimilarity function #13308

Conversation

Pulkitg64 commented Apr 15, 2024

Description

Pulkitg64 commented Apr 15, 2024 • edited

jpountz commented Apr 17, 2024

Pulkitg64 commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pulkitg64 Apr 21, 2024 • edited

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pulkitg64 commented May 6, 2024

benwtrent commented May 8, 2024

Pulkitg64 commented May 8, 2024

benwtrent commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty left a comment

Choose a reason for hiding this comment

Pulkitg64 commented Apr 15, 2024 •

edited

Pulkitg64 Apr 21, 2024 •

edited