Refactor model management to support apis #95

jmazanec15 · 2021-09-15T21:58:46Z

Signed-off-by: John Mazanec [email protected]

Description

This PR adds additional metadata to the Model System index, so that we can implement the APIs specified in #70. The metadata that is added is:

State - the lifecycle state the model is in - training, created, failed
Timestamp - when the model was created
Description - field allowing users to add detail to their models
Error - When model is in failed state, error will indicate why it failed

Additonally, this PR adds update functionality in ModelDao so that models can initially start in TRAINING state and later be updated.

Along with this, some minor code quality changes were made as well.

This PR also adds a TODO to fix the bug in clearing the cache in the ModelDao. This will be tracked in #93.

This PR does not add search functionality to the ModelDao. This will need to be done in a future PR.

Issues Resolved

#92

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --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.

Signed-off-by: John Mazanec <[email protected]>

src/main/java/org/opensearch/knn/common/KNNConstants.java

VijayanB · 2021-09-15T22:07:12Z

src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java

@@ -114,6 +114,10 @@ public void addKNNBinaryField(FieldInfo field, DocValuesProducer valuesProducer)
                String modelId = field.attributes().get(MODEL_ID);
                Model model = ModelCache.getInstance().get(modelId);

+                if (model.getModelBlob() == null) {
+                    throw new RuntimeException("Model cannot be null");


nit Model blob cannot be null

Will update.

VijayanB · 2021-09-15T22:08:12Z

src/main/java/org/opensearch/knn/indices/Model.java

-        this.modelBlob = Objects.requireNonNull(modelBlob, "modelBlob must not be null");
+
+        if (ModelState.CREATED.equals(this.modelMetadata.getState()) && modelBlob == null) {
+            throw new IllegalArgumentException("Model blob cannot be null when model metadata says model is created");


Will user understands what is model blob/ model meta data?

Right, we may have had this discussion before. Basically a model is made of a binary model and some metadata about the model. model_blob refers to the binary. I am not sure if there is a better name. Do you have any recs?

As long as we document what this error message means / how to resolve it, it should be fine.

Agree with Vijay. We can put message something like "We could not find model associated to this id xyz"?

Sure, will update error message.

VijayanB · 2021-09-15T22:09:11Z

src/main/java/org/opensearch/knn/indices/Model.java

@@ -56,7 +63,19 @@ public ModelMetadata getModelMetadata() {
     * @return length of model blob
     */
    public int getLength() {
-        return modelBlob.length;
+        if (modelBlob.get() == null) {


may be use getter from 56

do we have to mask length to zero if object is null? Why not let similar to other java objects like string/array?

may be use getter from 56

Will update

do we have to mask length to zero if object is null? Why not let similar to other java objects like string/array?

GetLength is called in ModelCache to determine how large the model is. For model's whose modelBlob is null, we say the length is 0 so cache loading doesnt fail.

VijayanB · 2021-09-15T22:13:57Z

src/main/java/org/opensearch/knn/indices/Model.java

@@ -69,13 +88,13 @@ public boolean equals(Object obj) {

        EqualsBuilder equalsBuilder = new EqualsBuilder();
        equalsBuilder.append(modelMetadata, other.modelMetadata);
-        equalsBuilder.append(modelBlob, other.modelBlob);
+        equalsBuilder.append(modelBlob.get(), other.modelBlob.get());


how are we accessing modelBlob as variable? shouldn't this be getModelBlolb?

Will update to use getter

VijayanB · 2021-09-15T22:14:59Z

src/main/java/org/opensearch/knn/indices/Model.java


        return equalsBuilder.isEquals();
    }

    @Override
    public int hashCode() {
-        return new HashCodeBuilder().append(modelMetadata).append(modelBlob).toHashCode();
+        return new HashCodeBuilder().append(modelMetadata).append(modelBlob.get()).toHashCode();


getModelBlob() to avoid modelBlob.get()

Will update to use getter

VijayanB · 2021-09-15T22:19:16Z

src/main/java/org/opensearch/knn/indices/ModelMetadata.java

    @Override
    public String toString() {
-        return knnEngine.getName() + DELIMITER + spaceType.getValue() + DELIMITER + dimension;
+        return knnEngine.getName() + DELIMITER +


nit: may be stringbuilder or String.join()

Sure, will update.

VijayanB · 2021-09-15T22:20:14Z

src/main/java/org/opensearch/knn/indices/ModelMetadata.java

@@ -104,13 +189,18 @@ public boolean equals(Object obj) {
        equalsBuilder.append(knnEngine, other.knnEngine);
        equalsBuilder.append(spaceType, other.spaceType);
        equalsBuilder.append(dimension, other.dimension);
+        equalsBuilder.append(state.get(), other.state.get());
+        equalsBuilder.append(timestamp, other.timestamp);


same as above, shouldn't we use getter?

yes, will update.

same for timestamp/description/error

Sure, will update

VijayanB · 2021-09-15T22:22:13Z

src/test/java/org/opensearch/knn/indices/ModelDaoTests.java

@@ -107,6 +141,54 @@ public void testPut_withId() throws InterruptedException, IOException {
        assertTrue(inProgressLatch2.await(100, TimeUnit.SECONDS));
    }

+    public void testPut_withoutModel() throws InterruptedException, IOException {


testPutWithoutModel

I use underscore for better readability. First portion says what we are testing. After underscore represents specific case. I could just test all put together but I felt that would be too long.

VijayanB · 2021-09-15T22:23:36Z

src/test/java/org/opensearch/knn/indices/ModelDaoTests.java

+        assertTrue(inProgressLatch1.await(100, TimeUnit.SECONDS));
+
+        // User provided model id that already exists - should be able to update
+        Model model2 = new Model(new ModelMetadata(KNNEngine.DEFAULT, SpaceType.DEFAULT, dimension, ModelState.CREATED,


nit: duplicateModel

will update

Signed-off-by: John Mazanec <[email protected]>

vamshin · 2021-09-22T05:08:54Z

src/main/java/org/opensearch/knn/index/codec/KNN80Codec/KNN80DocValuesConsumer.java

@@ -114,6 +114,10 @@ public void addKNNBinaryField(FieldInfo field, DocValuesProducer valuesProducer)
                String modelId = field.attributes().get(MODEL_ID);
                Model model = ModelCache.getInstance().get(modelId);

+                if (model.getModelBlob() == null) {
+                    throw new RuntimeException("Model blob cannot be null");


Lets have user friendly message. Something like "There is no model associated to this id xyz"

Sure, will update.

vamshin · 2021-09-22T05:10:29Z

src/main/java/org/opensearch/knn/indices/Model.java

-        this.modelBlob = Objects.requireNonNull(modelBlob, "modelBlob must not be null");
+
+        if (ModelState.CREATED.equals(this.modelMetadata.getState()) && modelBlob == null) {
+            throw new IllegalArgumentException("Model blob cannot be null when model metadata says model is created");


Agree with Vijay. We can put message something like "We could not find model associated to this id xyz"?

vamshin · 2021-09-22T05:11:10Z

src/main/java/org/opensearch/knn/indices/Model.java

        this.modelMetadata = Objects.requireNonNull(modelMetadata, "modelMetadata must not be null");
-        this.modelBlob = Objects.requireNonNull(modelBlob, "modelBlob must not be null");
+
+        if (ModelState.CREATED.equals(this.modelMetadata.getState()) && modelBlob == null) {


Should this be equalsIgnoreCase instead of equals?

CREATED is an enum not a string.

vamshin · 2021-09-22T05:13:04Z

src/main/java/org/opensearch/knn/indices/Model.java

@@ -56,7 +63,19 @@ public ModelMetadata getModelMetadata() {
     * @return length of model blob
     */
    public int getLength() {
-        return modelBlob.length;
+        if (getModelBlob() == null) {
+            return 0;


Minor: Should this be -1 to make it clear to the caller that there is no model?

Given that we use this in the cache, I think it should just be 0, indicating the model has a size of 0.

vamshin · 2021-09-22T05:20:05Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

+     * @param model new model
+     * @param listener handles index response
+     */
+    void update(String modelId, Model model, ActionListener<IndexResponse> listener) throws IOException;


I thought we do not want to update model once created? Can some one do it using this function?

This is required because initially we set state as training, then we train, then we update state to created and add the model. This will not be exposed to user.

vamshin · 2021-09-22T05:21:19Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

-                    listener);
+            // After metadata update finishes, remove item from cache if necessary. If no model id is
+            // passed then nothing needs to be removed from the cache
+            //TODO: Bug. Model needs to be removed from all nodes caches, not just local.


Are we planning to fix this TODO in next PR? Having model updatable would result in these scenarios. We should Ideally be not updating them.

Yes, this is a bug that needs to be fixed. The problem is more relevant in DELETE. #93

Ok lets also put the issue in the comment.

Signed-off-by: John Mazanec <[email protected]>

vamshin

LGTM! Thanks

Signed-off-by: Jack Mazanec <[email protected]>

Signed-off-by: Jack Mazanec <[email protected]> Signed-off-by: Martin Gaievski <[email protected]>

Signed-off-by: Jack Mazanec <[email protected]>

Refactor modeldao to support apis from opensearch-project#70 (comment)

069629c

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 requested review from vamshin and VijayanB September 15, 2021 21:58

VijayanB reviewed Sep 15, 2021

View reviewed changes

src/main/java/org/opensearch/knn/common/KNNConstants.java Show resolved Hide resolved

VijayanB reviewed Sep 15, 2021

View reviewed changes

Address comments (1)

e03799d

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 requested a review from VijayanB September 15, 2021 23:18

jmazanec15 added 2 commits September 17, 2021 11:17

Update modeldao put to use indexresponse listener

efb5943

Use getters/setters internally

b4885ee

Signed-off-by: John Mazanec <[email protected]>

vamshin reviewed Sep 22, 2021

View reviewed changes

Fix exception messages

7294d60

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 requested a review from vamshin September 22, 2021 16:39

jmazanec15 added 2 commits September 22, 2021 10:31

Reword additional exceptions

fb2924d

Signed-off-by: John Mazanec <[email protected]>

Add issue in todo comment

5bb6983

Signed-off-by: John Mazanec <[email protected]>

vamshin approved these changes Sep 22, 2021

View reviewed changes

VijayanB approved these changes Sep 22, 2021

View reviewed changes

jmazanec15 merged commit 3cc2857 into opensearch-project:feature/faiss-support Sep 22, 2021

jmazanec15 added a commit to jmazanec15/k-NN-1 that referenced this pull request Oct 22, 2021

Refactor model management to support apis (opensearch-project#95)

f221628

Signed-off-by: Jack Mazanec <[email protected]>

jmazanec15 added a commit that referenced this pull request Oct 22, 2021

Refactor model management to support apis (#95)

6319bb2

Signed-off-by: Jack Mazanec <[email protected]>

martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022

Refactor model management to support apis (opensearch-project#95)

af55007

Signed-off-by: Jack Mazanec <[email protected]>

martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022

Refactor model management to support apis (opensearch-project#95)

24016bb

Signed-off-by: Jack Mazanec <[email protected]> Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 30, 2022

Refactor model management to support apis (opensearch-project#95)

c9edeee

Signed-off-by: Jack Mazanec <[email protected]>

Refactor model management to support apis #95

Refactor model management to support apis #95

Conversation

jmazanec15 commented Sep 15, 2021

Description

Issues Resolved

Check List

VijayanB Sep 15, 2021 • edited Loading

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

jmazanec15 Sep 15, 2021 • edited Loading

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

VijayanB Sep 15, 2021 • edited Loading

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

vamshin Sep 22, 2021 • edited Loading

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

vamshin left a comment

Choose a reason for hiding this comment

VijayanB Sep 15, 2021 •

edited

Loading

jmazanec15 Sep 15, 2021 •

edited

Loading

VijayanB Sep 15, 2021 •

edited

Loading

vamshin Sep 22, 2021 •

edited

Loading