Enhance batch job task management by adding default action types #3080

rbhavna · 2024-10-08T18:21:07Z

Description

Enhance batch job task management by adding default action types

TODO: add test coverage to changes in this PR

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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 <[email protected]>

Zhangxunmt · 2024-10-09T18:22:09Z

LGTM overall. Please apply spotLess and add UT coverages to pass CI. Also please define createConnectorAction() in the Util class so it can be shared across multiple classes.

Zhangxunmt · 2024-10-09T18:22:47Z

common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java

@@ -36,6 +37,7 @@ public class ConnectorAction implements ToXContentObject, Writeable {
    public static final String ACTION_PRE_PROCESS_FUNCTION = "pre_process_function";
    public static final String ACTION_POST_PROCESS_FUNCTION = "post_process_function";

+


No need to add this empty line.

Zhangxunmt · 2024-10-09T18:26:37Z

common/src/main/java/org/opensearch/ml/common/connector/Connector.java

@@ -65,6 +65,8 @@ public interface Connector extends ToXContentObject, Writeable {

    List<ConnectorAction> getActions();

+    void setAction(ConnectorAction action);


Using addAction as the name is more accurate?

dhrubo-os · 2024-10-09T18:32:49Z

common/src/main/java/org/opensearch/ml/common/output/MLPredictionOutput.java

@@ -29,9 +30,11 @@ public class MLPredictionOutput extends MLOutput {
    public static final String TASK_ID_FIELD = "task_id";
    public static final String STATUS_FIELD = "status";
    public static final String PREDICTION_RESULT_FIELD = "prediction_result";
+    public static final String REMOTE_JOB_FIELD = "remote_job";


let's add a comment about this field.

dhrubo-os · 2024-10-09T18:37:39Z

plugin/src/main/java/org/opensearch/ml/action/tasks/CancelBatchJobTransportAction.java

+    }
+
+    private String getEndpointType(String url) {
+        if (url.contains("sagemaker"))


can we make static variables for all these values and then re-use?

dhrubo-os · 2024-10-09T18:38:29Z

plugin/src/main/java/org/opensearch/ml/action/tasks/GetTaskTransportAction.java

+
+    }
+
+    private String getEndpointType(String url) {


why do we need same method 2 times?

Yes will be moving the common methods to ConnectorUtils class

Signed-off-by: Bhavana Ramaram <[email protected]>

Zhangxunmt · 2024-10-09T21:36:58Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtils.java

+            case "sagemaker":
+                url = isCancelAction
+                    ? predictEndpoint.replace("CreateTransformJob", "StopTransformJob")
+                    : predictEndpoint.replace("CreateTransformJob", "DescribeTransformJob");
+                requestBody = "{ \"TransformJobName\" : \"${parameters.TransformJobName}\"}";
+                break;
+            case "openai":
+            case "cohere":
+                url = isCancelAction ? predictEndpoint + "/${parameters.id}/cancel" : predictEndpoint + "/${parameters.id}";
+                method = isCancelAction ? "POST" : "GET";
+                break;
+            case "bedrock":
+                url = isCancelAction
+                    ? predictEndpoint + "/${parameters.processedJobArn}/stop"
+                    : predictEndpoint + "/${parameters.processedJobArn}";
+                method = isCancelAction ? "POST" : "GET";
+                break;


We should add the default branch for this switch statement to return null. In the GetTask and CancelTask, if the ConnectorAction is null, throw an exception with meaning logs like "please provide GetTask/CancelTask action in the connector".

oh sorry I added it but I guess it got missed during refactoring. Let me add it

Zhangxunmt · 2024-10-09T21:38:32Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtils.java

+    public static final List<String> SUPPORTED_REMOTE_SERVERS_FOR_DEFAULT_ACTION_TYPES = List
+        .of("sagemaker", "openai", "bedrock", "cohere");
+


Ideally if a new platform is used but not listed here, CX should still be able to GetTask and CancelTask by manually adding the actions in the connector. But seems this is not the case in this PR?

Yes if the connector already has the actions configured, then they can get/cancel task for any platform. Only if no action is provided, they we perform this check

Signed-off-by: Bhavana Ramaram <[email protected]>

opensearch-trigger-bot · 2024-10-09T22:53:32Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3080-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ff6fe67b6b913b8c746faac04fc7e2f2b5d184e7
# Push it to GitHub
git push --set-upstream origin backport/backport-3080-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3080-to-2.x.

opensearch-trigger-bot · 2024-10-09T22:53:41Z

The backport to 2.17 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.17 2.17
# Navigate to the new working tree
cd .worktrees/backport-2.17
# Create a new branch
git switch --create backport/backport-3080-to-2.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ff6fe67b6b913b8c746faac04fc7e2f2b5d184e7
# Push it to GitHub
git push --set-upstream origin backport/backport-3080-to-2.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.17

Then, create a pull request where the base branch is 2.17 and the compare/head branch is backport/backport-3080-to-2.17.

…nsearch-project#3080) * enhance batch job task management by adding default action types Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit ff6fe67)

…) (#3086) * enhance batch job task management by adding default action types Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit ff6fe67)

…) (#3085) * enhance batch job task management by adding default action types Signed-off-by: Bhavana Ramaram <[email protected]> (cherry picked from commit ff6fe67)

enhance batch job task management by adding default action types

5556080

Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27, sam-herman and xinyual as code owners October 8, 2024 18:21

rbhavna had a problem deploying to ml-commons-cicd-env October 8, 2024 18:21 — with GitHub Actions Failure

Zhangxunmt reviewed Oct 9, 2024

View reviewed changes

dhrubo-os reviewed Oct 9, 2024

View reviewed changes

rbhavna added 2 commits October 9, 2024 15:21

add unit tests

17cbad0

Signed-off-by: Bhavana Ramaram <[email protected]>

address comments

0bdc387

Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna temporarily deployed to ml-commons-cicd-env October 9, 2024 20:35 — with GitHub Actions Inactive

rbhavna temporarily deployed to ml-commons-cicd-env October 9, 2024 21:34 — with GitHub Actions Inactive

Zhangxunmt reviewed Oct 9, 2024

View reviewed changes

add default switch case

14a1188

Signed-off-by: Bhavana Ramaram <[email protected]>

rbhavna temporarily deployed to ml-commons-cicd-env October 9, 2024 22:05 — with GitHub Actions Inactive

rbhavna had a problem deploying to ml-commons-cicd-env October 9, 2024 22:05 — with GitHub Actions Failure

Zhangxunmt approved these changes Oct 9, 2024

View reviewed changes

dhrubo-os approved these changes Oct 9, 2024

View reviewed changes

rbhavna merged commit ff6fe67 into opensearch-project:main Oct 9, 2024
5 of 6 checks passed

rbhavna added backport 2.x 2.17 backport 2.17 and removed 2.17 labels Oct 9, 2024

rbhavna mentioned this pull request Oct 9, 2024

[Backport 2.x] Enhance batch job task management by adding default action types (#3080) #3085

Merged

5 tasks

rbhavna mentioned this pull request Oct 9, 2024

[Backport 2.17] Enhance batch job task management by adding default action types (#3080) #3086

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance batch job task management by adding default action types #3080

Enhance batch job task management by adding default action types #3080

rbhavna commented Oct 8, 2024 •

edited

Loading

Zhangxunmt commented Oct 9, 2024

Zhangxunmt Oct 9, 2024

Zhangxunmt Oct 9, 2024

dhrubo-os Oct 9, 2024

dhrubo-os Oct 9, 2024

dhrubo-os Oct 9, 2024

rbhavna Oct 9, 2024

Zhangxunmt Oct 9, 2024

rbhavna Oct 9, 2024

Zhangxunmt Oct 9, 2024

rbhavna Oct 9, 2024

opensearch-trigger-bot bot commented Oct 9, 2024

opensearch-trigger-bot bot commented Oct 9, 2024

		@@ -36,6 +37,7 @@ public class ConnectorAction implements ToXContentObject, Writeable {
		public static final String ACTION_PRE_PROCESS_FUNCTION = "pre_process_function";
		public static final String ACTION_POST_PROCESS_FUNCTION = "post_process_function";

		@@ -65,6 +65,8 @@ public interface Connector extends ToXContentObject, Writeable {

		List<ConnectorAction> getActions();

		void setAction(ConnectorAction action);

		public static final List<String> SUPPORTED_REMOTE_SERVERS_FOR_DEFAULT_ACTION_TYPES = List
		.of("sagemaker", "openai", "bedrock", "cohere");

Enhance batch job task management by adding default action types #3080

Enhance batch job task management by adding default action types #3080

Conversation

rbhavna commented Oct 8, 2024 • edited Loading

Description

TODO: add test coverage to changes in this PR

Related Issues

Check List

Zhangxunmt commented Oct 9, 2024

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

opensearch-trigger-bot bot commented Oct 9, 2024

opensearch-trigger-bot bot commented Oct 9, 2024

rbhavna commented Oct 8, 2024 •

edited

Loading