Fix MLModelTool returns null if the response of LLM is a pure json object #2655

qianheng-aws · 2024-07-16T11:03:42Z

Description

Fix MLModelTool returns null if the response of LLM is a pure json object. It will detect wether the LLM response is a pure json object or a response string, and parse in different way.

There is similar logic in AgentUtils.java for MLChatAgentRunner

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

Line 193 in b6618b2

if (dataAsMap.size() == 1 && dataAsMap.containsKey("response")) {

Issues Resolved

#2654

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.

…ject Signed-off-by: Heng Qian <[email protected]>

Signed-off-by: Heng Qian <[email protected]>

zane-neo · 2024-07-17T09:37:06Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/MLModelTool.java

-            return mlModelOutputs.get(0).getMlModelTensors().get(0).getDataAsMap().get(responseField);
+            Map<String, ?> dataAsMap = mlModelOutputs.get(0).getMlModelTensors().get(0).getDataAsMap();
+            // Return the response field if it exists, otherwise return the whole response a json string.
+            if (dataAsMap.containsKey(responseField)) {


Original code doesn't have check on dataAsMap which might cause NPE in edge case, it would be good to add a check here.

I see many places that access dataAsMap directly without any index checking, so I follow that rule. But I can add array index checking.

Signed-off-by: Heng Qian <[email protected]>

dhrubo-os · 2024-07-17T16:11:24Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/MLModelTool.java

@@ -54,6 +55,7 @@ public class MLModelTool implements Tool {
    private Parser inputParser;
    @Setter
    @Getter
+    @VisibleForTesting


Why do we need this?

Make UT be able to access the private field outputParser and test whether its results are as expected.

But in the test you already declared: Parser outputParser = new MLModelTool(client, "modelId", "response").getOutputParser(); How are you using outputParser from the MLModelTool class then?

…ject (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914)

b4sjoo · 2024-07-18T09:55:25Z

@qianheng-aws This PR will break 2.x compile if backport, you need to fix and raise a backport PR manually, please check

…ject (opensearch-project#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914)

qianheng-aws · 2024-07-18T13:15:57Z

@qianheng-aws This PR will break 2.x compile if backport, you need to fix and raise a backport PR manually, please check

Thanks for reminding. It's because I use a java21 API in this change while branch 2.x is built under java17. I've raised a new PR to backport it, #2675

…ject (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914)

…ject (#2655) (#2677) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914) Co-authored-by: qianheng <[email protected]>

…ject (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914)

dhrubo-os · 2024-07-18T19:44:48Z

https://github.com/opensearch-project/ml-commons/actions/runs/9997362810/job/27633752977?pr=2679

this workflow is failing. May be we should use method which is compatible with java 11 too. Could you please refactor this change? Thanks

qianheng-aws · 2024-07-19T02:47:58Z

https://github.com/opensearch-project/ml-commons/actions/runs/9997362810/job/27633752977?pr=2679

this workflow is failing. May be we should use method which is compatible with java 11 too. Could you please refactor this change? Thanks

Raised this PR #2682 to avoid diverge on main branch. It will be compatible with java11 as well.

… a pure json object (#2675) * Fix MLModelTool returns null if the response of LLM is a pure json object (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914) * remove java21 API for backporting to 2.x Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]>

… a pure json object (#2675) * Fix MLModelTool returns null if the response of LLM is a pure json object (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914) * remove java21 API for backporting to 2.x Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 0a6a2b0)

… a pure json object (#2675) (#2685) * Fix MLModelTool returns null if the response of LLM is a pure json object (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914) * remove java21 API for backporting to 2.x Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 0a6a2b0) Co-authored-by: qianheng <[email protected]>

… a pure json object (#2675) (#2683) * Fix MLModelTool returns null if the response of LLM is a pure json object (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914) * remove java21 API for backporting to 2.x Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 0a6a2b0) Co-authored-by: qianheng <[email protected]>

… a pure json object (#2675) (#2684) * Fix MLModelTool returns null if the response of LLM is a pure json object (#2655) * Fix MLModelTool returns null if the response of LLM is a pure json object Signed-off-by: Heng Qian <[email protected]> * Fix UT failure Signed-off-by: Heng Qian <[email protected]> * Avoid NPE Signed-off-by: Heng Qian <[email protected]> * spotlessApply Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 007b914) * remove java21 API for backporting to 2.x Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> (cherry picked from commit 0a6a2b0) Co-authored-by: qianheng <[email protected]>

qianheng-aws added 2 commits July 16, 2024 18:44

Fix MLModelTool returns null if the response of LLM is a pure json ob…

b3b0e87

…ject Signed-off-by: Heng Qian <[email protected]>

Fix UT failure

c28de3b

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27, samuel-oci and xinyual as code owners July 16, 2024 11:03

qianheng-aws had a problem deploying to ml-commons-cicd-env July 16, 2024 11:03 — with GitHub Actions Failure

qianheng-aws temporarily deployed to ml-commons-cicd-env July 16, 2024 11:04 — with GitHub Actions Inactive

xinyual previously approved these changes Jul 16, 2024

View reviewed changes

qianheng-aws temporarily deployed to ml-commons-cicd-env July 16, 2024 12:02 — with GitHub Actions Inactive

zane-neo reviewed Jul 17, 2024

View reviewed changes

Avoid NPE

39d1bcf

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws dismissed xinyual’s stale review via 39d1bcf July 17, 2024 09:49

qianheng-aws had a problem deploying to ml-commons-cicd-env July 17, 2024 09:49 — with GitHub Actions Failure

qianheng-aws had a problem deploying to ml-commons-cicd-env July 17, 2024 09:50 — with GitHub Actions Failure

spotlessApply

76a2af9

Signed-off-by: Heng Qian <[email protected]>

qianheng-aws temporarily deployed to ml-commons-cicd-env July 17, 2024 09:51 — with GitHub Actions Inactive

qianheng-aws had a problem deploying to ml-commons-cicd-env July 17, 2024 09:52 — with GitHub Actions Failure

zane-neo approved these changes Jul 17, 2024

View reviewed changes

qianheng-aws requested a review from xinyual July 17, 2024 09:59

qianheng-aws temporarily deployed to ml-commons-cicd-env July 17, 2024 11:41 — with GitHub Actions Inactive

dhrubo-os reviewed Jul 17, 2024

View reviewed changes

xinyual approved these changes Jul 18, 2024

View reviewed changes

xinyual merged commit 007b914 into opensearch-project:main Jul 18, 2024
7 checks passed

b4sjoo added the backport 2.x label Jul 18, 2024

opensearch-trigger-bot bot mentioned this pull request Jul 18, 2024

[Backport 2.x] Fix MLModelTool returns null if the response of LLM is a pure json object #2674

Closed

qianheng-aws mentioned this pull request Jul 18, 2024

[Backport 2.x] Fix MLModelTool returns null if the response of LLM is a pure json object #2675

Merged

5 tasks

dhrubo-os added the backport feature/multi_tenancy label Jul 18, 2024

opensearch-trigger-bot bot mentioned this pull request Jul 18, 2024

[Backport feature/multi_tenancy] Fix MLModelTool returns null if the response of LLM is a pure json object #2677

Merged

dhrubo-os added the backport 2.13 label Jul 18, 2024

opensearch-trigger-bot bot mentioned this pull request Jul 18, 2024

[Backport 2.13] Fix MLModelTool returns null if the response of LLM is a pure json object #2679

Closed

dhrubo-os added the backport 2.15 label Jul 18, 2024

opensearch-trigger-bot bot mentioned this pull request Jul 18, 2024

[Backport 2.15] Fix MLModelTool returns null if the response of LLM is a pure json object #2680

Closed

qianheng-aws mentioned this pull request Jul 19, 2024

remove java21 API for aligning with BackPort PR #2682

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MLModelTool returns null if the response of LLM is a pure json object #2655

Fix MLModelTool returns null if the response of LLM is a pure json object #2655

qianheng-aws commented Jul 16, 2024

zane-neo Jul 17, 2024

qianheng-aws Jul 17, 2024

dhrubo-os Jul 17, 2024

qianheng-aws Jul 18, 2024

dhrubo-os Jul 18, 2024

b4sjoo commented Jul 18, 2024

qianheng-aws commented Jul 18, 2024

dhrubo-os commented Jul 18, 2024

qianheng-aws commented Jul 19, 2024

Fix MLModelTool returns null if the response of LLM is a pure json object #2655

Fix MLModelTool returns null if the response of LLM is a pure json object #2655

Conversation

qianheng-aws commented Jul 16, 2024

Description

Issues Resolved

Check List

zane-neo Jul 17, 2024

Choose a reason for hiding this comment

qianheng-aws Jul 17, 2024

Choose a reason for hiding this comment

dhrubo-os Jul 17, 2024

Choose a reason for hiding this comment

qianheng-aws Jul 18, 2024

Choose a reason for hiding this comment

dhrubo-os Jul 18, 2024

Choose a reason for hiding this comment

b4sjoo commented Jul 18, 2024

qianheng-aws commented Jul 18, 2024

dhrubo-os commented Jul 18, 2024

qianheng-aws commented Jul 19, 2024