-
Notifications
You must be signed in to change notification settings - Fork 138
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
add action input as parameters for tool execution in conversational agent #3200
base: main
Are you sure you want to change the base?
Conversation
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/AgentUtils.java
Outdated
Show resolved
Hide resolved
tests passed but failed in upload, not related to this code change. Approved.
|
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/AgentUtils.java
Outdated
Show resolved
Hide resolved
The changes for AgentUtils look fine, but is AgentUtils used for conversational agents? The tool parameters are build inside I showed a code proposal for I would also highly recommend to add the new "action_input" parameter to flow agents (i.e. |
…nversational agent Signed-off-by: Jing Zhang <jngz@amazon.com>
Hi @reuschling , thanks for the comments. Yeah, you are right. The changes is only for conversational agents. As you also mentioned, the |
Signed-off-by: Jing Zhang <jngz@amazon.com>
Sorry, but I think you misunderstood me. Currently this PR makes NO change to conversational agents. It changes
Exactly. But when someone wants to use the same tool inside a conversational agent and inside a flow agent, he/she has to change the tool definition from |
@reuschling Thanks for reviewing this PR. From #2918 (comment), I think you know "MLChatAgentRunner uses the AgentUtils method AgentUtils.constructToolParams for generating the params for a tool." The name seems confusing, but actually TestI have tested this PR with Bedrock anthropic.claude-instant-v1 model.
Test agent with
Feel free to test. BTW, you can find @jngz-es and me in the public OpenSearch ml Slack channel https://join.slack.com/t/opensearch/shared_invite/zt-2r5scz3ty-5SMPhqJE_Lk2HqC6ex4mWg, welcome to join. I'm ok to jump to a call if that's easier to explain details with a demo. |
@@ -472,6 +472,11 @@ public static Map<String, String> constructToolParams( | |||
if (toolSpecConfigMap != null) { | |||
toolParams.putAll(toolSpecConfigMap); | |||
} | |||
toolParams.put("llm_generated_action_input", actionInput); |
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.
Maybe no need to mention action explicitly considering REST API uses tool. User may feel confused about tool and action. How about just llm_generated_input
?
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.
can also consider using constant for this string since it is reused in the tests
Thanks a lot @ylwu-amzn for clarification with the code link. Yes the name confused me, sorry about that @jngz-es . In this case I'm also fine with the changes :) |
Description
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
#3134
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.