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

Fix test failure after lucene upgrade to 10 #3426

Conversation

zane-neo
Copy link
Collaborator

@zane-neo zane-neo commented Jan 23, 2025

Description

After this change: opensearch-project/OpenSearch#16366, the TotalHits's value is been moved to a private field, unable to access it directly outside that class, so removing this to fix the test failure.

We need to change the .value to .value() to make sure the code runs correctly.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

xinyual
xinyual previously approved these changes Jan 23, 2025
build.gradle Outdated
@@ -25,7 +25,7 @@ buildscript {
opensearch_build += "-SNAPSHOT"
}

common_utils_version = System.getProperty("common_utils.version", opensearch_build)
Copy link
Member

@peterzhuamazon peterzhuamazon Jan 23, 2025

Choose a reason for hiding this comment

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

This should stay on opensearch_build as it will be switched to 3.0.0.0-alpha1-SNAPSHOT as the version of common-utils.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterzhuamazon I didn't see the 3.0.0.0-alpha1-SNAPSHOT available on maven repo: https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/common-utils/, I can't compile success from my local.

Copy link
Member

Choose a reason for hiding this comment

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

it's still failing with opensearch_build for neural as well

Copy link
Member

@peterzhuamazon peterzhuamazon Jan 24, 2025

Choose a reason for hiding this comment

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

That is because Common Utils owner needs to update their plugins 1st to be compatible with lucene10.

Copy link
Member

Choose a reason for hiding this comment

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

@zane-neo @martin-gaievski I have updated common-utils main to 3.0.0.0-alpha1-SNAPSHOT and published to sonatype:

Thanks.

@@ -11,8 +11,8 @@ buildscript {
ext {
opensearch_group = "org.opensearch"
isSnapshot = "true" == System.getProperty("build.snapshot", "true")
opensearch_version = System.getProperty("opensearch.version", "3.0.0-SNAPSHOT")
buildVersionQualifier = System.getProperty("build.version_qualifier", "")
opensearch_version = System.getProperty("opensearch.version", "3.0.0-alpha1-SNAPSHOT")
Copy link
Member

Choose a reason for hiding this comment

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

This is correct.

@@ -56,7 +56,7 @@ dependencies {
implementation group: 'org.opensearch', name: 'opensearch', version: "${opensearch_version}"
implementation "org.opensearch.client:opensearch-rest-client:${opensearch_version}"
// Multi-tenant SDK Client
implementation "org.opensearch:opensearch-remote-metadata-sdk:${opensearch_build}"
implementation "org.opensearch:opensearch-remote-metadata-sdk:3.0.0-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

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

Remote sdk should also be 3.0.0.0-alpha1-SNAPSHOT.

@dbwiddis

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Remote sdk should also be 3.0.0.0-alpha1-SNAPSHOT.

We are still making changes on that for 2.19.0. I do not plan on updating to alpha until after at least code freeze date.

If anyone wants to get ahead of the game they are welcome to check out that repo and publish it locally with alpha1. All this churn as we're trying to finish up features within the week is causing a lot of wasted effort and delays.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@pyek-bot pyek-bot left a comment

Choose a reason for hiding this comment

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

changes for lucene upgrade (TotalHits) looks good

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Feb 11, 2025

Build is fixed, need to change all the rest client calls back to normal client, instead of transport.client for IT test to pass.

Think @rithin-pullela-aws will take a look on it per offline discussion.

Thanks.

Signed-off-by: zane-neo <[email protected]>
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env February 12, 2025 06:02 — with GitHub Actions Inactive
@zane-neo
Copy link
Collaborator Author

The failure test seems a flaky one, please review again

@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env February 12, 2025 17:41 — with GitHub Actions Inactive
@peterzhuamazon peterzhuamazon mentioned this pull request Feb 12, 2025
5 tasks
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 42.10526% with 11 lines in your changes missing coverage. Please review.

Project coverage is 80.26%. Comparing base (d7dec0f) to head (e14923c).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...pensearch/ml/common/model/LocalRegexGuardrail.java 0.00% 2 Missing ⚠️
...search/ml/action/tasks/GetTaskTransportAction.java 0.00% 2 Missing ⚠️
...nsearch/ml/autoredeploy/MLModelAutoReDeployer.java 0.00% 0 Missing and 2 partials ⚠️
...earch/ml/engine/indices/MLInputDatasetHandler.java 0.00% 1 Missing ⚠️
...action/undeploy/TransportUndeployModelsAction.java 0.00% 1 Missing ⚠️
...a/org/opensearch/ml/model/MLModelGroupManager.java 0.00% 0 Missing and 1 partial ⚠️
...ava/org/opensearch/ml/processor/ModelExecutor.java 75.00% 1 Missing ⚠️
.../main/java/org/opensearch/ml/utils/IndexUtils.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3426      +/-   ##
============================================
+ Coverage     80.25%   80.26%   +0.01%     
+ Complexity     6906     6904       -2     
============================================
  Files           610      610              
  Lines         30077    30077              
  Branches       3368     3368              
============================================
+ Hits          24137    24141       +4     
+ Misses         4487     4484       -3     
+ Partials       1453     1452       -1     
Flag Coverage Δ
ml-commons 80.26% <42.10%> (+0.01%) ⬆️

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.

@rithin-pullela-aws
Copy link
Contributor

@zane-neo please refer to this change to address the code coverage issue

Check warning on line 130 in plugin/src/main/java/org/opensearch/ml/utils/IndexUtils.java

@codecov
codecov
/ codecov/patch
plugin/src/main/java/org/opensearch/ml/utils/IndexUtils.java#L130

Added line #L130 was not covered by tests

@@ -228,17 +228,13 @@ default Object getModelOutputValue(MLOutput mlOutput, String modelOutputFieldNam
static Object parseDataInTensor(ModelTensor tensor) {
Object modelOutputValue;
if (tensor.getDataType().isInteger()) {
modelOutputValue = Arrays.stream(tensor.getData()).map(Number::intValue).map(Integer::new).collect(Collectors.toList());
modelOutputValue = Arrays.stream(tensor.getData()).map(Number::intValue).collect(Collectors.toList());
Copy link
Contributor

@rithin-pullela-aws rithin-pullela-aws Feb 12, 2025

Choose a reason for hiding this comment

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

Is removing .map(Integer::new) necessary for lucene 10 changes?
(Same for all the changes in this file)

Copy link
Collaborator Author

@zane-neo zane-neo Feb 13, 2025

Choose a reason for hiding this comment

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

JDK21 has removed the new Integer() method, and based on the code, these conversion are not necessary so it's safe to remove all of them.

@peterzhuamazon
Copy link
Member

@zane-neo please refer to this change to address the code coverage issue

Check warning on line 130 in plugin/src/main/java/org/opensearch/ml/utils/IndexUtils.java
@codecov
codecov
/ codecov/patch
plugin/src/main/java/org/opensearch/ml/utils/IndexUtils.java#L130
Added line #L130 was not covered by tests

Update @rithin-pullela-aws test coverage commit to this PR.

@peterzhuamazon
Copy link
Member

Seems like integTest is not correctly using 3.0.0-alpha1-SNAPSHOT


> Task :opensearch-ml-plugin:integTest
MLModelAutoReDeployerIT STANDARD_ERROR
    Feb 12, 2025 11:41:44 PM org.apache.lucene.internal.vectorization.VectorizationProvider lookup
    WARNING: Java vector incubator module is not readable. For optimal vector performance, pass '--add-modules jdk.incubator.vector' to enable Vector API.
MLModelAutoReDeployerIT > testModelAutoRedeploy STANDARD_OUT
    [2025-02-13T11:41:44,467][INFO ][o.o.m.a.MLModelAutoReDeployerIT] [testModelAutoRedeploy] before test
    [2025-02-13T11:41:44,588][INFO ][o.o.m.a.MLModelAutoReDeployerIT] [testModelAutoRedeploy] initializing REST clients against [http://[::1]:46321, http://127.0.0.1:43023, http://[::1]:40255, http://127.0.0.1:45239, http://[::1]:36245, http://127.0.0.1:40575]
MLModelAutoReDeployerIT > testModelAutoRedeploy STANDARD_ERROR
    feb. 13, 2025 11:42:00 DOPOŁDNJA org.opensearch.client.RestClient logResponse
    WARNING: request [DELETE http://[::1]:36245/.plugins-ml-model-group] returned 1 warnings: [299 OpenSearch-3.0.0-SNAPSHOT-d0a65d3b457e11d514171ecc0a77450fdd7d25bb "this request accesses system indices: [.plugins-ml-model-group], but in a future major version, direct access to system indices will be prevented by default"]
    feb. 13, 2025 11:42:00 DOPOŁDNJA org.opensearch.client.RestClient logResponse
    WARNING: request [DELETE http://127.0.0.1:45239/.plugins-ml-config] returned 1 warnings: [299 OpenSearch-3.0.0-SNAPSHOT-d0a65d3b457e11d514171ecc0a77450fdd7d25bb "this request accesses system indices: [.plugins-ml-config], but in a future major version, direct access to system indices will be prevented by default"]
    feb. 13, 2025 11:42:00 DOPOŁDNJA org.opensearch.client.RestClient logResponse
    WARNING: request [DELETE http://[::1]:40255/.plugins-ml-model] returned 1 warnings: [299 OpenSearch-3.0.0-SNAPSHOT-d0a65d3b457e11d514171ecc0a77450fdd7d25bb "this request accesses system indices: [.plugins-ml-model], but in a future major version, direct access to system indices will be prevented by default"]
    feb. 13, 2025 11:42:00 DOPOŁDNJA org.opensearch.client.RestClient logResponse
    WARNING: request [DELETE http://127.0.0.1:43023/.plugins-ml-task] returned 1 warnings: [299 OpenSearch-3.0.0-SNAPSHOT-d0a65d3b457e11d514171ecc0a77450fdd7d25bb "this request accesses system indices: [.plugins-ml-task], but in a future major version, direct access to system indices will be prevented by default"]

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Approve based on the windows test passing, as well as unblock other plugins that depend on ml plugin.

Thanks everyone chiming in and help!

@peterzhuamazon peterzhuamazon merged commit ebc0792 into opensearch-project:main Feb 13, 2025
7 of 8 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.