Parameter Passing for Predict via Remote Connector #4121

Sq-eng · 2025-08-15T06:13:33Z

Description

This PR adds support for parameter propagation in the predict API when using text_embedding function by ensuring user-supplied parameters are correctly merged into the connector's request_body and included in the outgoing request.

Related Issues

Resolves #4105

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

xinyual · 2025-08-15T07:15:50Z

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

        return (T) parameters.get("http_body");
    }

+    public String removeMissingParameterFields(String payload, Map<String, String> params) {


Can you add description for this function?

I think we don't need remove the unused field. We should keep consistent behavior with previous code. I.e. if user set a parameter in template and don't provide values, we just keep it. Otherwise it may change the behavior at user side

I understand. I’ve updated the code to follow this logic – now it keeps the unused fields consistent with the previous behavior.

zhichao-aws · 2025-08-18T03:04:57Z

...rithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java

-import java.util.Map;
-import java.util.Optional;
+import java.io.IOException;
+import java.util.*;


we should aviod using import *, and restore to the previous import

zhichao-aws · 2025-08-18T03:05:08Z

...rithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java

 import org.opensearch.core.action.ActionListener;
 import org.opensearch.core.rest.RestStatus;
-import org.opensearch.core.xcontent.NamedXContentRegistry;
+import org.opensearch.core.xcontent.*;


same as above

zane-neo · 2025-08-26T05:21:00Z

...rithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java

+
+        HashMap<String, String> paramMap = new HashMap<>();
+        for (Map.Entry<String, Object> entry : tempMap.entrySet()) {
+            paramMap.put(entry.getKey(), entry.getValue() != null ? entry.getValue().toString() : null);


If the Object is a map/list type, the toString by default returns their address in memory instead of a json, it seems not correct here, do you have test cases to cover this case?

I’ve addressed this issue by updating the code so that all types except basic types and String are serialized to JSON strings. Also added a unit test.

zane-neo · 2025-08-26T05:21:53Z

Patch coverage is 60.00000% with 8 lines in your changes missing coverage.

The code coverage in this PR is little bit low, can you add more UTs to increase the coverage?

Sq-eng · 2025-08-26T05:47:11Z

Patch coverage is 60.00000% with 8 lines in your changes missing coverage.

The code coverage in this PR is little bit low, can you add more UTs to increase the coverage?

I've already added UT for it. But the Codecov Report hasn't been updated yet. Could you please allow CI/CD so it can rerun the test?

zane-neo · 2025-08-27T02:27:22Z

...rithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java

+        HashMap<String, String> paramMap = new HashMap<>();
+        for (Map.Entry<String, Object> entry : tempMap.entrySet()) {
+            Object value = entry.getValue();
+            if (value == null) {
+                paramMap.put(entry.getKey(), null);
+            } else if (value instanceof String) {
+                paramMap.put(entry.getKey(), value.toString());
+            } else {
+                try {
+                    String jsonValue = StringUtils.MAPPER.writeValueAsString(value);
+                    paramMap.put(entry.getKey(), jsonValue);
+                } catch (JsonProcessingException e) {
+                    paramMap.put(entry.getKey(), null);
+                }


This part can be replaced with the existing util method: getParameterMap in org.opensearch.ml.common.utils.StringUtils, right? A good practice in coding is reusing and avoid duplication.

Yes, you are right. I’ve updated the code to reuse getParameterMap from StringUtils to avoid duplication. Thanks for the suggestion!

Signed-off-by: Shiqi Xia <[email protected]>

Sq-eng requested review from b4sjoo, dhrubo-os, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners August 15, 2025 06:13

Xia added 3 commits August 15, 2025 14:52

poc

b69fab3

Signed-off-by: Shiqi Xia <[email protected]>

version that run correctly

f9270db

Signed-off-by: Shiqi Xia <[email protected]>

passed all UT

4cba96b

Signed-off-by: Shiqi Xia <[email protected]>

Sq-eng force-pushed the main branch 2 times, most recently from 62c5b93 to 42fc5a5 Compare August 15, 2025 07:04

Sq-eng had a problem deploying to ml-commons-cicd-env-require-approval August 15, 2025 07:06 — with GitHub Actions Failure

xinyual reviewed Aug 15, 2025

View reviewed changes

Sq-eng force-pushed the main branch from 42fc5a5 to 8e2a3f2 Compare August 15, 2025 08:16

Sq-eng had a problem deploying to ml-commons-cicd-env-require-approval August 15, 2025 08:17 — with GitHub Actions Error

Sq-eng had a problem deploying to ml-commons-cicd-env-require-approval August 15, 2025 08:17 — with GitHub Actions Failure

Sq-eng had a problem deploying to ml-commons-cicd-env-require-approval August 15, 2025 08:17 — with GitHub Actions Error

Sq-eng had a problem deploying to ml-commons-cicd-env-require-approval August 15, 2025 08:17 — with GitHub Actions Failure

zhichao-aws reviewed Aug 18, 2025

View reviewed changes

Sq-eng force-pushed the main branch from 8e2a3f2 to ea980e3 Compare August 18, 2025 03:20

Sq-eng requested a deployment to ml-commons-cicd-env-require-approval August 26, 2025 05:09 — with GitHub Actions Waiting

zane-neo reviewed Aug 26, 2025

View reviewed changes

Sq-eng force-pushed the main branch from 77a4890 to cfb4d67 Compare August 26, 2025 08:42

Sq-eng requested a deployment to ml-commons-cicd-env-require-approval August 26, 2025 08:43 — with GitHub Actions Waiting

zane-neo reviewed Aug 27, 2025

View reviewed changes

Sq-eng force-pushed the main branch from cfb4d67 to 60274be Compare August 27, 2025 09:37

Sq-eng temporarily deployed to ml-commons-cicd-env-require-approval August 27, 2025 09:38 — with GitHub Actions Inactive

zane-neo previously approved these changes Aug 28, 2025

View reviewed changes

Sq-eng temporarily deployed to ml-commons-cicd-env-require-approval August 28, 2025 04:27 — with GitHub Actions Inactive

Remove the removeMissParameterFields

d3c64e7

Signed-off-by: Shiqi Xia <[email protected]>

Sq-eng dismissed zane-neo’s stale review via d3c64e7 August 28, 2025 07:46

Sq-eng force-pushed the main branch from 60274be to d3c64e7 Compare August 28, 2025 07:46

Sq-eng temporarily deployed to ml-commons-cicd-env-require-approval August 28, 2025 07:47 — with GitHub Actions Inactive

Sq-eng temporarily deployed to ml-commons-cicd-env-require-approval August 28, 2025 09:01 — with GitHub Actions Inactive

zane-neo approved these changes Aug 28, 2025

View reviewed changes

xinyual approved these changes Aug 29, 2025

View reviewed changes

xinyual merged commit d077f31 into opensearch-project:main Aug 29, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Parameter Passing for Predict via Remote Connector #4121

Parameter Passing for Predict via Remote Connector #4121

Uh oh!

Sq-eng commented Aug 15, 2025 •

edited by xinyual

Loading

Uh oh!

xinyual Aug 15, 2025

Uh oh!

Sq-eng Aug 15, 2025

Uh oh!

zhichao-aws Aug 18, 2025

Uh oh!

Sq-eng Aug 22, 2025

Uh oh!

zhichao-aws Aug 18, 2025

Uh oh!

Sq-eng Aug 18, 2025

Uh oh!

zhichao-aws Aug 18, 2025

Uh oh!

Sq-eng Aug 18, 2025

Uh oh!

zane-neo Aug 26, 2025

Uh oh!

Sq-eng Aug 26, 2025

Uh oh!

zane-neo commented Aug 26, 2025

Uh oh!

Sq-eng commented Aug 26, 2025

Uh oh!

zane-neo Aug 27, 2025

Uh oh!

Sq-eng Aug 27, 2025

Uh oh!

Uh oh!

Uh oh!

Parameter Passing for Predict via Remote Connector #4121

Parameter Passing for Predict via Remote Connector #4121

Uh oh!

Conversation

Sq-eng commented Aug 15, 2025 • edited by xinyual Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zane-neo commented Aug 26, 2025

Uh oh!

Sq-eng commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sq-eng commented Aug 15, 2025 •

edited by xinyual

Loading