return model id in registering remote model #1329

Zhangxunmt · 2023-09-12T22:31:26Z

Description

This change only applies to registering remote models. Registering a remote model doesn't need Asynchronize, it should return model_id right away in the response.

Before this change:
POST /_plugins/_ml/models/_register
{
"name": "openAI-GPT-3.5 completions",
"function_name": "remote",
"description": "test model",
"connector_id": "kUFFL4kB4ubqQRzeQ_r-"
}
Response:
{
"task_id": "l0FIL4kB4ubqQRzeKvpV",
"status": "CREATED"
}

After this change:
POST /_plugins/_ml/models/_register
{
"name": "openAI-GPT-3.5 completions",
"function_name": "remote",
"description": "test model",
"connector_id": "kUFFL4kB4ubqQRzeQ_r-"
}
Response:
{
"task_id": "l0FIL4kB4ubqQRzeKvpV",
"status": "CREATED",
"model_id": "xxxxxxxxxxxxxx",
}

Signed-off-by: Xun Zhang <[email protected]>

codecov · 2023-09-12T22:50:12Z

Codecov Report

Merging #1329 (23c0471) into 2.x (0855241) will decrease coverage by 0.87%.
Report is 1 commits behind head on 2.x.
The diff coverage is 23.77%.

@@             Coverage Diff              @@
##                2.x    #1329      +/-   ##
============================================
- Coverage     79.46%   78.60%   -0.87%     
+ Complexity     2273     2272       -1     
============================================
  Files           190      190              
  Lines          9167     9250      +83     
  Branches        906      909       +3     
============================================
- Hits           7285     7271      -14     
- Misses         1472     1569      +97     
  Partials        410      410

Flag	Coverage Δ
ml-commons	`78.60% <23.77%> (-0.87%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...n/java/org/opensearch/ml/model/MLModelManager.java	`54.62% <21.84%> (-8.36%)`	⬇️
.../action/register/TransportRegisterModelAction.java	`96.22% <100.00%> (+0.02%)`	⬆️

... and 2 files with indirect coverage changes

saratvemulapalli · 2023-09-13T18:54:18Z

common/src/main/java/org/opensearch/ml/common/transport/register/MLRegisterModelResponse.java

        this.taskId = in.readString();
        this.status = in.readString();
+        this.modelId = in.readOptionalString();
    }


Feels like a good improvement for remote models, do we really have a taskId which is doing real work for remote models?

Yes, we have a actual task id. It will do some validation work

Curious what kind of validations ?

saratvemulapalli · 2023-09-13T18:56:06Z

plugin/src/main/java/org/opensearch/ml/action/register/TransportRegisterModelAction.java

@@ -249,8 +250,8 @@ private void registerModel(MLRegisterModelInput registerModelInput, ActionListen
            mlTaskManager.createMLTask(mlTask, ActionListener.wrap(response -> {
                String taskId = response.getId();
                mlTask.setTaskId(taskId);
-                mlModelManager.registerMLModel(registerModelInput, mlTask);
-                listener.onResponse(new MLRegisterModelResponse(taskId, MLTaskState.CREATED.name()));
+                System.out.println("mlModelManager calls registerMLRemoteModel");


Nit: Probably unintended?
Same at L236

Good catch! I think I forgot to delete. They will be cleaned up for sure.

dhrubo-os · 2023-09-13T18:58:58Z

plugin/src/main/java/org/opensearch/ml/action/register/TransportRegisterModelAction.java

@@ -249,8 +250,8 @@ private void registerModel(MLRegisterModelInput registerModelInput, ActionListen
            mlTaskManager.createMLTask(mlTask, ActionListener.wrap(response -> {
                String taskId = response.getId();
                mlTask.setTaskId(taskId);
-                mlModelManager.registerMLModel(registerModelInput, mlTask);
-                listener.onResponse(new MLRegisterModelResponse(taskId, MLTaskState.CREATED.name()));
+                System.out.println("mlModelManager calls registerMLRemoteModel");


Are we keeping this line?

dhrubo-os · 2023-09-13T19:06:34Z

plugin/src/main/java/org/opensearch/ml/action/register/TransportRegisterModelAction.java

@@ -233,6 +233,7 @@ private void registerModel(MLRegisterModelInput registerModelInput, ActionListen
                throw new IllegalArgumentException("URL can't match trusted url regex");
            }
        }
+        System.out.println("registering the model");


Are we keeping this?

Let's avoid using system out print

Yes I guarante these redundant lines will be removed in my next PR.

dhrubo-os · 2023-09-13T19:12:28Z

plugin/src/main/java/org/opensearch/ml/model/MLModelManager.java

+        return (int) modelGroupSourceMap.get(MLModelGroup.LATEST_VERSION_FIELD) + 1;
+    }
+
+    private void indexRemoteModel(


Do we need the other method: private void indexRemoteModel(MLRegisterModelInput registerModelInput, MLTask mlTask, String modelVersion) as we are invoking this method only.

Yes it's needed for registering non-remote models. Actually if you use the other method, the model_id will not be included and the model registering could be asynchronize.

dhrubo-os · 2023-09-13T19:17:13Z

plugin/src/main/java/org/opensearch/ml/model/MLModelManager.java

+                            );
+
+                            client.update(updateModelGroupRequest, ActionListener.wrap(r -> {
+                                uploadMLModelMeta(mlRegisterModelMetaInput, updatedVersion + "", listener);


regarding : updatedVersion + "" if the goal is to convert int to string, may be we can follow standard way like: Integer.toString(updatedVersion)?

dhrubo-os · 2023-09-13T19:49:24Z

plugin/src/main/java/org/opensearch/ml/action/register/TransportRegisterModelAction.java

-                mlModelManager.registerMLModel(registerModelInput, mlTask);
-                listener.onResponse(new MLRegisterModelResponse(taskId, MLTaskState.CREATED.name()));
+                System.out.println("mlModelManager calls registerMLRemoteModel");
+                mlModelManager.registerMLRemoteModel(registerModelInput, mlTask, listener);


Is this the expectation that, only remote model registration will be asynchronous? If yes, can we please add a comment here also?

Just the opposite, remote model registration is synchronous only so the model_id can be returned right away. Comments will be added in the next PR.

return model id in registering remote model

23c0471

Signed-off-by: Xun Zhang <[email protected]>

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 22:31 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 22:31 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 22:31 — with GitHub Actions Error

Zhangxunmt requested review from jngz-es, ylwu-amzn and b4sjoo September 12, 2023 22:31

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 23:22 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 23:22 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 23:22 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 13, 2023 17:45 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 13, 2023 17:45 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 13, 2023 17:45 — with GitHub Actions Inactive

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 13, 2023 18:08 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 13, 2023 18:08 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 13, 2023 18:08 — with GitHub Actions Error

b4sjoo approved these changes Sep 13, 2023

View reviewed changes

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 13, 2023 18:24 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 13, 2023 18:24 — with GitHub Actions Failure

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 13, 2023 18:24 — with GitHub Actions Inactive

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 13, 2023 18:42 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 13, 2023 18:42 — with GitHub Actions Failure

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 13, 2023 18:42 — with GitHub Actions Inactive

rbhavna approved these changes Sep 13, 2023

View reviewed changes

saratvemulapalli reviewed Sep 13, 2023

View reviewed changes

dhrubo-os reviewed Sep 13, 2023

View reviewed changes

Zhangxunmt merged commit d21b032 into opensearch-project:2.x Sep 13, 2023

Zhangxunmt mentioned this pull request Sep 15, 2023

[Enhancement] Register Model API return model_id and Deploy Model API return deployment results #1146

Closed

zane-neo added the backport main label Sep 27, 2023

opensearch-trigger-bot bot mentioned this pull request Sep 27, 2023

[Backport main] return model id in registering remote model #1396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return model id in registering remote model #1329

return model id in registering remote model #1329

Zhangxunmt commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

saratvemulapalli Sep 13, 2023

ylwu-amzn Sep 13, 2023

saratvemulapalli Sep 13, 2023

saratvemulapalli Sep 13, 2023 •

edited

Loading

Zhangxunmt Sep 13, 2023

dhrubo-os Sep 13, 2023

dhrubo-os Sep 13, 2023

ylwu-amzn Sep 13, 2023

Zhangxunmt Sep 13, 2023

dhrubo-os Sep 13, 2023 •

edited

Loading

Zhangxunmt Sep 13, 2023

dhrubo-os Sep 13, 2023 •

edited

Loading

dhrubo-os Sep 13, 2023

Zhangxunmt Sep 13, 2023

return model id in registering remote model #1329

return model id in registering remote model #1329

Conversation

Zhangxunmt commented Sep 12, 2023 • edited Loading

Description

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli Sep 13, 2023 • 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

dhrubo-os Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhangxunmt commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

saratvemulapalli Sep 13, 2023 •

edited

Loading

dhrubo-os Sep 13, 2023 •

edited

Loading

dhrubo-os Sep 13, 2023 •

edited

Loading