Serialize all models into cluster metadata #1499

ryanbogan · 2024-02-27T20:12:23Z

Description

This PR removes a check that only serialized models into cluster metadata when they were in the "created" state. This means that any model in "training" or "failed" states could only be accessed through blocking transport requests. This PR also minimizes the transport calls in TrainingJobClusterStateListener, instead accessing model metadata from the cluster metadata.

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.

…teListener Signed-off-by: Ryan Bogan <[email protected]>

Signed-off-by: Ryan Bogan <[email protected]>

codecov · 2024-02-27T20:30:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.06%. Comparing base (771c4b5) to head (a0cb2be).
Report is 9 commits behind head on main.

❗ Current head a0cb2be differs from pull request most recent head 231452e. Consider uploading reports for the commit 231452e to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1499      +/-   ##
============================================
+ Coverage     84.92%   85.06%   +0.13%     
+ Complexity     1375     1280      -95     
============================================
  Files           172      168       -4     
  Lines          5605     5229     -376     
  Branches        553      494      -59     
============================================
- Hits           4760     4448     -312     
+ Misses          612      573      -39     
+ Partials        233      208      -25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15

We need to update here as well: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/mapper/ModelFieldMapper.java#L48-L52

jmazanec15 · 2024-03-08T18:57:09Z

src/main/java/org/opensearch/knn/training/TrainingJobClusterStateListener.java

@@ -109,10 +109,9 @@ protected void updateModelsNewCluster() throws IOException, InterruptedException
        if (modelDao.isCreated()) {
            List<String> modelIds = searchModelIds();
            for (String modelId : modelIds) {
-                Model model = modelDao.get(modelId);
-                ModelMetadata modelMetadata = model.getModelMetadata();
+                ModelMetadata modelMetadata = modelDao.getMetadata(modelId);


Is this going to be backwards compatible?

I should probably add a version check, since they won't be in the metadata on older clusters if it's in created state

Actually, I'm not sure if there is a way to make it backwards compatible, since the old models wouldn't be in the cluster metadata. I think I have to revert to the get call

jmazanec15 · 2024-03-08T19:06:20Z

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

-            } else {
-                onIndexListener = onMetaListener;
-            }
+            ActionListener<IndexResponse> onIndexListener = getUpdateModelMetadataListener(model.getModelMetadata(), onMetaListener);


Want to discuss a little bit here some thoughts:

So before, the logic was that the model index is the only source of truth until the model is actually created. Then the model-metadata is a source of truth as well.

Now, we are saying that the model metadata will always lag behind the model system index (i.e. on any call to change model state, we will first persist in system index and then update in cluster state). This is going to open up to the possibility that model index and cluster state fall out of sync on failure. We may need to think about how we handle different scenarios.

I think a model's state can be updated in one of the following ways:

Training process (this will be in sync - single thread, synchronous process)

Model deleted (we block the model delete if the model is not in the created or error state - we should confirm that we check this from cluster metadata before blocking)

Node drop (if a node drops, the offending node will know not to change anything in the state correct?)

Are there other cases you can think of that might be of concern?

@jmazanec15 The goal of this was to treat the cluster metadata as the main source of truth in case of node drops I believe. This would decrease the chance of nodes being out of sync with the cluster since they could always access the model metadata from the cluster metadata.

I wrote a temporary integration test trying to create a race condition between a model finishing training and entering the created state in ModelDao and then deleting the model immediately. All behavior was as expected and the model was deleted from the cluster metadata as well.

@ryanbogan can you push to another branch and link to the test? I want to take a quick look.

@jmazanec15 I already deleted it off the local, do you want me to recreate it?

jmazanec15 · 2024-03-08T19:08:59Z

@ryanbogan and check from here:

k-NN/src/main/java/org/opensearch/knn/index/query/KNNWeight.java

Lines 213 to 221 in 7a144b8

    
           String modelId = fieldInfo.getAttribute(MODEL_ID); 
        
           if (modelId != null) { 
        
               ModelMetadata modelMetadata = modelDao.getMetadata(modelId); 
        
               if (modelMetadata == null) { 
        
                   throw new RuntimeException("Model \"" + modelId + "\" does not exist."); 
        
               } 
        
               knnEngine = modelMetadata.getKnnEngine(); 
        
               spaceType = modelMetadata.getSpaceType();

jmazanec15 · 2024-03-08T19:09:44Z

and here:

k-NN/src/main/java/org/opensearch/knn/index/IndexUtil.java

Line 25 in 7a144b8

import org.opensearch.knn.indices.ModelMetadata;

ryanbogan · 2024-03-28T16:25:10Z

@ryanbogan and check from here:

k-NN/src/main/java/org/opensearch/knn/index/query/KNNWeight.java

Lines 213 to 221 in 7a144b8

String modelId = fieldInfo.getAttribute(MODEL_ID);

if (modelId != null) {

ModelMetadata modelMetadata = modelDao.getMetadata(modelId);

if (modelMetadata == null) {

throw new RuntimeException("Model \"" + modelId + "\" does not exist.");

}

knnEngine = modelMetadata.getKnnEngine();

spaceType = modelMetadata.getSpaceType();

I'll add a check to get the state

Signed-off-by: Ryan Bogan <[email protected]>

jni/CMakeLists.txt

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan · 2024-04-17T20:24:34Z

Windows failures are unrelated to this PR

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15

a few minor comments. overall looks good

src/main/java/org/opensearch/knn/index/IndexUtil.java

src/main/java/org/opensearch/knn/index/mapper/ModelFieldMapper.java

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

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan · 2024-04-18T18:16:31Z

BWC failure unrelated to this change: #1622

src/main/java/org/opensearch/knn/training/TrainingJobClusterStateListener.java

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15

LGTM thanks!

martin-gaievski · 2024-04-19T15:24:24Z

src/main/java/org/opensearch/knn/index/query/KNNWeight.java

-            if (modelMetadata == null) {
-                throw new RuntimeException("Model \"" + modelId + "\" does not exist.");
+            if (!ModelUtil.isModelCreated(modelMetadata)) {
+                throw new RuntimeException("Model \"" + modelId + "\" is not created.");


nit: please use String.format for string concatenation

@martin-gaievski this comes up a couple times because we do not do it for a lot of existing code. Do you know if its possible to add something to spotless to automatically fix this?

it should be possible, we did some custom template for spotless in neural-search, please check this PR opensearch-project/neural-search#515

@martin-gaievski is there a comment for it or file? Im not seeing it in formatting

Yeah I can't see anything for string.format, maybe it's not possible. @vibrantvarun do you know by any chance if we can automate fix for String.format(locale, "")?

* Remove transport calls in TrainingJobRunner and TrainingJobClusterStateListener Signed-off-by: Ryan Bogan <[email protected]> * Fix tests Signed-off-by: Ryan Bogan <[email protected]> * Add changelog Signed-off-by: Ryan Bogan <[email protected]> * Fix CMake Faiss bug Signed-off-by: Ryan Bogan <[email protected]> * Add state checks for existing cluster metadata calls Signed-off-by: Ryan Bogan <[email protected]> * Remove CMake bug fix Signed-off-by: Ryan Bogan <[email protected]> * Fix changelog Signed-off-by: Ryan Bogan <[email protected]> * Fix failing tests Signed-off-by: Ryan Bogan <[email protected]> * Refactor and add two more created state checks Signed-off-by: Ryan Bogan <[email protected]> * Rebase and fix new tests Signed-off-by: Ryan Bogan <[email protected]> * Refactor created checks and modify error messages Signed-off-by: Ryan Bogan <[email protected]> * Refactor cluster state listener transport calls Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit dc8eb6b)

* Remove transport calls in TrainingJobRunner and TrainingJobClusterStateListener Signed-off-by: Ryan Bogan <[email protected]> * Fix tests Signed-off-by: Ryan Bogan <[email protected]> * Add changelog Signed-off-by: Ryan Bogan <[email protected]> * Fix CMake Faiss bug Signed-off-by: Ryan Bogan <[email protected]> * Add state checks for existing cluster metadata calls Signed-off-by: Ryan Bogan <[email protected]> * Remove CMake bug fix Signed-off-by: Ryan Bogan <[email protected]> * Fix changelog Signed-off-by: Ryan Bogan <[email protected]> * Fix failing tests Signed-off-by: Ryan Bogan <[email protected]> * Refactor and add two more created state checks Signed-off-by: Ryan Bogan <[email protected]> * Rebase and fix new tests Signed-off-by: Ryan Bogan <[email protected]> * Refactor created checks and modify error messages Signed-off-by: Ryan Bogan <[email protected]> * Refactor cluster state listener transport calls Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit dc8eb6b) Co-authored-by: Ryan Bogan <[email protected]>

ryanbogan added 3 commits February 2, 2024 15:19

Remove transport calls in TrainingJobRunner and TrainingJobClusterSta…

64a5e98

…teListener Signed-off-by: Ryan Bogan <[email protected]>

Merge branch 'main' into serialize_all_models

d0d82f0

Fix tests

ec35d3d

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan added 2 commits March 8, 2024 10:11

Merge branch 'main' into serialize_all_models

47e8647

Add changelog

a0cb2be

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan marked this pull request as ready for review March 8, 2024 18:13

ryanbogan requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei and martin-gaievski as code owners March 8, 2024 18:13

jmazanec15 reviewed Mar 8, 2024

View reviewed changes

ryanbogan added 2 commits April 15, 2024 14:24

Merge main into serialize_all_models

aacc030

Signed-off-by: Ryan Bogan <[email protected]>

Fix CMake Faiss bug

a205ad2

Signed-off-by: Ryan Bogan <[email protected]>

martin-gaievski reviewed Apr 16, 2024

View reviewed changes

jni/CMakeLists.txt Outdated Show resolved Hide resolved

ryanbogan added 4 commits April 16, 2024 10:17

Add state checks for existing cluster metadata calls

c58ef98

Signed-off-by: Ryan Bogan <[email protected]>

Remove CMake bug fix

775dfd2

Signed-off-by: Ryan Bogan <[email protected]>

Fix changelog

c8dcfb3

Signed-off-by: Ryan Bogan <[email protected]>

Fix failing tests

9eb9c45

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan added 3 commits April 17, 2024 13:59

Refactor and add two more created state checks

df9f3d8

Signed-off-by: Ryan Bogan <[email protected]>

Merge branch 'main' into serialize_all_models

59c1456

Rebase and fix new tests

0299567

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15 reviewed Apr 18, 2024

View reviewed changes

Refactor created checks and modify error messages

3508c79

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15 reviewed Apr 18, 2024

View reviewed changes

src/main/java/org/opensearch/knn/training/TrainingJobClusterStateListener.java Outdated Show resolved Hide resolved

Refactor cluster state listener transport calls

231452e

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15 approved these changes Apr 18, 2024

View reviewed changes

martin-gaievski reviewed Apr 19, 2024

View reviewed changes

martin-gaievski approved these changes Apr 19, 2024

View reviewed changes

ryanbogan merged commit dc8eb6b into opensearch-project:main Apr 23, 2024
49 of 51 checks passed

ryanbogan added the backport 2.x label Apr 23, 2024

ryanbogan deleted the serialize_all_models branch April 23, 2024 17:27

opensearch-trigger-bot bot mentioned this pull request Apr 23, 2024

[Backport 2.x] Serialize all models into cluster metadata #1644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize all models into cluster metadata #1499

Serialize all models into cluster metadata #1499

ryanbogan commented Feb 27, 2024

codecov bot commented Feb 27, 2024 •

edited

Loading

jmazanec15 left a comment

jmazanec15 Mar 8, 2024

ryanbogan Mar 26, 2024

ryanbogan Apr 16, 2024 •

edited

Loading

jmazanec15 Mar 8, 2024

ryanbogan Mar 26, 2024 •

edited

Loading

ryanbogan Apr 16, 2024

jmazanec15 Apr 16, 2024

ryanbogan Apr 17, 2024

jmazanec15 commented Mar 8, 2024

jmazanec15 commented Mar 8, 2024

ryanbogan commented Mar 28, 2024

ryanbogan commented Apr 17, 2024

jmazanec15 left a comment

ryanbogan commented Apr 18, 2024

jmazanec15 left a comment

martin-gaievski Apr 19, 2024

jmazanec15 Apr 19, 2024

martin-gaievski Apr 19, 2024

jmazanec15 Apr 19, 2024

martin-gaievski Apr 19, 2024

Serialize all models into cluster metadata #1499

Serialize all models into cluster metadata #1499

Conversation

ryanbogan commented Feb 27, 2024

Description

Check List

codecov bot commented Feb 27, 2024 • edited Loading

Codecov Report

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

ryanbogan Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbogan Mar 26, 2024 • 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

jmazanec15 commented Mar 8, 2024

jmazanec15 commented Mar 8, 2024

ryanbogan commented Mar 28, 2024

ryanbogan commented Apr 17, 2024

jmazanec15 left a comment

Choose a reason for hiding this comment

ryanbogan commented Apr 18, 2024

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

codecov bot commented Feb 27, 2024 •

edited

Loading

ryanbogan Apr 16, 2024 •

edited

Loading

ryanbogan Mar 26, 2024 •

edited

Loading