From 610796a0c17b5c309ca55e1754150dcf9ecd69f4 Mon Sep 17 00:00:00 2001 From: Brian Flores Date: Mon, 23 Sep 2024 17:26:06 -0700 Subject: [PATCH 1/2] Prevents future UT build issues addresses #2967 Depending on the GitHub CI workflow executes it will invoke ./gradlew build which will run tests on random parameters everytime, there are local-code's that will break a build one of them being az-Cyrl. The solution here was preventing (explicitly writing the local to be set to en-US) Github actions building on these random parameters that will interupt a build and lose time. Manual testing was done to prove that using this flag breaks a build, you can check the issue #2967 or you can run ./gradlew build -Dtests.local=az-Cyrl to see it breaks Signed-off-by: Brian Flores --- .github/workflows/CI-workflow.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/CI-workflow.yml b/.github/workflows/CI-workflow.yml index debe6958b6..108911fdef 100644 --- a/.github/workflows/CI-workflow.yml +++ b/.github/workflows/CI-workflow.yml @@ -65,7 +65,7 @@ jobs: export COHERE_KEY=`aws secretsmanager get-secret-value --secret-id github_cohere_key --query SecretString --output text` && echo "::add-mask::$OPENAI_KEY" && echo "::add-mask::$COHERE_KEY" && - echo "build and run tests" && ./gradlew build && + echo "build and run tests" && ./gradlew build -Dtests.locale=en-US && echo "Publish to Maven Local" && ./gradlew publishToMavenLocal && echo "Multi Nodes Integration Testing" && ./gradlew integTest -PnumNodes=3' plugin=`basename $(ls plugin/build/distributions/*.zip)` From e4eded413cf12fe4ca0af83a53d23702ee46142d Mon Sep 17 00:00:00 2001 From: Brian Flores Date: Tue, 24 Sep 2024 12:06:54 -0700 Subject: [PATCH 2/2] Updates current codebase with Local.Root to string operations The previous commit made a hard change on the build while ignoring the root problem, which was making sure that our codebase currently supports string operations regardless of the locale code. In this new commit String operations like toUpperCase have a extra argument of Locale.Root making the codebase agnostic to the rules of other langugages such as Spanish or Turkish. Manual testing was done like raised in the GitHub issue #2967 also ./gradlew build -Dtests.Local=az-Cyrl passes Signed-off-by: Brian Flores --- .github/workflows/CI-workflow.yml | 2 +- .../org/opensearch/ml/common/connector/ConnectorAction.java | 2 +- .../input/parameter/clustering/RCFSummarizeParams.java | 3 ++- .../ml/engine/algorithms/agent/MLAgentExecutor.java | 3 ++- .../ml/action/batch/TransportBatchIngestionAction.java | 5 +++-- .../java/org/opensearch/ml/rest/RestMLPredictionAction.java | 3 ++- 6 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/CI-workflow.yml b/.github/workflows/CI-workflow.yml index 108911fdef..debe6958b6 100644 --- a/.github/workflows/CI-workflow.yml +++ b/.github/workflows/CI-workflow.yml @@ -65,7 +65,7 @@ jobs: export COHERE_KEY=`aws secretsmanager get-secret-value --secret-id github_cohere_key --query SecretString --output text` && echo "::add-mask::$OPENAI_KEY" && echo "::add-mask::$COHERE_KEY" && - echo "build and run tests" && ./gradlew build -Dtests.locale=en-US && + echo "build and run tests" && ./gradlew build && echo "Publish to Maven Local" && ./gradlew publishToMavenLocal && echo "Multi Nodes Integration Testing" && ./gradlew integTest -PnumNodes=3' plugin=`basename $(ls plugin/build/distributions/*.zip)` diff --git a/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java b/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java index b62337d49f..4a7555d69b 100644 --- a/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java +++ b/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java @@ -208,7 +208,7 @@ public static boolean isValidActionInModelPrediction(ActionType actionType) { public static boolean isValidAction(String action) { try { - ActionType.valueOf(action.toUpperCase()); + ActionType.valueOf(action.toUpperCase(Locale.ROOT)); return true; } catch (IllegalArgumentException e) { return false; diff --git a/common/src/main/java/org/opensearch/ml/common/input/parameter/clustering/RCFSummarizeParams.java b/common/src/main/java/org/opensearch/ml/common/input/parameter/clustering/RCFSummarizeParams.java index 3956514b47..68ce2eec78 100644 --- a/common/src/main/java/org/opensearch/ml/common/input/parameter/clustering/RCFSummarizeParams.java +++ b/common/src/main/java/org/opensearch/ml/common/input/parameter/clustering/RCFSummarizeParams.java @@ -8,6 +8,7 @@ import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken; import java.io.IOException; +import java.util.Locale; import org.opensearch.core.ParseField; import org.opensearch.core.common.io.stream.StreamInput; @@ -97,7 +98,7 @@ public static MLAlgoParams parse(XContentParser parser) throws IOException { parallel = parser.booleanValue(); break; case DISTANCE_TYPE_FIELD: - distanceType = DistanceType.from(parser.text().toUpperCase()); + distanceType = DistanceType.from(parser.text().toUpperCase(Locale.ROOT)); break; default: parser.skipChildren(); diff --git a/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLAgentExecutor.java b/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLAgentExecutor.java index ec5031b595..73876990ed 100644 --- a/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLAgentExecutor.java +++ b/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLAgentExecutor.java @@ -13,6 +13,7 @@ import java.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.Map; import org.opensearch.ResourceNotFoundException; @@ -281,7 +282,7 @@ private ActionListener createAgentActionListener( @VisibleForTesting protected MLAgentRunner getAgentRunner(MLAgent mlAgent) { - final MLAgentType agentType = MLAgentType.from(mlAgent.getType().toUpperCase()); + final MLAgentType agentType = MLAgentType.from(mlAgent.getType().toUpperCase(Locale.ROOT)); switch (agentType) { case FLOW: return new MLFlowAgentRunner(client, settings, clusterService, xContentRegistry, toolFactories, memoryFactoryMap); diff --git a/plugin/src/main/java/org/opensearch/ml/action/batch/TransportBatchIngestionAction.java b/plugin/src/main/java/org/opensearch/ml/action/batch/TransportBatchIngestionAction.java index 6fd03b7b52..236f535348 100644 --- a/plugin/src/main/java/org/opensearch/ml/action/batch/TransportBatchIngestionAction.java +++ b/plugin/src/main/java/org/opensearch/ml/action/batch/TransportBatchIngestionAction.java @@ -14,6 +14,7 @@ import java.time.Instant; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -93,7 +94,7 @@ protected void doExecute(Task task, ActionRequest request, ActionListener { executeWithErrorHandling(() -> { double successRate = ingestable.ingest(mlBatchIngestionInput); @@ -185,7 +186,7 @@ private void validateBatchIngestInput(MLBatchIngestionInput mlBatchIngestionInpu if (dataSources.get(TYPE) == null || dataSources.get(SOURCE) == null) { throw new IllegalArgumentException("The batch ingest input data source is missing data type or source"); } - if (((String) dataSources.get(TYPE)).toLowerCase() == "s3") { + if (((String) dataSources.get(TYPE)).equalsIgnoreCase("s3")) { List s3Uris = (List) dataSources.get(SOURCE); if (s3Uris == null || s3Uris.isEmpty()) { throw new IllegalArgumentException("The batch ingest input s3Uris is empty"); diff --git a/plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java b/plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java index 72b841eb7b..acf9834350 100644 --- a/plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java +++ b/plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java @@ -129,7 +129,8 @@ MLPredictionTaskRequest getRequest(String modelId, String algorithm, RestRequest ActionType actionType = ActionType.from(getActionTypeFromRestRequest(request)); if (FunctionName.REMOTE.name().equals(algorithm) && !mlFeatureEnabledSetting.isRemoteInferenceEnabled()) { throw new IllegalStateException(REMOTE_INFERENCE_DISABLED_ERR_MSG); - } else if (FunctionName.isDLModel(FunctionName.from(algorithm.toUpperCase())) && !mlFeatureEnabledSetting.isLocalModelEnabled()) { + } else if (FunctionName.isDLModel(FunctionName.from(algorithm.toUpperCase(Locale.ROOT))) + && !mlFeatureEnabledSetting.isLocalModelEnabled()) { throw new IllegalStateException(LOCAL_MODEL_DISABLED_ERR_MSG); } else if (!ActionType.isValidActionInModelPrediction(actionType)) { throw new IllegalArgumentException("Wrong action type in the rest request path!");