Skip to content

Fix failing IT - #4657

Closed
mingshl wants to merge 1 commit into
opensearch-project:mainfrom
mingshl:main-fix-ITs
Closed

Fix failing IT#4657
mingshl wants to merge 1 commit into
opensearch-project:mainfrom
mingshl:main-fix-ITs

Conversation

@mingshl

@mingshl mingshl commented Feb 21, 2026

Copy link
Copy Markdown
Collaborator

Description

fix faling test testMLInferenceProcessorRemoteModelRewriteQueryType

I crossed verify in ml playground, we can still run range query on keyword field. and it's comparing in lexical order as expected. I am 100% sure it's not related to range query running on a keyword field. But I change the range query to search on an integer field to roll out this assumption that it might impact the test,.

There might be recently changes to local model. because I was using post-processing function that would convert to a local model output format, and it throw exception in the range query. After I removed it, the tests passed. I will fix the IT for now but we might need more investigation on why the post processing function with a local model format failed recently.

./gradlew ':opensearch-ml-plugin:integTest' --tests 'org.opensearch.ml.rest.RestMLInferenceSearchRequestProcessorIT.testMLInferenceProcessorRemoteModelRewriteQueryType' -Dtests.seed=6C875EAF50A5877 -Dtests.security.manager=false -Dtests.locale=fr-MF -Dtests.timezone=IST -Druntime.java=21

Related Issues

Resolves #4650

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.

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

coderabbitai Bot commented Feb 21, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8bb78f6 and 1cfd272.

📒 Files selected for processing (1)
  • plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:02:36.322Z
Learnt from: akolarkunnu
Repo: opensearch-project/ml-commons PR: 3919
File: plugin/src/test/java/org/opensearch/ml/cluster/MLSyncUpCronTests.java:184-191
Timestamp: 2025-12-18T16:02:36.322Z
Learning: In tests that exercise initialization logic (e.g., initMLConfig), verify idempotence by calling the init method twice and asserting that the master key is initialized once and cached for subsequent calls. The test should confirm that repeated initializations yield the same result and no unnecessary recomputation. Apply this pattern to similar test files under plugin/src/test/java/org/opensearch/ml/cluster and other modules that initialize shared state.

Applied to files:

  • plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build and Test MLCommons Plugin on Windows (25)
  • GitHub Check: Build and Test MLCommons Plugin on linux (25)
  • GitHub Check: Build and Test MLCommons Plugin on linux (21)
  • GitHub Check: Build and Test MLCommons Plugin on Windows (21)
🔇 Additional comments (2)
plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java (2)

72-107: LGTM - Simplified connector configuration.

Removing unused pre_process_function and post_process_function from the Bedrock embedding connector is appropriate. The Titan embedding model returns embeddings directly in a standard format, so custom processing functions are not required for this integration test.


344-344: Correct fix for the flaky range query.

Switching from diary_embedding_size (keyword type) to diary_embedding_size_int (integer type) properly resolves the parsing error. The query template injects a raw numeric value from embedding.length(), and integer fields handle numeric range comparisons (lte) correctly, whereas keyword fields expect string values and can fail in certain locales due to lexicographic comparison semantics.

This is consistent with testMLInferenceProcessorRemoteModelOptionalInputs at line 396, which already uses diary_embedding_size_int for its range query template.


📝 Walkthrough

Walkthrough

This change fixes a flaky test in RestMLInferenceSearchRequestProcessorIT by removing unused pre/post processing functions from a Bedrock embedding configuration and updating a range query template to reference an integer-typed embedding size field, addressing query parsing errors.

Changes

Cohort / File(s) Summary
Test Configuration Fix
plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java
Removed unused pre_process_function and post_process_function fields from Bedrock embedding connector config; updated query template to reference diary_embedding_size_int instead of diary_embedding_size to correct range query type mismatch.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Fix failing IT' is vague and generic, not clearly describing what the specific failing test fix entails. Provide a more descriptive title that specifies which test is being fixed, e.g., 'Fix testMLInferenceProcessorRemoteModelRewriteQueryType query validation error'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the issue being fixed and includes a related issue link, though it contains some grammatical issues and mentions potential investigation needed.
Linked Issues check ✅ Passed The PR fixes the failing test by removing pre/post-processing functions and switching to an integer field for range queries, directly addressing issue #4650's parsing_exception error.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the failing integration test identified in issue #4650 with no extraneous modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

@ylwu-amzn

Copy link
Copy Markdown
Collaborator

OpenSearch added stricter checking in this PR https://github.com/opensearch-project/OpenSearch/pull/20518/changes

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.

[Flaky Test] RestMLInferenceSearchRequestProcessorIT

2 participants