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

[Backport 2.x] Support default model id in neural_sparse query #637

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

zhichao-aws
Copy link
Member

@zhichao-aws zhichao-aws commented Mar 14, 2024

Description

manually backport #614

…-project#614)

* feature: implement default model id for neural sparse

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* feature: implement default model id for neural sparse

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add ut

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add ut it

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add changelog

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* nit

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* fix ingest pipeline in it

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc restart-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc restart-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc restart-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc restart-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* fix undeploy with retry

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc restart-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc restart-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* optimize it code structure

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add it for bwc rolling-upgrade

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* tidy

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* update index mapping in it

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* nit

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* move version check to build script

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* resolve modelId

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* nit

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* update init model id

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* modify versions check logic in bwc test

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* add comments

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* nit

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

* updates for comments

Signed-off-by: zhichao-aws <zhichaog@amazon.com>

---------

Signed-off-by: zhichao-aws <zhichaog@amazon.com>
(cherry picked from commit e41fba7)
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

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

Project coverage is 83.28%. Comparing base (78fa7e4) to head (4b47f38).

Files Patch % Lines
...h/neuralsearch/query/NeuralSparseQueryBuilder.java 86.66% 0 Missing and 2 partials ⚠️
...search/query/visitor/NeuralSearchQueryVisitor.java 81.81% 0 Missing and 2 partials ⚠️
...nsearch/neuralsearch/query/NeuralQueryBuilder.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x     #637      +/-   ##
============================================
+ Coverage     83.21%   83.28%   +0.07%     
- Complexity      667      674       +7     
============================================
  Files            52       52              
  Lines          2079     2088       +9     
  Branches        336      338       +2     
============================================
+ Hits           1730     1739       +9     
  Misses          196      196              
  Partials        153      153              

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

Signed-off-by: zhichao-aws <zhichaog@amazon.com>
@zhichao-aws
Copy link
Member Author

zhichao-aws commented Mar 14, 2024

The codecov/patch failed in this backport PR but succeed in #614 . I checked the codedev report, and find there is one extra change in 2.x: https://github.com/zhichao-aws/neural-search/blob/4b47f386c47222102983f8f03af36a8f9b3516fe/src/main/java/org/opensearch/neuralsearch/query/NeuralSparseQueryBuilder.java#L117

And this line was different in main and 2.x. This difference was introduced from #479 and #441 . Where there was an additional comment to change the != null to Objects.isNull. Now in this PR we make this line consistent in main and 2.x . But cause a difference in codecov result.

I don't think this codecov failure is a blocking issue for the PR. If we want to increase the codecov, I think it's better to add tests in main and do backport to 2.x. Otherwise there will be too much effort to sync between main and 2.x.

@vibrantvarun vibrantvarun changed the title [Manually Backport 2.x] [FEATURE] support default model id in neural_sparse query [Backport 2.x] [FEATURE] support default model id in neural_sparse query Mar 14, 2024
@vibrantvarun vibrantvarun changed the title [Backport 2.x] [FEATURE] support default model id in neural_sparse query [Backport 2.x] support default model id in neural_sparse query Mar 14, 2024
@vibrantvarun vibrantvarun changed the title [Backport 2.x] support default model id in neural_sparse query [Backport 2.x] Support default model id in neural_sparse query Mar 14, 2024
@vibrantvarun
Copy link
Member

@zhichao-aws is the PR ready to merge?

@zhichao-aws
Copy link
Member Author

@zhichao-aws is the PR ready to merge?

Yes,please merge it.

@zane-neo zane-neo merged commit 7e57f65 into opensearch-project:2.x Mar 15, 2024
66 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants