Skip to content
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

Introduce Ml Inference Search Request Extension #3284

Merged

Conversation

mingshl
Copy link
Collaborator

@mingshl mingshl commented Dec 16, 2024

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
Copy link
Collaborator

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

@mingshl mingshl force-pushed the feature/ml-inference-search-extension branch from b2504c4 to d5f7605 Compare December 17, 2024 19:45
@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 17, 2024 19:45 — with GitHub Actions Failure
@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 17, 2024 19:45 — with GitHub Actions Failure
@mingshl
Copy link
Collaborator Author

mingshl commented Dec 17, 2024

thanks @dhrubo-os , rebased and resolved conflicts.

@mingshl mingshl changed the title Introduce Ml Inference Search Extension Introduce Ml Inference Search Request Extension Dec 17, 2024
@dhrubo-os
Copy link
Collaborator

Tests are failing

@mingshl
Copy link
Collaborator Author

mingshl commented Dec 30, 2024

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

@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 30, 2024 22:40 — with GitHub Actions Failure
@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 00:36 — with GitHub Actions Failure
@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 00:36 — with GitHub Actions Failure
@mingshl mingshl force-pushed the feature/ml-inference-search-extension branch from d5f7605 to cfa8a38 Compare December 31, 2024 06:29
@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 06:30 — with GitHub Actions Failure
@mingshl mingshl had a problem deploying to ml-commons-cicd-env-require-approval December 31, 2024 06:30 — with GitHub Actions Failure
@mingshl
Copy link
Collaborator Author

mingshl commented Dec 31, 2024

rebased and resolved conflicts.

@dhrubo-os
Copy link
Collaborator

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 <mingshl@amazon.com>
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
…uest body

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
@mingshl mingshl force-pushed the feature/ml-inference-search-extension branch from d23f14c to 0960694 Compare January 25, 2025 06:11
@mingshl
Copy link
Collaborator Author

mingshl commented Jan 25, 2025

Looks good overall, but can you add REST test cases with some non trivial examples?

I added two REST tests to verify in the new commit add REST test and javadoc and just rebased the changes from main branch

@mingshl mingshl had a problem deploying to ml-commons-cicd-env January 25, 2025 06:12 — with GitHub Actions Failure
@mingshl mingshl temporarily deployed to ml-commons-cicd-env January 25, 2025 06:12 — with GitHub Actions Inactive
@mingshl mingshl temporarily deployed to ml-commons-cicd-env January 25, 2025 07:26 — with GitHub Actions Inactive
@mingshl mingshl temporarily deployed to ml-commons-cicd-env January 25, 2025 08:26 — with GitHub Actions Inactive
Copy link

codecov bot commented Jan 25, 2025

Codecov Report

Attention: Patch coverage is 85.03401% with 22 lines in your changes missing coverage. Please review.

Project coverage is 80.38%. Comparing base (f28bb74) to head (bafd235).
Report is 175 commits behind head on main.

Files with missing lines Patch % Lines
...l/processor/MLInferenceSearchRequestProcessor.java 68.00% 5 Missing and 3 partials ⚠️
...ava/org/opensearch/ml/processor/ModelExecutor.java 58.82% 5 Missing and 2 partials ⚠️
...ml/searchext/MLInferenceRequestParametersUtil.java 75.00% 1 Missing and 3 partials ⚠️
...rg/opensearch/ml/plugin/MachineLearningPlugin.java 50.00% 2 Missing ⚠️
...va/org/opensearch/ml/common/utils/StringUtils.java 97.67% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3284      +/-   ##
============================================
- Coverage     81.31%   80.38%   -0.93%     
- Complexity     6094     6720     +626     
============================================
  Files           573      602      +29     
  Lines         25268    28972    +3704     
  Branches       2666     3213     +547     
============================================
+ Hits          20547    23290    +2743     
- Misses         3601     4307     +706     
- Partials       1120     1375     +255     
Flag Coverage Δ
ml-commons 80.38% <85.03%> (-0.93%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
@dhrubo-os
Copy link
Collaborator

I didn't get the time to review the PR thoroughly. But approving the PR not to block. @austintlee could you please review this PR as you know more of this context. Thanks.

@mingshl mingshl merged commit 3cbd09a into opensearch-project:main Jan 27, 2025
9 of 10 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3284-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3cbd09ab2c9924c9c81214f0d24c20b2f5b58a02
# Push it to GitHub
git push --set-upstream origin backport/backport-3284-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3284-to-2.x.

@mingshl
Copy link
Collaborator Author

mingshl commented Jan 27, 2025

backport conflict due to this PR also changes in StringUtils, but it merge in main not backport to 2.x yet #3342

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 27, 2025
* add ml inference search extension

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* pass search extension to pipeline context

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* allow write to requestContext when outputmap's key not present in request body

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* add REST test and javadoc

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* increase code coverage for StringUtils

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

---------

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
(cherry picked from commit 3cbd09a)
mingshl added a commit that referenced this pull request Jan 27, 2025
* add ml inference search extension

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* pass search extension to pipeline context

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* allow write to requestContext when outputmap's key not present in request body

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* add REST test and javadoc

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

* increase code coverage for StringUtils

Signed-off-by: Mingshi Liu <mingshl@amazon.com>

---------

Signed-off-by: Mingshi Liu <mingshl@amazon.com>
(cherry picked from commit 3cbd09a)

Co-authored-by: Mingshi Liu <mingshl@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants