-
Notifications
You must be signed in to change notification settings - Fork 178
Add escape output parameter #4179
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
base: main
Are you sure you want to change the base?
Add escape output parameter #4179
Conversation
Signed-off-by: Hailong Cui <[email protected]>
c55e49a
to
14bcb07
Compare
Is this mainly for flow agent? |
Yes, that's right. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4179 +/- ##
============================================
+ Coverage 82.01% 82.03% +0.01%
- Complexity 9183 9197 +14
============================================
Files 789 789
Lines 39567 39574 +7
Branches 4389 4390 +1
============================================
+ Hits 32450 32463 +13
+ Misses 5227 5221 -6
Partials 1890 1890
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
common/src/main/java/org/opensearch/ml/common/utils/ToolUtils.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Hailong Cui <[email protected]>
870a9a9
to
97c269a
Compare
Run the unit test and integTest locally. Both are pass
|
* @return | ||
*/ | ||
public static String prepareJsonValue(String input) { | ||
public static String prepareJsonValue(String input, boolean escape) { |
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.
You need to change API documentation based on new behavior
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.
right, the documentation need update as well
public void prepareJsonValue_escapesBadCharsOtherwise() { | ||
String input = "Tom & Jerry \"<script>"; | ||
String escaped = StringUtils.prepareJsonValue(input); | ||
String escaped = StringUtils.prepareJsonValue(input, false); |
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.
Something is misleading here, you are actually escaping the plain text, but passing "escape" parameter as false. May be rename "escape" parameter in the API definition, which should understand it is only about json string.
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.
make sense, i will update it
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.
rename it to escapeJson
params | ||
.put( | ||
outputKey, | ||
StringUtils.prepareJsonValue(filteredOutput, Boolean.parseBoolean(toolParameters.getOrDefault(TOOL_ESCAPE_OUTPUT, "false"))) |
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.
are we expecting customer to provide this settings: TOOL_ESCAPE_OUTPUT
?
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.
yes, as tool parameters whebln configure the agent.
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.
An example
"user_inputs": {
"description": "Agent Tool",
"include_output_in_agent_response": True,
"type": "AgentTool",
"parameters": {
"max_iteration": "10",
"escape_output": "true"
},
"name": "AgentTool"
}
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.
Just trying to understand, based on what condition/experience customer will try to set True
/False
on this parameter? How will customer determine if they need to set this or not? This seems too much details for customer to understand.
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.
that depends on the use case, if the tool output will used to concat with other strings to build a prompt for LLM, that's a typical case we need to set it as true. That is the case for chatbot question suggestor which is a MLModelTool and leverage previous tool output to build the prompt for LLM
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.
Just trying to understand, based on what condition/experience customer will try to set
True
/False
on this parameter? How will customer determine if they need to set this or not? This seems too much details for customer to understand.
+1
Signed-off-by: Hailong Cui <[email protected]>
CI failed due to model throttling
|
Hi hailong, do you think this PR will help address the current issue: #4093 |
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.
Functionality looks good! Please address the other comments and add coverage if required.
Description
Add a new parameter to escape tool output,so that the tool output is escaped and able to used in MLModelTool as part of prompt
Related Issues
#4178
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.