updating request field for the connectors #1215

dhrubo-os · 2023-08-18T00:00:45Z

Description

[updating request field for the connectors]

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

codecov · 2023-08-18T01:03:58Z

Codecov Report

Merging #1215 (a48dba5) into 2.9 (1ffd6a5) will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                2.9    #1215      +/-   ##
============================================
+ Coverage     78.76%   78.81%   +0.04%     
- Complexity     2136     2138       +2     
============================================
  Files           168      168              
  Lines          8745     8745              
  Branches        877      877              
============================================
+ Hits           6888     6892       +4     
+ Misses         1456     1453       -3     
+ Partials        401      400       -1

Flag	Coverage Δ
ml-commons	`78.81% <ø> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

ylwu-amzn · 2023-08-18T01:56:36Z

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

+    protected String applyCustomSubstitution(Map<String, String> parameters, String inputPayload) {
+
+        if (parameters == null) {
+            throw new IllegalArgumentException("Some parameter placeholder not filled in payload: input");


This will add limitation that parameters can't be null. If inputPayload doesn't have placeholder, we don't need this checking and the error message is not reflecting correctly.

Yeah, that's a good point. We don't need to throw exception here. I was mostly trying to reflect this test case

ylwu-amzn · 2023-08-18T02:02:36Z

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

+        return result;
+    }
+
+    protected boolean isNotPlainString(String value) {


If user input {"key1": "123"}, we will detect "123" as not plain String?

From line 282
isNotPlainString(paramValue) ? paramValue : "\"" + paramValue + "\"";

It will return 123 rather than "123" which not what user expected.

So if user provides a number with "" ("123"), although it's a number we still should send value as "123"?

if user provide a number without "" (123), in that case I suppose we want to send the value as 123?

What about boolean values? Same idea?

For example "{ \"texts\": ${parameters.texts}, \"truncate\": ${parameters.truncate} }" , if user input "truncate": "123" in parameters when predict, will we pass \"truncate\": 123 to model with your PR?

austintlee · 2023-08-18T18:01:37Z

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

@@ -254,8 +256,7 @@ public  <T> T createPredictPayload(Map<String, String> parameters) {
        if (predictAction.isPresent() && predictAction.get().getRequestBody() != null) {
            String payload = predictAction.get().getRequestBody();
            payload = fillNullParameters(parameters, payload);
-            StringSubstitutor substitutor = new StringSubstitutor(parameters, "${parameters.", "}");
-            payload = substitutor.replace(payload);
+            payload = applyCustomSubstitution(parameters, payload);
            if (!isJson(payload)) {


In my local testing, I found this can return false on "valid" json. The issue was that parameter values needed to be escaped. I can make a separate PR for that, but thought Id mention it here since there's related code in this PR for escaping parameter values.

In my testing I saw, at this point payload is supposed to replaced with the actual value:

like: "{ \"texts\": ${parameters.texts}, \"truncate\": ${parameters.truncate} }" --> "{ \"texts\": [\"hello world\"], \"truncate\": \"END\" }" which is a valid JSON.

Could you please share your use case where you get false for a "valid" JSON?

I am basically taking a bunch of text from my OpenSearch cluster (as search results/hits) and passing it as part of the messages parameter to OpenAI (search hit -> message). I don't know which character in my text caused the issue, but to get around it, I applied a patch locally to always do escapeJson on the messages and got around the issue. So, what I am trying to say is that if the text is not properly escaped for Json, you can get IllegalArgumentException. I guess this is not a comment specific to your PR. I am not pointing out an issue with the custom substitution logic per se. Let me know if you want me to open a new issue to track this.

@austintlee I think you are using 2.x branch. We have some bug fix in 2.9 branch, we just backported them to 2.x branch. Please rebase your code and retest.

austintlee · 2023-08-18T18:03:12Z

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

+            }
+        }
+
+        result = StringEscapeUtils.unescapeJava(result); // Unescape Unicode escape sequences


Here, I am not sure if this is sufficient to avoid valid values causing isJson() to fail as I mentioned above.

Could you please explain this little bit more for me?

Yaliang answered my question above.

austintlee · 2023-08-18T18:03:36Z

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

+        }
+        String result = inputPayload;
+
+        Pattern pattern = Pattern.compile("\\$\\{parameters\\.(\\w+)\\}");


Maybe pull this out as a class static?

Sure, I can do that.

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:00 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:00 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:00 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:17 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:17 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:17 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:24 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:24 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env August 18, 2023 00:24 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env August 18, 2023 00:45 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env August 18, 2023 00:45 — with GitHub Actions Error

dhrubo-os temporarily deployed to ml-commons-cicd-env August 18, 2023 00:45 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env August 18, 2023 01:39 — with GitHub Actions Inactive

ylwu-amzn reviewed Aug 18, 2023

View reviewed changes

austintlee reviewed Aug 18, 2023

View reviewed changes

ylwu-amzn mentioned this pull request Aug 18, 2023

Use Search Pipeline processors, Remote Inference and HttpConnector to enable Retrieval Augmented Generation (RAG) #1195

Merged

dhrubo-os force-pushed the 2.9 branch from a48dba5 to 0d9e59c Compare July 24, 2024 23:08

dhrubo-os had a problem deploying to ml-commons-cicd-env July 24, 2024 23:08 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env July 24, 2024 23:08 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env July 24, 2024 23:08 — with GitHub Actions Failure

dhrubo-os closed this Jul 24, 2024

dhrubo-os force-pushed the 2.9 branch from 0d9e59c to 6300da6 Compare July 24, 2024 23:10

dhrubo-os had a problem deploying to ml-commons-cicd-env July 24, 2024 23:10 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env July 24, 2024 23:10 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env July 24, 2024 23:10 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating request field for the connectors #1215

updating request field for the connectors #1215

dhrubo-os commented Aug 18, 2023

codecov bot commented Aug 18, 2023 •

edited

Loading

ylwu-amzn Aug 18, 2023

dhrubo-os Aug 18, 2023

ylwu-amzn Aug 18, 2023

ylwu-amzn Aug 18, 2023

dhrubo-os Aug 18, 2023

ylwu-amzn Aug 18, 2023 •

edited

Loading

austintlee Aug 18, 2023

dhrubo-os Aug 18, 2023

austintlee Aug 18, 2023

ylwu-amzn Aug 18, 2023

austintlee Aug 18, 2023

dhrubo-os Aug 18, 2023

austintlee Aug 18, 2023

austintlee Aug 18, 2023

dhrubo-os Aug 18, 2023

updating request field for the connectors #1215

updating request field for the connectors #1215

Conversation

dhrubo-os commented Aug 18, 2023

Description

Issues Resolved

Check List

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

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

ylwu-amzn Aug 18, 2023 • edited Loading

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

codecov bot commented Aug 18, 2023 •

edited

Loading

ylwu-amzn Aug 18, 2023 •

edited

Loading