add config field in MLToolSpec for static parameters #2977

jngz-es · 2024-09-23T18:09:34Z

Description

Add config field in MLToolSpec to support static parameters in tool execution.

An example from the issue 2918.

POST /_plugins/_ml/agents/_register
{
	"name": "Test Agent",
	"type": "conversational",
	"description": "test agent",
	"llm": {
		"model_id": "<model id>",
		"parameters": {
      "max_iteration": 5,
      "stop_when_no_tool_found": true,
      "response_filter": "$.completion"
    }
	},
	"memory": {
		"type": "conversation_index"
	},
	"app_type": "chat",
	"tools": [
		{
			"type": "SearchIndexTool",
			"description": "A tool to search opensearch index with natural language question.",
			"include_output_in_agent_response": true,
			"config": {
				"input": "{\"index\": \"sample-index\", \"query\": {\"query\": { \"match_all\": {}}} }"
			}
		}
	]
}

Related Issues

#2836
#2918

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: Jing Zhang <[email protected]>

dbwiddis

Can you open an issue on Flow Framework repo amplifying what changed here? We will need to update the ToolStep to account for a change in API. Better yet, open a PR with the changes!

Also please make sure you update the API specification which Flow Framework will be using to validate these API parameters. (It's a checklist item at the top of this PR.)

CC: @owaiskazi19 @amitgalitz @junweid62 @ylwu-amzn

common/src/main/java/org/opensearch/ml/common/agent/MLToolSpec.java

jngz-es · 2024-09-23T20:04:06Z

The flow framework issue, opensearch-project/flow-framework#878.

Signed-off-by: Jing Zhang <[email protected]>

common/src/main/java/org/opensearch/ml/common/agent/MLToolSpec.java

common/src/test/java/org/opensearch/ml/common/agent/MLAgentTest.java

common/src/test/java/org/opensearch/ml/common/agent/MLToolSpecTest.java

Signed-off-by: Jing Zhang <[email protected]>

yuye-aws · 2024-09-24T09:08:39Z

@reuschling Can you help review this PR?

yuye-aws · 2024-09-24T09:14:34Z

@jngz-es Can you please provide an example on how to use tool config?

Zhangxunmt · 2024-10-16T21:39:00Z

common/src/main/java/org/opensearch/ml/common/agent/MLToolSpec.java

+        String description,
+        Map<String, String> parameters,
+        boolean includeOutputInAgentResponse,
+        Map<String, String> configMap


Is it valid that all the configs are Strings? Is it better to use Map<String, Object> here?

-1 on that. The parameters map is already a Map<String, String>. In the initial PR comment an example is given of a much more complex "object" but escaped as a String.

Trying to parse anything else from Flow Framework will get a lot more complex without typed objects to construct from the JSON.

dhrubo-os · 2024-10-16T22:31:37Z

common/src/main/java/org/opensearch/ml/common/agent/MLToolSpec.java

@@ -56,6 +70,9 @@ public MLToolSpec(StreamInput input) throws IOException {
            parameters = input.readMap(StreamInput::readString, StreamInput::readOptionalString);
        }
        includeOutputInAgentResponse = input.readBoolean();
+        if (input.getVersion().onOrAfter(MINIMAL_SUPPORTED_VERSION_FOR_TOOL_CONFIG) && input.readBoolean()) {
+            configMap = input.readMap(StreamInput::readString, StreamInput::readOptionalString);


why readOptionalString ? so a key might not have a value?

Yes, this allows you to override a value with a non-value.

dhrubo-os · 2024-10-16T22:35:28Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/AgentUtils.java

        if (tools.get(action).useOriginalInput()) {
            toolParams.put("input", question);
            lastActionInput.set(question);
+        } else if (toolSpecConfigMap != null && toolSpecConfigMap.containsKey("input")) {


Don't we have any test class for this?

dhrubo-os · 2024-10-17T17:20:43Z

...src/main/java/org/opensearch/ml/engine/algorithms/agent/MLConversationalFlowAgentRunner.java

@@ -428,12 +428,18 @@ Map<String, String> getToolExecuteParams(MLToolSpec toolSpec, Map<String, String
            }
        }

+        // Override all parameters in tool config to tool execution parameters as the config contains the static parameters.


The comment isn’t quite clear to me. Could we try to explain it more clearly?

dhrubo-os · 2024-10-17T17:22:37Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLFlowAgentRunner.java

@@ -287,13 +287,18 @@ Map<String, String> getToolExecuteParams(MLToolSpec toolSpec, Map<String, String
                executeParams.put(key, params.get(key));
            }
        }
+        // Override all parameters in tool config to tool execution parameters as the config contains the static parameters.
+        if (toolSpec.getConfigMap() != null && !toolSpec.getConfigMap().isEmpty()) {


rather than putting same piece of code, can we try to use a common method getToolExecuteParams for both MLFlowAgentRunner & MLConversationalFlowAgentRunner?

yuye-aws · 2024-10-18T00:27:55Z

Pinging @reuschling to review the PR.

dbwiddis · 2024-10-18T05:54:46Z

Pinging @reuschling to review the PR.

Thanks @yuye-aws for prompting review on this. Thanks @Zhangxunmt for approval. Thanks @dhrubo-os for your initial review.

Code freeze / 2.18 RC generation is next Tuesday Oct 22. Downstream plugin PRs (opensearch-project/flow-framework#899) are dependent on this one being merged and need time to update and consume tne new snapshots, get proper reviews, and merge before code freeze.

Please do not wait until the last minute to get this merged; last-minute PR merges have occurred in at least 2 previous releases and this one's been open for a few weeks. I'm sensing a pattern here.

CC: @minalsha @ylwu-amzn

reuschling · 2024-10-18T14:13:35Z

...src/main/java/org/opensearch/ml/engine/algorithms/agent/MLConversationalFlowAgentRunner.java

        if (executeParams.containsKey("input")) {
            String input = executeParams.get("input");
            StringSubstitutor substitutor = new StringSubstitutor(executeParams, "${parameters.", "}");
            input = substitutor.replace(input);
            executeParams.put("input", input);
        }
+


I'm not sure if we have a chicken-and-egg problem here regarding the 'input' parameter. In #2918 'SearchIndexTool inside conversational agent', there is the need of a placeholder for the last output (i.e. action input) of the former agent step. This output/action input must be inserted into the OpenSearch query defined statically inside the new config, by placeholder.

There exists two possibilities for this:

There is an existing placeholder reference to the last output/action input that can be used, i.e. a dedicated parameter list entry. I'm not aware of such an entry, but if such a placeholder exists, the current solution would fix this scenario with a static config of e.g. ["input='"query": {"match": {"body": {"query": "${someUnknownPlaceholderToActionInput}"}}'"].

There is no such a placeholder, the last output/action input is just the 'input' entry in the current 'params' method parameter - so the placeholder inside the static config would be 'input'. This value inside 'params' is currently overwritten with 'input' from the static config, before substitution. So it is lost, right?

The final behavior should be as follows:

method params: "input='What is Frodos age?'" (comes from former agent step)

static config params "input='"query": {"match": {"body": {"query": "${parameters.input}"}}'"

=> final params for tool execution should be then
"input='"query": {"match": {"body": {"query": "What is Frodos age?"}}'"

This could be achieved as follows:

// remember former input String formerInput = executeParams.get("input"); if (toolSpec.getConfigMap() != null && !toolSpec.getConfigMap().isEmpty()) { executeParams.putAll(toolSpec.getConfigMap()); } if (executeParams.containsKey("input")) { String input = executeParams.get("input"); // set former input as substitution value if(formerInput != null) executeParams.put("input", formerInput); // add it as "formerInput" or "actionInput" to introduce a new placeholder StringSubstitutor substitutor = new StringSubstitutor(executeParams, "${parameters.", "}"); input = substitutor.replace(input); executeParams.put("input", input);

The lack is that you can not reference to the input specified in the new static config anymore, which is maybe wanted behavior. To overcome this, we could introduce a new placeholder for the former input (aka the placeholder I mentioned in 1.). This would be a new parameter list entry, e.g. 'formerInput' (just add the formerInput with executeParams.put("formerInput", formerInput);).

The static config could be defined now as ["input='"query": {"match": {"body": {"query": "${parameters.formerInput}"}}'"].

Another name for 'formerInput' could be 'actionInput', maybe this is more intuitive.

The same is true for MLFlowAgentRunner of course, and for AgentUtils, whereby in AgentUtils the former input is defined in the method parameter 'actionInput'. So the only change here would be executeParams.put("formerInput", actionInput); or executeParams.put("actionInput", actionInput); respectively.

Thanks @reuschling for the detailed comment. Actually we are the first possibility you mentioned.

Flow Agent

We have a dedicated parameter parameter for the previous tool output here https://github.com/opensearch-project/ml-commons/blob/main/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/MLFlowAgentRunner.java#L112. So you can reference it in the current tool input parameter as ${parameters.previous_tool_name.output}.

Conversational Agent

We take the action input for tools only from LLM response, i.e, the agent totally depends on LLM to do the ReAct. User can define a tool input to override the action input, but I don't see a use case for a placeholder of action input.

Good to hear that there is already a parameter for flow agent.
For Conversational agent, my example is exactly my use case. I want to predefine an Opensearch query, and the tool description says what's to insert. Thus, the previous llm step (the agent) doesn't have to generate the opensearch syntax, only a simple drop in, so I can guarantee this query will be executed, under all circumstances.
More specifically, I want to e. g. count documents with a query string inside.
Without a placeholder, the agent would have no possibility to adjust the query - only static queries independent from the previous steps in the thought chain would be possible.

@jngz-es any comment to my use case? This is my origin need and the reason behind #2918

Got your point. I created another issue to track it. #3134

Is there a chance left that it make it to 2.18?

@reuschling The backport PR has been merged into the 2.x branch. The 2.18 branch will include this PR.

I mean the conversational agent placeholder now tracked in #3134

Hi @reuschling , I raised a pr #3200, could you take a look?

ylwu-amzn · 2024-10-21T18:19:50Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/agent/AgentUtils.java

+            String input = toolSpecConfigMap.get("input");
+            StringSubstitutor substitutor = new StringSubstitutor(toolParams, "${parameters.", "}");
+            input = substitutor.replace(input);
+            toolParams.put("input", input);


Add these

if (isJson(actionInput)) { Map<String, String> params = getParameterMap(gson.fromJson(actionInput, Map.class)); toolParams.putAll(params); }

good catch, added

Signed-off-by: Jing Zhang <[email protected]>

* add config field in MLToolSpec for static parameters Signed-off-by: Jing Zhang <[email protected]> * add version control Signed-off-by: Jing Zhang <[email protected]> * address comments I Signed-off-by: Jing Zhang <[email protected]> * address commits II Signed-off-by: Jing Zhang <[email protected]> * address comments III Signed-off-by: Jing Zhang <[email protected]> --------- Signed-off-by: Jing Zhang <[email protected]> (cherry picked from commit 9ed0040)

* add config field in MLToolSpec for static parameters Signed-off-by: Jing Zhang <[email protected]> * add version control Signed-off-by: Jing Zhang <[email protected]> * address comments I Signed-off-by: Jing Zhang <[email protected]> * address commits II Signed-off-by: Jing Zhang <[email protected]> * address comments III Signed-off-by: Jing Zhang <[email protected]> --------- Signed-off-by: Jing Zhang <[email protected]> (cherry picked from commit 9ed0040) Co-authored-by: Jing Zhang <[email protected]>

add config field in MLToolSpec for static parameters

1bdc0ac

Signed-off-by: Jing Zhang <[email protected]>

jngz-es requested review from b4sjoo, dhrubo-os, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27, sam-herman and xinyual as code owners September 23, 2024 18:09

jngz-es temporarily deployed to ml-commons-cicd-env September 23, 2024 18:09 — with GitHub Actions Inactive

jngz-es temporarily deployed to ml-commons-cicd-env September 23, 2024 18:10 — with GitHub Actions Inactive

jngz-es temporarily deployed to ml-commons-cicd-env September 23, 2024 19:08 — with GitHub Actions Inactive

dbwiddis reviewed Sep 23, 2024

View reviewed changes

ylwu-amzn reviewed Sep 23, 2024

View reviewed changes

common/src/main/java/org/opensearch/ml/common/agent/MLToolSpec.java Outdated Show resolved Hide resolved

jngz-es mentioned this pull request Sep 23, 2024

[FEATURE] Ml-commons plugin introducing a new optional field "config" in MLTooSpec opensearch-project/flow-framework#878

Closed

add version control

1eb2fd2

Signed-off-by: Jing Zhang <[email protected]>

jngz-es temporarily deployed to ml-commons-cicd-env September 23, 2024 20:38 — with GitHub Actions Inactive

jngz-es had a problem deploying to ml-commons-cicd-env September 23, 2024 20:38 — with GitHub Actions Failure

jngz-es requested a review from ylwu-amzn September 23, 2024 20:39

jngz-es had a problem deploying to ml-commons-cicd-env September 23, 2024 21:38 — with GitHub Actions Failure

dbwiddis reviewed Sep 23, 2024

View reviewed changes

address comments I

9176b93

Signed-off-by: Jing Zhang <[email protected]>

jngz-es temporarily deployed to ml-commons-cicd-env September 23, 2024 23:31 — with GitHub Actions Inactive

jngz-es had a problem deploying to ml-commons-cicd-env September 23, 2024 23:34 — with GitHub Actions Failure

jngz-es temporarily deployed to ml-commons-cicd-env October 16, 2024 20:53 — with GitHub Actions Inactive

Zhangxunmt reviewed Oct 16, 2024

View reviewed changes

Zhangxunmt previously approved these changes Oct 16, 2024

View reviewed changes

dhrubo-os reviewed Oct 16, 2024

View reviewed changes

dhrubo-os reviewed Oct 17, 2024

View reviewed changes

reuschling suggested changes Oct 18, 2024

View reviewed changes

ylwu-amzn reviewed Oct 21, 2024

View reviewed changes

address comments III

8bca218

Signed-off-by: Jing Zhang <[email protected]>

jngz-es dismissed Zhangxunmt’s stale review via 8bca218 October 21, 2024 18:54

jngz-es temporarily deployed to ml-commons-cicd-env October 21, 2024 18:54 — with GitHub Actions Inactive

ylwu-amzn approved these changes Oct 21, 2024

View reviewed changes

jngz-es requested review from Zhangxunmt, reuschling, dhrubo-os and ylwu-amzn October 21, 2024 19:49

jngz-es temporarily deployed to ml-commons-cicd-env October 21, 2024 19:53 — with GitHub Actions Inactive

ylwu-amzn approved these changes Oct 21, 2024

View reviewed changes

Zhangxunmt approved these changes Oct 21, 2024

View reviewed changes

ylwu-amzn added the backport 2.x label Oct 21, 2024

jngz-es merged commit 9ed0040 into opensearch-project:main Oct 21, 2024
8 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 21, 2024

[Backport 2.x] add config field in MLToolSpec for static parameters #3133

Merged

reuschling mentioned this pull request Oct 22, 2024

[Enhancement] Conversational agent supports replacing the placeholder in tool input parameter with action input from llm #3134

Open

reuschling mentioned this pull request Nov 18, 2024

add action input as parameters for tool execution in conversational agent #3200

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add config field in MLToolSpec for static parameters #2977

add config field in MLToolSpec for static parameters #2977

jngz-es commented Sep 23, 2024 •

edited

Loading

dbwiddis left a comment •

edited

Loading

jngz-es commented Sep 23, 2024

yuye-aws commented Sep 24, 2024

yuye-aws commented Sep 24, 2024

Zhangxunmt Oct 16, 2024

dhrubo-os Oct 16, 2024

dbwiddis Oct 18, 2024

dhrubo-os Oct 16, 2024

dbwiddis Oct 18, 2024

dhrubo-os Oct 16, 2024

dhrubo-os Oct 17, 2024

dhrubo-os Oct 17, 2024

yuye-aws commented Oct 18, 2024

dbwiddis commented Oct 18, 2024 •

edited

Loading

reuschling Oct 18, 2024

jngz-es Oct 21, 2024

reuschling Oct 21, 2024

reuschling Oct 21, 2024

jngz-es Oct 21, 2024

reuschling Oct 21, 2024

yuye-aws Oct 22, 2024

reuschling Oct 22, 2024

jngz-es Nov 13, 2024

ylwu-amzn Oct 21, 2024

jngz-es Oct 21, 2024

add config field in MLToolSpec for static parameters #2977

add config field in MLToolSpec for static parameters #2977

Conversation

jngz-es commented Sep 23, 2024 • edited Loading

Description

Related Issues

Check List

dbwiddis left a comment • edited Loading

Choose a reason for hiding this comment

jngz-es commented Sep 23, 2024

yuye-aws commented Sep 24, 2024

yuye-aws commented Sep 24, 2024

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

yuye-aws commented Oct 18, 2024

dbwiddis commented Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flow Agent

Conversational Agent

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

jngz-es commented Sep 23, 2024 •

edited

Loading

dbwiddis left a comment •

edited

Loading

dbwiddis commented Oct 18, 2024 •

edited

Loading