Introduce Ml Inference Search Request Extension #3284

mingshl · 2024-12-16T23:29:14Z

Description

Introduce Ml Inference Search Request Extension, this is adding ml_inference search extension during search request/query phase,

For the search response side, I already introduced a ml_inference search response extension in earlier PR released in 2.18. Support ML Inference Search Processor Writing to Search Extension

Related Issues

#3286
#3054

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.

dhrubo-os · 2024-12-16T23:31:28Z

@mingshl let's take the update from main. File has conflict.

mingshl · 2024-12-17T19:49:08Z

thanks @dhrubo-os ， rebased and resolved conflicts.

dhrubo-os · 2024-12-30T22:37:02Z

Tests are failing

mingshl · 2024-12-30T22:40:18Z

Tests are failing

there is no changes related to ConversationalMemory, will try rerun the tests.

> Task :opensearch-ml-memory:test

ConversationalMemoryHandlerITTests > testDifferentUsers_DifferentConversations FAILED
    java.lang.NoClassDefFoundError at ConversationalMemoryHandlerITTests.java:389
        Caused by: java.lang.ClassNotFoundException at ConversationalMemoryHandlerITTests.java:389

Signed-off-by: Mingshi Liu <[email protected]>

mingshl · 2024-12-31T06:34:50Z

rebased and resolved conflicts.

dhrubo-os · 2024-12-31T15:08:56Z

D:\a\ml-commons\ml-commons\common\build\generated\sources\delombok\java\main\org\opensearch\ml\common\utils\StringUtils.java:379: error: unknown tag: implNote
     * @implNote This method uses JsonPath for JSON manipulation and StringUtils for path existence checks.
       ^

Signed-off-by: Mingshi Liu <[email protected]>

martin-gaievski · 2025-01-03T00:06:14Z

plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java

-                    Object modelOutputValue = getModelOutputValue(mlOutput, modelOutputFieldName, ignoreMissing, fullResponsePath);
-                    String jsonPathExpression = "$." + newQueryField;
-                    JsonPath.parse(incomeQueryObject).set(jsonPathExpression, modelOutputValue);
+                    String newQueryField = null;


this declaration can be moved inside the try/catch block

martin-gaievski · 2025-01-03T00:07:55Z

plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java

+                        requestContext.setAttribute(newQueryField, modelOutputValue);
+
+                    } catch (PathNotFoundException e) {
+                        throw new IllegalArgumentException("can not find path " + newQueryField + "in query string");


do you want to log the full exception message here?

martin-gaievski · 2025-01-03T00:09:19Z

plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java

+                    if (!queryField.startsWith("ext.") && !queryField.startsWith("$.ext.")) {
+                        Object pathData = jsonData.read(queryField);
+                        if (pathData == null) {
+                            throw new IllegalArgumentException();


please add more information so user can have more information on the cause and address the issue

martin-gaievski · 2025-01-03T00:17:47Z

plugin/src/main/java/org/opensearch/ml/processor/ModelExecutor.java

@@ -334,4 +341,26 @@ default List<String> writeNewDotPathForNestedObject(Object json, String dotPath)
    default String convertToDotPath(String path) {
        return path.replaceAll("\\[(\\d+)\\]", "$1\\.").replaceAll("\\['(.*?)']", "$1\\.").replaceAll("^\\$", "").replaceAll("\\.$", "");
    }
+
+    default void setRequestContextFromExt(SearchRequest request, PipelineProcessingContext requestContext) {


this class looks more like an abstract class rather then an interface, there are verbose default implementations for every method. Consider refactoring interface->abstract class sometime in the future

martin-gaievski · 2025-01-03T00:19:04Z

plugin/src/main/java/org/opensearch/ml/processor/ModelExecutor.java

+                mlParams
+                    .forEach(
+                        (key, value) -> requestContext
+                            .setAttribute(String.format("ext.%s.%s", MLInferenceRequestParametersExtBuilder.NAME, key), value)


you need to use locale with String.format, default should be good enough

martin-gaievski · 2025-01-03T00:26:03Z

plugin/src/main/java/org/opensearch/ml/searchext/MLInferenceRequestParametersExtBuilder.java

+import org.opensearch.core.xcontent.XContentParser;
+import org.opensearch.search.SearchExtBuilder;
+
+public class MLInferenceRequestParametersExtBuilder extends SearchExtBuilder {


Please add class level java doc that describes class responsibilities

martin-gaievski · 2025-01-03T00:27:12Z

plugin/src/main/java/org/opensearch/ml/searchext/MLInferenceRequestParametersExtBuilder.java

+
+public class MLInferenceRequestParametersExtBuilder extends SearchExtBuilder {
+    private static final Logger logger = LogManager.getLogger(MLInferenceRequestParametersExtBuilder.class);
+    public static final String NAME = ML_INFERENCE_FIELD;


you can use ML_INFERENCE_FIELD directly, NAME is not needed

martin-gaievski · 2025-01-03T00:28:49Z

plugin/src/main/java/org/opensearch/ml/searchext/MLInferenceRequestParametersUtil.java

+import org.opensearch.action.search.SearchRequest;
+import org.opensearch.search.SearchExtBuilder;
+
+public class MLInferenceRequestParametersUtil {


please add class level comments

martin-gaievski · 2025-01-03T00:33:06Z

plugin/src/main/java/org/opensearch/ml/searchext/MLInferenceRequestParametersUtil.java

+                .collect(Collectors.toList());
+
+            if (!extBuilders.isEmpty()) {
+                mLInferenceRequestParametersExtBuilder = (MLInferenceRequestParametersExtBuilder) extBuilders.get(0);


if scenario with more than one element is valid did we document current behavior of using only first element and ignoring everything else? Btw, is this guaranteed that ext elements in this list will be in the same order as in user requst?

martin-gaievski · 2025-01-03T00:34:53Z

plugin/src/main/java/org/opensearch/ml/searchext/MLInferenceRequestParametersUtil.java

+                .filter(extBuilder -> MLInferenceRequestParametersExtBuilder.NAME.equals(extBuilder.getWriteableName()))
+                .collect(Collectors.toList());
+
+            if (!extBuilders.isEmpty()) {


I suggest we check for extBuilders.isEmpty() condition first and fail fast by returning null. This way we can organize code on lines 32-36

mingshl requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners December 16, 2024 23:29

mingshl requested a deployment to ml-commons-cicd-env-require-approval December 16, 2024 23:29 — with GitHub Actions Waiting

mingshl force-pushed the feature/ml-inference-search-extension branch from b2504c4 to d5f7605 Compare December 17, 2024 19:45

mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 17, 2024 19:45 — with GitHub Actions Failure

mingshl changed the title ~~Introduce Ml Inference Search Extension~~ Introduce Ml Inference Search Request Extension Dec 17, 2024

mingshl mentioned this pull request Dec 17, 2024

[DOC] Introduce Ml Inference Search Request Extension opensearch-project/documentation-website#8968

Open

3 tasks

mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 30, 2024 22:40 — with GitHub Actions Failure

mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 00:36 — with GitHub Actions Failure

add ml inference search extension

cfa8a38

Signed-off-by: Mingshi Liu <[email protected]>

mingshl force-pushed the feature/ml-inference-search-extension branch from d5f7605 to cfa8a38 Compare December 31, 2024 06:29

mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 06:30 — with GitHub Actions Failure

mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 20:07 — with GitHub Actions Failure

mingshl force-pushed the feature/ml-inference-search-extension branch from 9c576ec to 3cf89dc Compare December 31, 2024 22:34

mingshl requested a deployment to ml-commons-cicd-env-require-approval December 31, 2024 22:34 — with GitHub Actions Waiting

pass search extension to pipeline context

037fd7f

Signed-off-by: Mingshi Liu <[email protected]>

mingshl force-pushed the feature/ml-inference-search-extension branch from 3cf89dc to 037fd7f Compare December 31, 2024 22:39

mingshl temporarily deployed to ml-commons-cicd-env-require-approval December 31, 2024 22:40 — with GitHub Actions Inactive

mingshl temporarily deployed to ml-commons-cicd-env-require-approval January 2, 2025 20:33 — with GitHub Actions Inactive

martin-gaievski reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Ml Inference Search Request Extension #3284

Introduce Ml Inference Search Request Extension #3284

mingshl commented Dec 16, 2024 •

edited

Loading

dhrubo-os commented Dec 16, 2024

mingshl commented Dec 17, 2024

dhrubo-os commented Dec 30, 2024

mingshl commented Dec 30, 2024

mingshl commented Dec 31, 2024

dhrubo-os commented Dec 31, 2024

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

martin-gaievski Jan 3, 2025

Introduce Ml Inference Search Request Extension #3284

Are you sure you want to change the base?

Introduce Ml Inference Search Request Extension #3284

Conversation

mingshl commented Dec 16, 2024 • edited Loading

Description

Related Issues

Check List

dhrubo-os commented Dec 16, 2024

mingshl commented Dec 17, 2024

dhrubo-os commented Dec 30, 2024

mingshl commented Dec 30, 2024

mingshl commented Dec 31, 2024

dhrubo-os commented Dec 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingshl commented Dec 16, 2024 •

edited

Loading