-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support list in response body #2811
Support list in response body #2811
Conversation
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Outdated
Show resolved
Hide resolved
I assume failing tests are related to this change? |
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Outdated
Show resolved
Hide resolved
match by regex expression is not a good way and it only solves the type casting from jsonstring(list) into a jsonstring(escape list). I will rethink about a better way to do this |
Signed-off-by: Mingshi Liu <[email protected]>
86d4598
to
3860c47
Compare
3860c47
to
fd25bc3
Compare
came up with a better solution to try fix the invalid payload by adding escape, in this fix, we don't worry if the parameter is a list or a map, we try help to fix the invalid payload by adding escape, if adding escape helps, return the payload after adding escape, please help review @Zhangxunmt @dhrubo-os |
Test is failing |
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Show resolved
Hide resolved
Signed-off-by: Mingshi Liu <[email protected]> try help escape when the payload is not valid Signed-off-by: Mingshi Liu <[email protected]>
fd25bc3
to
83572b5
Compare
@dhrubo-os any ideas for the linux test failure? I saw the window tests passed all the way through. This failure is not related to the code changes in this PR. |
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mingshi Liu <[email protected]>
99d3796
to
b465bed
Compare
all tests passed, merging now. |
(cherry picked from commit f64e3f3)
} | ||
|
||
private static String escapeValue(String value) { | ||
return value.replace("\\", "\\\\").replace("\"", "\\\"").replace("\n", "\\n").replace("\r", "\\r").replace("\t", "\\t"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use existing escape method? This escape method will replace "\n" in "This is a \new book" , which is not necessary. It may confuse user why same sentence will generate different embeddings after this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it won't be a break change, because before this change, if it's an invalid payload, it will throw exception directly, it won't generate embedding at all.
if (!isJson(payload)) { | ||
throw new IllegalArgumentException("Invalid payload: " + payload); | ||
String payloadAfterEscape = connectorAction.get().getRequestBody(); | ||
Map<String, String> escapedParameters = escapeMapValues(parameters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If payload is wrong will escape all parameters again. Escape good parameters again may generate wrong result. For example, the original prompt is "please replace all \n of the context with --", if we escape again , it becomes "please replace all \\n of the context with --"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a string is already partial escaped, meaning the string escaped for the next line symbol but doesn't escape for double quote, this change will still not fix the payload. Please let me know if there is better way to identify partial escape cases.
(cherry picked from commit f64e3f3) Co-authored-by: Mingshi Liu <[email protected]>
This reverts commit f64e3f3
This reverts commit f64e3f3 Signed-off-by: Mingshi Liu <[email protected]>
Description
When an array as a string in parameter, the string in list are not escaped.
This PR will escape the string when it's a json array.
for example, before this change:
Related Issues
#2839
Check List
--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.