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.

Sign up for GitHub

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

Jump to bottom

Add interface for no postprocessing function case #3553

Merged
merged 6 commits into from
Feb 15, 2025
Merged

Add interface for no postprocessing function case #3553

merged 6 commits into from
Feb 15, 2025

Conversation

b4sjoo
Copy link
Collaborator

@b4sjoo b4sjoo commented Feb 14, 2025

Description

Add interface for no postprocessing function case

Related Issues

Resolves #3529

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.

@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 12:24 — with GitHub Actions Failure
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 12:24 — with GitHub Actions Failure
@b4sjoo
Correction after test
dfb375a 
Signed-off-by: Sicheng Song <[email protected]>
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 12:48 — with GitHub Actions Failure
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 12:48 — with GitHub Actions Failure
@dhrubo-os
Copy link
Collaborator

Take updates from main to pass the tests.

@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 19:39 — with GitHub Actions Failure
@b4sjoo b4sjoo temporarily deployed to ml-commons-cicd-env February 14, 2025 19:39 — with GitHub Actions Inactive
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 19:39 — with GitHub Actions Failure
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 23:47 — with GitHub Actions Failure
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 23:47 — with GitHub Actions Failure
@b4sjoo
Fix typo
8458298 
Signed-off-by: Sicheng Song <[email protected]>
@b4sjoo b4sjoo temporarily deployed to ml-commons-cicd-env February 14, 2025 23:50 — with GitHub Actions Inactive
@b4sjoo b4sjoo temporarily deployed to ml-commons-cicd-env February 14, 2025 23:50 — with GitHub Actions Inactive
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 14, 2025 23:50 — with GitHub Actions Failure
@b4sjoo b4sjoo temporarily deployed to ml-commons-cicd-env February 14, 2025 23:50 — with GitHub Actions Inactive
mingshl
mingshl reviewed Feb 15, 2025
View reviewed changes
common/src/main/java/org/opensearch/ml/common/utils/ModelInterfaceUtils.java
"Creating preset model interface for Amazon Bedrock model with post-process function: {}",
connector.getParameters().get("model")
);
return BEDROCK_TITAN_EMBED_TEXT_V1_MODEL_INTERFACE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking this default interface is matching the processing function from the blueprint. if users register the model with their custom processing functions, then the interface is not matching, and they should write their own model interface, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

@codecov Codecov
Copy link

codecov bot commented Feb 15, 2025

Codecov Report

Attention: Patch coverage is 91.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 80.32%. Comparing base (d7dec0f) to head (8458298).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
...pensearch/ml/common/utils/ModelInterfaceUtils.java 91.07% 0 Missing and 5 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #3553      +/-   ##
============================================
+ Coverage     80.25%   80.32%   +0.07%     
- Complexity     6906     6936      +30     
============================================
  Files           610      610              
  Lines         30077    30245     +168     
  Branches       3368     3380      +12     
============================================
+ Hits          24137    24295     +158     
- Misses         4487     4489       +2     
- Partials       1453     1461       +8
Flag Coverage Δ
ml-commons 80.32% <91.66%> (+0.07%) ⬆️

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.

@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 15, 2025 00:55 — with GitHub Actions Failure
@b4sjoo b4sjoo had a problem deploying to ml-commons-cicd-env February 15, 2025 00:55 — with GitHub Actions Failure
@mingshl
Copy link
Collaborator

mingshl commented Feb 15, 2025 •
edited
Loading

Thanks @b4sjoo for addressing the model interface logic, so for the left out Jurassic model, we should also track this,

when there is a predefine PostProcessingFunction for Jurassic model, similar to BedrockEmbeddingPostProcessFunction, we should add the check for the name of post processing function, then create the default interface.

I will create an issue to track this. cc @ylwu-amzn

@b4sjoo b4sjoo merged commit f9aef70 into opensearch-project:main Feb 15, 2025
5 of 9 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 15, 2025
@b4sjoo @github-actions
Add interface for no postprocessing function case (#3553)
e01a467 
* Add interface for no postprocessing function case

Signed-off-by: Sicheng Song <[email protected]>

* Correction after test

Signed-off-by: Sicheng Song <[email protected]>

* Make exception message clearer

Signed-off-by: Sicheng Song <[email protected]>

* Fix typo

Signed-off-by: Sicheng Song <[email protected]>

* Strictly the interface with correct post processing function

Signed-off-by: Sicheng Song <[email protected]>

---------

Signed-off-by: Sicheng Song <[email protected]>
(cherry picked from commit f9aef70)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Feb 15, 2025
Open
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 15, 2025
@b4sjoo @github-actions
Add interface for no postprocessing function case (#3553)
582948e 
* Add interface for no postprocessing function case

Signed-off-by: Sicheng Song <[email protected]>

* Correction after test

Signed-off-by: Sicheng Song <[email protected]>

* Make exception message clearer

Signed-off-by: Sicheng Song <[email protected]>

* Fix typo

Signed-off-by: Sicheng Song <[email protected]>

* Strictly the interface with correct post processing function

Signed-off-by: Sicheng Song <[email protected]>

---------

Signed-off-by: Sicheng Song <[email protected]>
(cherry picked from commit f9aef70)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Feb 15, 2025
Open
@mingshl mingshl mentioned this pull request Feb 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhrubo-os dhrubo-os dhrubo-os left review comments

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@mingshl mingshl mingshl approved these changes

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

@xinyual xinyual Awaiting requested review from xinyual xinyual is a code owner

Assignees
No one assigned
Labels
backport 2.x backport 2.19
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] Improve Model Interface Positioning and Auto-Application
4 participants
@b4sjoo @dhrubo-os @mingshl @ylwu-amzn