FEAT: support cosine similarity #90

chenqi0805 · 2020-04-12T23:28:25Z

Issue #, if available:
#28

Description of changes:

Add "index.knn.space_type" as string param into KNNSettings and use validator for fast fail
Pass in spaceType as String into jni functions (java/c++)
Update vector field mapper to correctly parse the spaceType setting param
Adapt KNNIndexCache
Adapt existing test cases

TODO

check if "index.knn.space_type" should be reset or not once a index is created
More test cases to cover cosine similarity
update README

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jmazanec15

Thanks for working on this! Added a couple comments. Additionally, nmslib submodule should not be altered.

jni/src/v1736/com_amazon_opendistroforelasticsearch_knn_index_v1736_KNNIndex.cpp

jmazanec15 · 2020-04-13T21:30:29Z

src/test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNJNITestsIT.java

@@ -31,8 +31,8 @@
 import java.util.Map;
 import java.util.stream.Collectors;

-public class KNNJNITests extends ESTestCase {
-    private static final Logger logger = LogManager.getLogger(KNNJNITests.class);
+public class KNNJNITestsIT extends ESTestCase {


Why switch this?

Ah. When I ran the test locally on IntelliJ, it fails to detect the integration tests therein due to lack of suffix "IT" in the class name. So I switched that for easier debugging. Will change it back but I think it might worth doublecheck if CI also neglects the tests.

Tests as opposed to IT signals that it is a unit test. These tests run during ./gradlew build, which is what we use for the PR testing workflow. You can also run them individually with this command:

./gradlew ':test' --tests "com.amazon.opendistroforelasticsearch.knn.index.KNNJNITests.testCreateHnswIndex"

We consider these unit tests because they are testing an isolated portion of code. Interesting that IntelliJ does not pick them up. Will need to look into it.

Ah. Sorry, I misidentified it as IT.

jmazanec15 · 2020-04-14T18:18:42Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNSettings.java

+        private Set<String> types = SpaceTypes.getValues();
+
+        @Override public void validate(String value) {
+            if (!types.contains(value.toLowerCase())){


Why is this case insensitive?

I intended to set it case insensitive, e.g. L2, Cosinesimil is also valid string. But I can remove this if not necessary.

I think case insensitive is good

jmazanec15 · 2020-04-14T18:21:29Z

src/test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNJNITests.java

@@ -165,7 +214,7 @@ public Void run() {
        float[] queryVector = {1.0f, 1.0f, 1.0f, 1.0f};
        String[] algoQueryParams = {"efSearch=200"};

-        final KNNIndex index = KNNIndex.loadIndex(indexPath, algoQueryParams);
+        final KNNIndex index = KNNIndex.loadIndex(indexPath, "l2", algoQueryParams);
        final KNNQueryResult[] results = index.queryIndex(queryVector, 30);

        Map<Integer, Float> scores = Arrays.stream(results).collect(


Could you add test cases to ensure that invalid space names throw the expected exceptions?

Yes. My initial thought was to add such tests at KNNSettings module b/c that is where I want the input value to be validated and fail fast. Do you think it is also necessary to prevent invalid spaceType into JNI functions?

Added an integration test in KNNESSettingsIT.

Makes sense thanks

jmazanec15 · 2020-04-14T18:35:31Z

src/test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNJNITestsIT.java

@@ -31,8 +31,8 @@
 import java.util.Map;
 import java.util.stream.Collectors;

-public class KNNJNITests extends ESTestCase {
-    private static final Logger logger = LogManager.getLogger(KNNJNITests.class);
+public class KNNJNITestsIT extends ESTestCase {


Tests as opposed to IT signals that it is a unit test. These tests run during ./gradlew build, which is what we use for the PR testing workflow. You can also run them individually with this command:

./gradlew ':test' --tests "com.amazon.opendistroforelasticsearch.knn.index.KNNJNITests.testCreateHnswIndex"

We consider these unit tests because they are testing an isolated portion of code. Interesting that IntelliJ does not pick them up. Will need to look into it.

…N into feat/28-cosine-similarity * 'feat/28-cosine-similarity' of github.com:chenqi0805/k-NN: Compile .so on linux

chenqi0805 · 2020-04-15T03:41:52Z

@jmazanec15 I have added one more test coverage on invalid spaceType setting and also updated README. The only concern left is whether spaceType setting is allowed to be dynamic or not. I think this depends on whether the KNN index setting parameters are embedded into fields and docs when stored, which I am not sure. Do you have idea here? Besides that, this work is now complete.

vamshin · 2020-04-15T06:10:21Z

@jmazanec15 I have added one more test coverage on invalid spaceType setting and also updated README. The only concern left is whether spaceType setting is allowed to be dynamic or not. I think this depends on whether the KNN index setting parameters are embedded into fields and docs when stored, which I am not sure. Do you have idea here? Besides that, this work is now complete.

spaceType setting should be static for an index to lead consistent results otherwise segments in the shards would be scattered with different spaces and queries would lead inconsistent results

vamshin · 2020-04-15T07:03:13Z

jni/src/v1736/com_amazon_opendistroforelasticsearch_knn_index_v1736_KNNIndex.cpp

@@ -85,15 +85,19 @@ void catch_cpp_exception_and_throw_java(JNIEnv* env)
    }
 }

-JNIEXPORT void JNICALL Java_com_amazon_opendistroforelasticsearch_knn_index_v1736_KNNIndex_saveIndex(JNIEnv* env, jclass cls, jintArray ids, jobjectArray vectors, jstring indexPath, jobjectArray algoParams)
+JNIEXPORT void JNICALL Java_com_amazon_opendistroforelasticsearch_knn_index_v1736_KNNIndex_saveIndex(JNIEnv* env, jclass cls, jintArray ids, jobjectArray vectors, jstring indexPath, jstring spaceType, jobjectArray algoParams)


nit: Please add the new function arguments to the end of the existing parameters list. Please take care in other places as well.

vamshin · 2020-04-15T07:19:27Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNSettings.java

+
+        private Set<String> types = SpaceTypes.getValues();
+
+        @Override public void validate(String value) {


null check on value

@vamshin I put a null check here, but it still fails to guard against null value, i.e. for some reason, if index.knn.space_type == null, it will not go through the validate. Similar bug happens to other parameters, e.g.

PUT /myindex { "settings" : { "index": { "knn": true, "knn.algo_param.m": null } } }

PUT /myindex/_doc/2?refresh=true { "my_vector1" : [1.5, 2.5], "price":10 }

POST /myindex/_search { "size" : 10, "query": { "knn": { "my_vector1": { "vector": [15, 25], "k": 2 } } } }

yields

{ "took" : 0, "timed_out" : false, "_shards" : { "total" : 1, "successful" : 1, "skipped" : 0, "failed" : 0 }, "hits" : { "total" : { "value" : 0, "relation" : "eq" }, "max_score" : null, "hits" : [ ] } }

How about we open a separate issue to track this?

Ah got it. I think we can probably ignore this case.

vamshin · 2020-04-15T07:20:27Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNSettings.java

+    /**
+     * Enum contains space types for k-NN similarity search
+     */
+    public enum SpaceTypes {


nit: Move this enum class to a dedicated file

vamshin · 2020-04-15T07:37:50Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/v1736/KNNIndex.java

     * @param algoParams hnsw algorithm parameters
     * @return knn index that can be queried for k nearest neighbours
     */
-    public static KNNIndex loadIndex(String indexPath, final String[] algoParams) {
+    public static KNNIndex loadIndex(String indexPath, String spaceType, final String[] algoParams) {


nit: final String spaceType?

jmazanec15

LGTM! Thanks for this contribution!

jmazanec15 · 2020-04-16T16:25:33Z

src/test/java/com/amazon/opendistroforelasticsearch/knn/index/KNNJNITests.java

@@ -165,7 +214,7 @@ public Void run() {
        float[] queryVector = {1.0f, 1.0f, 1.0f, 1.0f};
        String[] algoQueryParams = {"efSearch=200"};

-        final KNNIndex index = KNNIndex.loadIndex(indexPath, algoQueryParams);
+        final KNNIndex index = KNNIndex.loadIndex(indexPath, "l2", algoQueryParams);
        final KNNQueryResult[] results = index.queryIndex(queryVector, 30);

        Map<Integer, Float> scores = Arrays.stream(results).collect(


Makes sense thanks

chenqi0805 · 2020-04-17T17:52:22Z

@vamshin Thanks for the comments. Besides the comment on null value check, it is good for another look.

vamshin

LGTM! Thanks for the contribution. Much required feature for our community. Adding new space types supported by nmslib should be much simpler going forward and we could support it as per the community request.

As this change involves JNI level code changes and new functionality, we would evaluate this commit with performance tests and recall tests and then consider for merge. We are in the process of building automated performance tests for the commits but till then we may need to do some manual evaluation.

We will update this thread with the results and you could merge it then. Thank you.

vamshin · 2020-04-17T18:48:48Z

src/main/java/com/amazon/opendistroforelasticsearch/knn/index/KNNSettings.java

+
+        private Set<String> types = SpaceTypes.getValues();
+
+        @Override public void validate(String value) {


Ah got it. I think we can probably ignore this case.

jmazanec15 · 2020-04-22T22:48:55Z

Hi @chenqi0805, I think we are going to check this in as an experimental feature, until we can test it more thoroughly. Could you add a section in the readme under this section for Cosine Similarity usage with an example and mark it in the header as experimental? Like this: Cosine Similarity Usage (experimental)

chenqi0805 · 2020-04-22T22:51:39Z

@jmazanec15 Will do in free time.

chenqi0805 · 2020-04-26T18:38:52Z

@jmazanec15 updated.

jmazanec15 · 2020-04-27T17:20:49Z

Great thank you @chenqi0805! This is a great feature. Going to check it in.

chenqi0805 added 11 commits April 12, 2020 18:10

MNT: pass in spaceType in jni cpp

99cc16c

MNT: pass in spaceType in jni java

6375d49

Regenerate header file

28f33f9

Regenerate jnilib

784e67f

ADD: spacetype into KNN settings

d29cabd

Update vector field mapper

bde898c

Pass in spaceType in consumer

0bdf53e

ADD: spaceType param into constant

25cafe9

fix loadIndex in cache

2920f5b

FIX: existing test cases

a9388a9

Update jni external library

8a58bf6

jmazanec15 reviewed Apr 13, 2020

View reviewed changes

chenqi0805 added 7 commits April 13, 2020 22:05

FIX: release jstring and char array

de8271d

Rename test file and added a consinesimil test

c49efaf

update .jnilib

ce2660f

Rename file

2c10607

Rename file

65052c8

Compile .so

3eb7a47

Compile .so on linux

aa26cfb

jmazanec15 reviewed Apr 14, 2020

View reviewed changes

chenqi0805 added 4 commits April 14, 2020 21:48

TST: IT on invalid spaceType settings

18a8406

Merge branch 'feat/28-cosine-similarity' of github.com:chenqi0805/k-N…

5ebb930

…N into feat/28-cosine-similarity * 'feat/28-cosine-similarity' of github.com:chenqi0805/k-NN: Compile .so on linux

Unused import

635c7b1

Update README

83e4a90

chenqi0805 changed the title ~~WIP: FEAT: support cosine similarity~~ FEAT: support cosine similarity Apr 15, 2020

chenqi0805 changed the title ~~FEAT: support cosine similarity~~ [WIP]FEAT: support cosine similarity Apr 15, 2020

chenqi0805 changed the title ~~[WIP]FEAT: support cosine similarity~~ FEAT: support cosine similarity Apr 15, 2020

vamshin reviewed Apr 15, 2020

View reviewed changes

jmazanec15 approved these changes Apr 16, 2020

View reviewed changes

chenqi0805 added 4 commits April 17, 2020 10:36

MNT: null check on spacetype

e858038

REF: SpaceTypes

6cbfc79

MNT: arg order in jni

66cbbe8

Compile .so

c852d84

chenqi0805 requested a review from vamshin April 17, 2020 17:52

vamshin approved these changes Apr 17, 2020

View reviewed changes

MNT: update readme on settings and experimental section

0394e93

jmazanec15 merged commit 1c80b62 into opendistro-for-elasticsearch:master Apr 27, 2020

jmazanec15 mentioned this pull request May 5, 2020

Library incompatibility #103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: support cosine similarity #90

FEAT: support cosine similarity #90

chenqi0805 commented Apr 12, 2020 •

edited

Loading

jmazanec15 left a comment

jmazanec15 Apr 13, 2020

chenqi0805 Apr 14, 2020

jmazanec15 Apr 14, 2020

chenqi0805 Apr 14, 2020

jmazanec15 Apr 14, 2020

chenqi0805 Apr 14, 2020

jmazanec15 Apr 16, 2020

jmazanec15 Apr 14, 2020

chenqi0805 Apr 14, 2020

chenqi0805 Apr 15, 2020

jmazanec15 Apr 16, 2020

jmazanec15 Apr 14, 2020

chenqi0805 commented Apr 15, 2020 •

edited

Loading

vamshin commented Apr 15, 2020

vamshin Apr 15, 2020

vamshin Apr 15, 2020

chenqi0805 Apr 17, 2020

vamshin Apr 17, 2020

vamshin Apr 15, 2020

vamshin Apr 15, 2020

jmazanec15 left a comment

jmazanec15 Apr 16, 2020

chenqi0805 commented Apr 17, 2020

vamshin left a comment

vamshin Apr 17, 2020

jmazanec15 commented Apr 22, 2020

chenqi0805 commented Apr 22, 2020

chenqi0805 commented Apr 26, 2020

jmazanec15 commented Apr 27, 2020


		private Set<String> types = SpaceTypes.getValues();

		@Override public void validate(String value) {

FEAT: support cosine similarity #90

FEAT: support cosine similarity #90

Conversation

chenqi0805 commented Apr 12, 2020 • edited Loading

jmazanec15 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

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

chenqi0805 commented Apr 15, 2020 • edited Loading

vamshin commented Apr 15, 2020

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

jmazanec15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 commented Apr 17, 2020

vamshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 commented Apr 22, 2020

chenqi0805 commented Apr 22, 2020

chenqi0805 commented Apr 26, 2020

jmazanec15 commented Apr 27, 2020

chenqi0805 commented Apr 12, 2020 •

edited

Loading

chenqi0805 commented Apr 15, 2020 •

edited

Loading