Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable auto redeploy for hidden model #2102

Merged
merged 3 commits into from
Feb 19, 2024
Merged

enable auto redeploy for hidden model #2102

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80d3a75
enable auto redeploy for hidden model
rbhavna Feb 13, 2024
81f619b
addressing comments
rbhavna Feb 16, 2024
9b65523
addressed comments
rbhavna Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions common/src/main/java/org/opensearch/ml/common/transport/deploy/MLDeployModelRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,24 +38,29 @@ 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, we also perform deploy operation. This field is to distinguish between
// these two situations.
private final boolean isUserInitiatedDeployRequest;

@Builder
public MLDeployModelRequest(String modelId, String[] modelNodeIds, boolean async, boolean dispatchTask) {
public MLDeployModelRequest(String modelId, String[] modelNodeIds, boolean async, boolean dispatchTask, boolean isUserInitiatedDeployRequest) {
super(dispatchTask);
this.modelId = modelId;
this.modelNodeIds = modelNodeIds;
this.async = async;
this.isUserInitiatedDeployRequest = isUserInitiatedDeployRequest;
}

public MLDeployModelRequest(String modelId, boolean async) {
this(modelId, null, async, true);
this(modelId, null, async, true, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Override
Expand All @@ -74,6 +79,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(modelId);
out.writeOptionalStringArray(modelNodeIds);
out.writeBoolean(async);
out.writeOptionalBoolean(isUserInitiatedDeployRequest);
}

public static MLDeployModelRequest parse(XContentParser parser, String modelId) throws IOException {
Expand All @@ -96,7 +102,7 @@ public static MLDeployModelRequest parse(XContentParser parser, String modelId)
}
}
String[] nodeIds = nodeIdList == null ? null : nodeIdList.toArray(new String[0]);
return new MLDeployModelRequest(modelId, nodeIds, false, true);
return new MLDeployModelRequest(modelId, nodeIds, false, true, true);
}

public static MLDeployModelRequest fromActionRequest(ActionRequest actionRequest) {
Expand Down
3 changes: 2 additions & 1 deletion plugin/src/main/java/org/opensearch/ml/action/deploy/TransportDeployModelAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ public TransportDeployModelAction(
protected void doExecute(Task task, ActionRequest request, ActionListener<MLDeployModelResponse> listener) {
MLDeployModelRequest deployModelRequest = MLDeployModelRequest.fromActionRequest(request);
String modelId = deployModelRequest.getModelId();
Boolean isUserInitiatedDeployRequest = deployModelRequest.isUserInitiatedDeployRequest();
User user = RestActionUtils.getUserContext(client);
boolean isSuperAdmin = isSuperAdminUserWrapper(clusterService, client);
String[] excludes = new String[] { MLModel.MODEL_CONTENT_FIELD, MLModel.OLD_MODEL_CONTENT_FIELD };
Expand All @@ -144,7 +145,7 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLDepl
throw new IllegalStateException(REMOTE_INFERENCE_DISABLED_ERR_MSG);
}
if (isHidden != null && isHidden) {
if (isSuperAdmin) {
if (isSuperAdmin || !isUserInitiatedDeployRequest) {
deployModel(deployModelRequest, mlModel, modelId, wrappedListener, listener);
} else {
wrappedListener
Expand Down
2 changes: 1 addition & 1 deletion plugin/src/main/java/org/opensearch/ml/autoredeploy/MLModelAutoReDeployer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ private void triggerModelRedeploy(ModelAutoRedeployArrangement modelAutoRedeploy
ImmutableMap.of(MLModel.AUTO_REDEPLOY_RETRY_TIMES_FIELD, Optional.ofNullable(autoRedeployRetryTimes).orElse(0) + 1)
);

MLDeployModelRequest deployModelRequest = new MLDeployModelRequest(modelId, nodeIds, false, true);
MLDeployModelRequest deployModelRequest = new MLDeployModelRequest(modelId, nodeIds, false, true, false);
client.execute(MLDeployModelAction.INSTANCE, deployModelRequest, listener);
}

Expand Down
2 changes: 1 addition & 1 deletion plugin/src/main/java/org/opensearch/ml/model/MLModelManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@ylwu-amzn ylwu-amzn Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ActionListener<MLDeployModelResponse> listener = ActionListener
.wrap(r -> log.debug("model deployed, response {}", r), e -> log.error("Failed to deploy model", e));
client.execute(MLDeployModelAction.INSTANCE, request, listener);
Expand Down
1 change: 1 addition & 0 deletions plugin/src/test/java/org/opensearch/ml/action/deploy/TransportDeployModelActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add unit test to cover isUserInitiatedDeployRequest set to false.

doAnswer(invocation -> {
ActionListener<MLModel> listener = invocation.getArgument(3);
listener.onResponse(mlModel);
Expand Down
Loading