[Enhancement] Enhance validation for create connector API #3579

akolarkunnu · 2025-02-21T10:48:22Z

Description

This change will address the second part of validation "pre and post processing function validation".
Moved the method getRemoteServerFromURL() from ConnectorUtils.java to ConnectorAction.java, to avoid the cyclic dependency

Related Issues

Partially resolves #2993

Check List

New functionality includes testing.
API changes companion pull request created.
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.

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

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

akolarkunnu · 2025-02-26T02:06:42Z

Can anyone trigger the failed test suite once more, failure doesn't have any relation with changes.

This change will address the second part of validation "pre and post processing function validation". Partially resolves opensearch-project#2993 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

akolarkunnu · 2025-03-13T06:05:53Z

@zane-neo Can you please look in o these two failures, my changes around prepostProcessFunctions:
116 tests completed, 3 failed, 11 skipped

org.opensearch.ml.rest.RestBedRockInferenceIT.test_bedrock_embedding_v2_model_with_postProcessFunction
org.opensearch.ml.rest.RestCohereInferenceIT.test_cohereInference_withDifferent_postProcessFunction

I am not able to reproduce and debug these failures locally because of below dependencies:

The AWS credentials are not set. Skipping test.

COHERE_KEY is null, skipping the test!

zane-neo · 2025-03-13T06:10:40Z

@zane-neo Can you please look in o these two failures, my changes around prepostProcessFunctions: 116 tests completed, 3 failed, 11 skipped

org.opensearch.ml.rest.RestBedRockInferenceIT.test_bedrock_embedding_v2_model_with_postProcessFunction

org.opensearch.ml.rest.RestCohereInferenceIT.test_cohereInference_withDifferent_postProcessFunction

I am not able to reproduce and debug these failures locally because of below dependencies:

The AWS credentials are not set. Skipping test.

COHERE_KEY is null, skipping the test!

Please rebase the latest code on main branch, this has been fixed.

akolarkunnu · 2025-03-13T17:18:05Z

linux (23) - Known flaky org.opensearch.ml.rest.RestMLRemoteInferenceIT.testPredictWithAutoDeployAndTTL_RemoteModel #3544

linux (21) - There are no real failures

Windows (21) - org.opensearch.ml.rest.RestMLRAGSearchProcessorIT.testBM25WithBedrockConverse - not related the code changes

akolarkunnu · 2025-03-14T01:54:06Z

Hi Maintainers, Please help to move forward this task.
There are 2 skipped tasks, 1 cancelled task and 3 failed tasks, failures are not related to this changes.

pyek-bot · 2025-03-14T03:57:42Z

Hi Maintainers, Please help to move forward this task? There are 2 skipped tasks, 1 cancelled task and 3 failed tasks, failures are not related to this changes.

Hi @akolarkunnu, let me review this PR and test out some of the changes. In the meanwhile, the maintainers can help re-trigger the CI and we can check if it goes through? Thanks for the fix and sharing the test details!

...ithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutorTest.java

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

This change will address the second part of validation "pre and post processing function validation". Partially resolves opensearch-project#2993 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

akolarkunnu · 2025-03-18T05:13:10Z

Please approve for CI - "Waiting for review: ml-commons-cicd-env-require-approval needs approval to start deploying changes."

akolarkunnu · 2025-03-20T00:21:02Z

Hi Maintainers, @pyek-bot Please approve to run CI.

pyek-bot · 2025-03-20T06:32:25Z

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

+        }
+        switch (remoteServer) {
+            case OPENAI:
+                if (!preProcessFunction.contains(OPENAI)) {


a bit confused with the latest change, so now the only validation is if the preProcessFunction contains "openai" as opposed to "connector.post_process.default.embedding"? so this in theory a more lenient validation?

Yes, I did that change based on your previous comment - "thinking if for these constants we can create the string using the INBUILT_FUNC_PREFIX? and maybe even have preprocessprefix and postprocessprefix? this can avoid any accidental changes "
I can see only that's the only(eg: openai, bedrock, cohere) unique text in the function names which we can defer for different llm services.
Or you meant something differently ? Creating different constant arrays for each llm service post and pre Process Functions ? So in this case we will have 8 arrays, 4 (openai, bedrock, cohere, sagemaker) for pre process functions and 4 for post process functions ?

I believe there has been a confusion: #3579 (comment)

In this comment, I meant that we should create the constants as such:
MLPostProcessFunction.OPENAI_EMBEDDING = INBUILT_FUNC_PREFIX + "openai.embedding" + ACTION_POST_PROCESS_FUNCTION*

*just an example may not be fully code accurate

This way if either of these constants change, we don't have to change the code. With respect to whether the check should be lenient or not we can wait on more comments.

dhrubo-os · 2025-03-21T16:26:35Z

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

@@ -185,6 +198,90 @@ public static ConnectorAction parse(XContentParser parser) throws IOException {
            .build();
    }

+    public void validatePrePostProcessFunctions(Map<String, String> parameters) {


Can we please add java doc with more detailed explanations.

dhrubo-os · 2025-03-21T16:29:06Z

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

+        validatePostProcessFunctions(remoteServer);
+    }
+
+    private void validatePreProcessFunctions(String remoteServer) {


let's organize the private methods in the order it was being invoked.getRemoteServerFromURL and then validatePreProcessFunctions. Its easy to read for reviewers.

dhrubo-os · 2025-03-21T16:32:02Z

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

+        StringSubstitutor substitutor = new StringSubstitutor(parameters, "${parameters.", "}");
+        String endPoint = substitutor.replace(url);
+        String remoteServer = getRemoteServerFromURL(endPoint);
+        validatePreProcessFunctions(remoteServer);


if (isInBuiltProcessFunction(preProcessFunction)) then we can invoke validatePreProcessFunctions?

dhrubo-os · 2025-03-21T16:55:04Z

common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java

@@ -62,6 +85,257 @@ public void constructor_NullMethod() {
        assertEquals("method can't be null", exception.getMessage());
    }

+    @Test
+    public void connectorWithNullPreProcessFunction() {


Let's rename the unit tests. The name doesn't quite reflect the intent of the test. Same applies for other methods as well.

To be more clear Your test name should answer three things:

What is being tested? (method or scenario)
Under what conditions? (input or setup)
What is the expected outcome?

akolarkunnu requested review from b4sjoo, dhrubo-os, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners February 21, 2025 10:48

akolarkunnu requested a deployment to ml-commons-cicd-env-require-approval February 21, 2025 11:12 — with GitHub Actions Waiting

pyek-bot reviewed Feb 21, 2025

View reviewed changes

common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java Outdated Show resolved Hide resolved

pyek-bot reviewed Feb 21, 2025

View reviewed changes

common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java Outdated Show resolved Hide resolved

pyek-bot reviewed Feb 21, 2025

View reviewed changes

common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java Show resolved Hide resolved

pyek-bot reviewed Feb 21, 2025

View reviewed changes

common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java Show resolved Hide resolved

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval February 24, 2025 11:09 — with GitHub Actions Error

akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval February 24, 2025 11:09 — with GitHub Actions Inactive

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval February 24, 2025 11:09 — with GitHub Actions Failure

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval February 26, 2025 07:11 — with GitHub Actions Error

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval February 26, 2025 07:11 — with GitHub Actions Failure

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval February 26, 2025 19:35 — with GitHub Actions Failure

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 3, 2025 20:19 — with GitHub Actions Failure

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 3, 2025 20:19 — with GitHub Actions Error

[Enhancement] Enhance validation for create connector API

236fda2

This change will address the second part of validation "pre and post processing function validation". Partially resolves opensearch-project#2993 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 12, 2025 06:04 — with GitHub Actions Failure

akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval March 12, 2025 06:04 — with GitHub Actions Inactive

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 12, 2025 06:04 — with GitHub Actions Error

akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval March 12, 2025 06:04 — with GitHub Actions Inactive

Merge branch 'opensearch-project:main' into prepostfuncvalidation

953f16e

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 13, 2025 11:56 — with GitHub Actions Failure

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 13, 2025 11:56 — with GitHub Actions Error

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 13, 2025 11:56 — with GitHub Actions Failure

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 14, 2025 05:46 — with GitHub Actions Failure

akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval March 14, 2025 05:46 — with GitHub Actions Inactive

pyek-bot reviewed Mar 14, 2025

View reviewed changes

[Enhancement] Enhance validation for create connector API

4e5455c

This change will address the second part of validation "pre and post processing function validation". Partially resolves opensearch-project#2993 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval March 17, 2025 09:12 — with GitHub Actions Inactive

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 17, 2025 09:12 — with GitHub Actions Error

akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval March 17, 2025 09:12 — with GitHub Actions Inactive

akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval March 17, 2025 09:12 — with GitHub Actions Failure

pyek-bot reviewed Mar 20, 2025

View reviewed changes

dhrubo-os reviewed Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Enhance validation for create connector API #3579

[Enhancement] Enhance validation for create connector API #3579

akolarkunnu commented Feb 21, 2025

akolarkunnu commented Feb 26, 2025

akolarkunnu commented Mar 13, 2025

zane-neo commented Mar 13, 2025

The AWS credentials are not set. Skipping test.

akolarkunnu commented Mar 13, 2025

akolarkunnu commented Mar 14, 2025 •

edited

Loading

pyek-bot commented Mar 14, 2025

akolarkunnu commented Mar 18, 2025 •

edited

Loading

akolarkunnu commented Mar 20, 2025

pyek-bot Mar 20, 2025

akolarkunnu Mar 20, 2025

pyek-bot Mar 21, 2025

dhrubo-os Mar 21, 2025

dhrubo-os Mar 21, 2025

dhrubo-os Mar 21, 2025

dhrubo-os Mar 21, 2025

dhrubo-os Mar 21, 2025

[Enhancement] Enhance validation for create connector API #3579

Are you sure you want to change the base?

[Enhancement] Enhance validation for create connector API #3579

Conversation

akolarkunnu commented Feb 21, 2025

Description

Related Issues

Check List

akolarkunnu commented Feb 26, 2025

akolarkunnu commented Mar 13, 2025

The AWS credentials are not set. Skipping test.

zane-neo commented Mar 13, 2025

The AWS credentials are not set. Skipping test.

akolarkunnu commented Mar 13, 2025

akolarkunnu commented Mar 14, 2025 • edited Loading

pyek-bot commented Mar 14, 2025

akolarkunnu commented Mar 18, 2025 • edited Loading

akolarkunnu commented Mar 20, 2025

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

akolarkunnu commented Mar 14, 2025 •

edited

Loading

akolarkunnu commented Mar 18, 2025 •

edited

Loading