enable auto redeploy for hidden model #2102

rbhavna · 2024-02-13T23:41:56Z

Description

enable auto redeploy for hidden model

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed 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: Bhavana Ramaram <rbhavna@amazon.com>

dhrubo-os · 2024-02-13T23:51:30Z

common/src/main/java/org/opensearch/ml/common/transport/deploy/MLDeployModelRequest.java

    }

    public MLDeployModelRequest(String modelId, boolean async) {
-        this(modelId, null, async, true);
+        this(modelId, null, async, true, true);


[nit] may be add a comment why here isUserInitiatedDeployRequest will always be true?

dhrubo-os · 2024-02-13T23:59:13Z

common/src/main/java/org/opensearch/ml/common/transport/deploy/MLDeployModelRequest.java

    }

    public MLDeployModelRequest(StreamInput in) throws IOException {
        super(in);
        this.modelId = in.readString();
        this.modelNodeIds = in.readOptionalStringArray();
        this.async = in.readBoolean();
+        this.isUserInitiatedDeployRequest = in.readOptionalBoolean();


why optionalboolean? what'll happen if the value isn't present?

codecov · 2024-02-14T00:07:36Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (67e904e) 81.83% compared to head (9b65523) 81.83%.
Report is 10 commits behind head on 2.x.

Files	Patch %	Lines
...h/ml/action/deploy/TransportDeployModelAction.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #2102      +/-   ##
============================================
- Coverage     81.83%   81.83%   -0.01%     
+ Complexity     5621     5620       -1     
============================================
  Files           543      543              
  Lines         22819    22825       +6     
  Branches       2339     2340       +1     
============================================
+ Hits          18675    18679       +4     
- Misses         3210     3213       +3     
+ Partials        934      933       -1

Flag	Coverage Δ
ml-commons	`81.83% <90.90%> (-0.01%)`	⬇️

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

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

arjunkumargiri

Can we follow similar logic for non-hidden models to bypass access check in case the request is system initiated? : https://github.com/opensearch-project/ml-commons/pull/2102/files#diff-c341f5a67c1d221e5593dd55fb689e9d0c9a60d33e0fa26090803cfa90818fa7R162

arjunkumargiri · 2024-02-14T00:06:23Z

plugin/src/test/java/org/opensearch/ml/action/deploy/TransportDeployModelActionTests.java

@@ -286,6 +286,7 @@ public void testDoExecute_no_permission_hidden_model() {
        MLModel mlModel = mock(MLModel.class);
        when(mlModel.getAlgorithm()).thenReturn(FunctionName.ANOMALY_LOCALIZATION);
        when(mlModel.getIsHidden()).thenReturn(true);
+        when(mlDeployModelRequest.isUserInitiatedDeployRequest()).thenReturn(true);


Add unit test to cover isUserInitiatedDeployRequest set to false.

rbhavna · 2024-02-14T00:29:49Z

Can we follow similar logic for non-hidden models to bypass access check in case the request is system initiated? : https://github.com/opensearch-project/ml-commons/pull/2102/files#diff-c341f5a67c1d221e5593dd55fb689e9d0c9a60d33e0fa26090803cfa90818fa7R162

This is already handled in the validateModelGroupAccess method. But it checks for user being null and skips access check which handles the auto redeploy scenario

ylwu-amzn · 2024-02-14T00:38:38Z

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

@@ -857,7 +857,7 @@ private void updateModelRegisterStateAsDone(
    void deployModelAfterRegistering(MLRegisterModelInput registerModelInput, String modelId) {
        String[] modelNodeIds = registerModelInput.getModelNodeIds();
        log.debug("start deploying model after registering, modelId: {} on nodes: {}", modelId, Arrays.toString(modelNodeIds));
-        MLDeployModelRequest request = new MLDeployModelRequest(modelId, modelNodeIds, false, true);
+        MLDeployModelRequest request = new MLDeployModelRequest(modelId, modelNodeIds, false, true, true);


What if user define a flow template with deploy hidden model action in it ? Can user deploy any hidden model?

Isn't hidden model management (register, deploy, undeploy, delete) is out of scope for AI-Flow?

With a flow template, we will still consider it as user initiated right. So we will still check for access. Only if it is system initiated, we will bypass the access checks

Isn't hidden model management (register, deploy, undeploy, delete) is out of scope for AI-Flow?

Yes for normal process, but that's not a hard control. User can copy the hidden model id and define a flow template to deploy it.

Can user see the hidden model id? I thought only superAdmin can see it.

Can user see the hidden model id? I thought only superAdmin can see it.

model id isn't hidden to users. But model content is hidden to users. During prediction, model id is being used. As user can make the prediction request, they can know the model id eventually.

Yes for normal process, but that's not a hard control. User can copy the hidden model id and define a flow template to deploy it.

We opened this deploy request to clients and then user initiated request flag is being set to true from code. So every request sent from ai-flow or any other clients should be treated as user initiated request too.

arjunkumargiri · 2024-02-14T00:53:35Z

Can we follow similar logic for non-hidden models to bypass access check in case the request is system initiated? : https://github.com/opensearch-project/ml-commons/pull/2102/files#diff-c341f5a67c1d221e5593dd55fb689e9d0c9a60d33e0fa26090803cfa90818fa7R162

This is already handled in the validateModelGroupAccess method. But it checks for user being null and skips access check which handles the auto redeploy scenario

Null user checks is a workaround, can we explicitly check if deploy request is not user initiated so that same behavior is maintained for system initiated deployments?

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>

dhrubo-os · 2024-02-18T01:26:20Z

common/src/main/java/org/opensearch/ml/common/transport/deploy/MLDeployModelRequest.java

@@ -38,24 +38,32 @@ public class MLDeployModelRequest extends MLTaskRequest {
    private String modelId;
    private String[] modelNodeIds;
    boolean async;
+    // This is to identify if the get request is initiated by user or not. During auto redeploy also, we perform deploy operation.


[nit] this is a deploy request not a get request.

dhrubo-os · 2024-02-18T01:30:48Z

Can we follow similar logic for non-hidden models to bypass access check in case the request is system initiated? : https://github.com/opensearch-project/ml-commons/pull/2102/files#diff-c341f5a67c1d221e5593dd55fb689e9d0c9a60d33e0fa26090803cfa90818fa7R162

This is already handled in the validateModelGroupAccess method. But it checks for user being null and skips access check which handles the auto redeploy scenario

Null user checks is a workaround, can we explicitly check if deploy request is not user initiated so that same behavior is maintained for system initiated deployments?

@rbhavna are we planning to address this?

rbhavna · 2024-02-19T18:05:06Z

Can we follow similar logic for non-hidden models to bypass access check in case the request is system initiated? : https://github.com/opensearch-project/ml-commons/pull/2102/files#diff-c341f5a67c1d221e5593dd55fb689e9d0c9a60d33e0fa26090803cfa90818fa7R162

This is already handled in the validateModelGroupAccess method. But it checks for user being null and skips access check which handles the auto redeploy scenario

Null user checks is a workaround, can we explicitly check if deploy request is not user initiated so that same behavior is maintained for system initiated deployments?

@rbhavna are we planning to address this?

Yes this has been addressed already https://github.com/opensearch-project/ml-commons/pull/2102/files#diff-c341f5a67c1d221e5593dd55fb689e9d0c9a60d33e0fa26090803cfa90818fa7R147

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>

* enable auto redeploy for hidden model Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> (cherry picked from commit 9567ca5)

* enable auto redeploy for hidden model Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> (cherry picked from commit 9567ca5) Co-authored-by: Bhavana Ramaram <rbhavna@amazon.com>

…search-project#2136) * enable auto redeploy for hidden model Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> (cherry picked from commit 9567ca5) Co-authored-by: Bhavana Ramaram <rbhavna@amazon.com>

Zhangxunmt · 2024-03-19T03:39:34Z

Why this PR is not backported to main?

rbhavna · 2024-03-19T03:42:46Z

Why this PR is not backported to main?

This was merged in this PR @Zhangxunmt #2136

…search-project#2136) * enable auto redeploy for hidden model Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> (cherry picked from commit 9567ca5) Co-authored-by: Bhavana Ramaram <rbhavna@amazon.com>

enable auto redeploy for hidden model

80d3a75

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>

rbhavna requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and sam-herman as code owners February 13, 2024 23:41

rbhavna temporarily deployed to ml-commons-cicd-env February 13, 2024 23:42 — with GitHub Actions Inactive

dhrubo-os reviewed Feb 13, 2024

View reviewed changes

rbhavna had a problem deploying to ml-commons-cicd-env February 14, 2024 00:08 — with GitHub Actions Failure

arjunkumargiri reviewed Feb 14, 2024

View reviewed changes

ylwu-amzn reviewed Feb 14, 2024

View reviewed changes

addressing comments

81f619b

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>

rbhavna had a problem deploying to ml-commons-cicd-env February 16, 2024 00:28 — with GitHub Actions Error

rbhavna had a problem deploying to ml-commons-cicd-env February 16, 2024 00:28 — with GitHub Actions Failure

rbhavna had a problem deploying to ml-commons-cicd-env February 16, 2024 15:41 — with GitHub Actions Error

rbhavna temporarily deployed to ml-commons-cicd-env February 16, 2024 15:41 — with GitHub Actions Inactive

rbhavna had a problem deploying to ml-commons-cicd-env February 16, 2024 15:41 — with GitHub Actions Failure

rbhavna had a problem deploying to ml-commons-cicd-env February 16, 2024 15:41 — with GitHub Actions Error

arjunkumargiri approved these changes Feb 17, 2024

View reviewed changes

dhrubo-os reviewed Feb 18, 2024

View reviewed changes

addressed comments

9b65523

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>

rbhavna temporarily deployed to ml-commons-cicd-env February 19, 2024 22:09 — with GitHub Actions Inactive

dhrubo-os approved these changes Feb 19, 2024

View reviewed changes

ylwu-amzn approved these changes Feb 19, 2024

View reviewed changes

rbhavna temporarily deployed to ml-commons-cicd-env February 19, 2024 22:37 — with GitHub Actions Inactive

rbhavna merged commit 9567ca5 into opensearch-project:2.x Feb 19, 2024
15 checks passed

rbhavna added the backport main label Feb 19, 2024

opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 19, 2024

enable auto redeploy for hidden model (#2102)

6e3a586

* enable auto redeploy for hidden model Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> (cherry picked from commit 9567ca5)

opensearch-trigger-bot bot mentioned this pull request Feb 19, 2024

[Backport main] enable auto redeploy for hidden model #2136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable auto redeploy for hidden model #2102

enable auto redeploy for hidden model #2102

rbhavna commented Feb 13, 2024

dhrubo-os Feb 13, 2024

dhrubo-os Feb 13, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading

arjunkumargiri left a comment

arjunkumargiri Feb 14, 2024

rbhavna commented Feb 14, 2024

ylwu-amzn Feb 14, 2024

dhrubo-os Feb 14, 2024

rbhavna Feb 14, 2024

ylwu-amzn Feb 14, 2024 •

edited

Loading

zane-neo Feb 18, 2024

dhrubo-os Feb 18, 2024

dhrubo-os Feb 18, 2024

arjunkumargiri commented Feb 14, 2024

dhrubo-os Feb 18, 2024

dhrubo-os commented Feb 18, 2024

rbhavna commented Feb 19, 2024

Zhangxunmt commented Mar 19, 2024

rbhavna commented Mar 19, 2024

enable auto redeploy for hidden model #2102

enable auto redeploy for hidden model #2102

Conversation

rbhavna commented Feb 13, 2024

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

arjunkumargiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbhavna commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn Feb 14, 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

arjunkumargiri commented Feb 14, 2024

Choose a reason for hiding this comment

dhrubo-os commented Feb 18, 2024

rbhavna commented Feb 19, 2024

Zhangxunmt commented Mar 19, 2024

rbhavna commented Mar 19, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading

ylwu-amzn Feb 14, 2024 •

edited

Loading