Skip to content

Conversation

@rithin-pullela-aws
Copy link
Contributor

@rithin-pullela-aws rithin-pullela-aws commented Nov 28, 2025

Description

Needs to be merged after #4458

#4458 exposes the agent model ID in params which can be used by tools.

Query Planning Tool has been refactored to use the agent's model ID during agent creation if not explicitly specified during model registration.

Hence the older logic of adding the model ID during QPT registration is not required anymore.

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.

Summary by CodeRabbit

  • Refactor

    • Simplified agent creation by removing automatic tool augmentation logic.
    • Streamlined internal tool processing workflow.
  • Tests

    • Removed test cases related to tool model ID handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 28, 2025

Walkthrough

This PR removes automatic augmentation of QueryPlanningTool with a model_id from the agent registration process. The changes eliminate the processQueryPlannerTools method, related tool modification logic, and corresponding test cases while preserving public API signatures.

Changes

Cohort / File(s) Summary
Production logic removal
plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java
Removed processQueryPlannerTools method and associated tool augmentation logic. Eliminated imports (MLToolSpec, QueryPlanningTool, stream/collection handlers). MLAgent builder no longer receives updatedTools modifications.
Test coverage cleanup
plugin/src/test/java/org/opensearch/ml/action/agents/RegisterAgentTransportActionTests.java
Removed unit tests validating model_id injection/preservation for QueryPlanningTool in Plan/Query scenarios. Eliminated related imports (QueryPlanningTool, MLToolSpec, IndexRequest, List).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that removal of automatic tool augmentation does not break existing agent creation workflows
  • Confirm that no other components depend on the removed processQueryPlannerTools method
  • Ensure test removal aligns with the deliberate removal of tool modification behavior

Possibly related PRs

  • ml-commons#4458: Implements the QueryPlanningTool model_id injection and fallback logic that this PR removes—directly related through inverse modifications of the same behavior.

Suggested labels

v3.4.0

Suggested reviewers

  • austintlee
  • HenryL27
  • Zhangxunmt
  • jngz-es
  • ylwu-amzn
  • model-collapse
  • xinyual
  • sam-herman
  • dhrubo-os
  • rbhavna
  • mingshl
  • zane-neo
  • b4sjoo
  • pyek-bot

Poem

🐰 A tool too automatic, we've learned,
Clutters the path where agents have turned,
With model_id gone from the plan,
Our agents are simpler, and faster they ran!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: removing the Query Planning Tool model ID autofill logic from agent registration.
Description check ✅ Passed The description includes context about the dependency (PR #4458), explains why the change is needed, and uses the template format with Description and Related Issues sections.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 743115e and 60060f3.

📒 Files selected for processing (2)
  • plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java (1 hunks)
  • plugin/src/test/java/org/opensearch/ml/action/agents/RegisterAgentTransportActionTests.java (0 hunks)
💤 Files with no reviewable changes (1)
  • plugin/src/test/java/org/opensearch/ml/action/agents/RegisterAgentTransportActionTests.java
🔇 Additional comments (1)
plugin/src/main/java/org/opensearch/ml/action/agents/TransportRegisterAgentAction.java (1)

105-124: Builder-based enrichment of MLAgent looks correct and side‑effect free

Using agent.toBuilder() to set createdTime, lastUpdateTime, and isHidden and assign the result to mlAgent keeps the original request object immutable and aligns with how conversationAgent is built below. Downstream logic already operates on mlAgent, so this change is consistent and does not introduce new correctness issues.


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

@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval November 28, 2025 20:29 — with GitHub Actions Error
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval November 28, 2025 20:29 — with GitHub Actions Error
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval November 28, 2025 20:29 — with GitHub Actions Failure
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval November 28, 2025 20:29 — with GitHub Actions Failure
@owaiskazi19
Copy link
Member

@rithin-pullela-aws #4458 is merged. We can get this one in too for 3.4

@dhrubo-os dhrubo-os requested a deployment to ml-commons-cicd-env-require-approval December 2, 2025 01:54 — with GitHub Actions Waiting
@dhrubo-os dhrubo-os requested a deployment to ml-commons-cicd-env-require-approval December 2, 2025 01:54 — with GitHub Actions Waiting
@dhrubo-os dhrubo-os requested a deployment to ml-commons-cicd-env-require-approval December 2, 2025 01:54 — with GitHub Actions Waiting
@dhrubo-os dhrubo-os requested a deployment to ml-commons-cicd-env-require-approval December 2, 2025 01:54 — with GitHub Actions Waiting
@rithin-pullela-aws rithin-pullela-aws marked this pull request as ready for review December 3, 2025 19:39
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval December 3, 2025 19:54 — with GitHub Actions Error
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval December 3, 2025 19:54 — with GitHub Actions Failure
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval December 3, 2025 19:56 — with GitHub Actions Error
@rithin-pullela-aws rithin-pullela-aws had a problem deploying to ml-commons-cicd-env-require-approval December 3, 2025 19:56 — with GitHub Actions Failure
@rithin-pullela-aws
Copy link
Contributor Author

Insufficient disk space issue for linux:

Exec output and error:
| Output for ./bin/opensearch-plugin:-> Installing file:/home/ci-runner/.gradle/caches/modules-2/files-2.1/org.opensearch.plugin/opensearch-job-scheduler/3.4.0.0-SNAPSHOT/e0c382d2e412ab2978b8d81b36601feedf62b775/opensearch-job-scheduler-3.4.0.0-SNAPSHOT.zip
| -> Downloading file:/home/ci-runner/.gradle/caches/modules-2/files-2.1/org.opensearch.plugin/opensearch-job-scheduler/3.4.0.0-SNAPSHOT/e0c382d2e412ab2978b8d81b36601feedf62b775/opensearch-job-scheduler-3.4.0.0-SNAPSHOT.zip

> Task :opensearch-ml-plugin:integTest FAILED
| -> Installed opensearch-job-scheduler with folder name opensearch-job-scheduler
| -> Installing file:/__w/ml-commons/ml-commons/plugin/build/distributions/opensearch-ml-3.4.0.0-SNAPSHOT.zip
| -> Downloading file:/__w/ml-commons/ml-commons/plugin/build/distributions/opensearch-ml-3.4.0.0-SNAPSHOT.zip
| -> Failed installing file:/__w/ml-commons/ml-commons/plugin/build/distributions/opensearch-ml-3.4.0.0-SNAPSHOT.zip
| -> Rolling back opensearch-job-scheduler
| -> Rolled back opensearch-job-scheduler
| -> Rolling back file:/__w/ml-commons/ml-commons/plugin/build/distributions/opensearch-ml-3.4.0.0-SNAPSHOT.zip
| -> Rolled back file:/__w/ml-commons/ml-commons/plugin/build/distributions/opensearch-ml-3.4.0.0-SNAPSHOT.zip
| Exception in thread "main" java.io.IOException: No space left on device
| 	at java.base/sun.nio.ch.UnixFileDispatcherImpl.write0(Native Method)
| 	at java.base/sun.nio.ch.UnixFileDispatcherImpl.write(UnixFileDispatcherImpl.java:69)
| 	at java.base/sun.nio.ch.IOUtil.writeFromNativeBuffer(IOUtil.java:137)
| 	at java.base/sun.nio.ch.IOUtil.write(IOUtil.java:102)
| 	at java.base/sun.nio.ch.IOUtil.write(IOUtil.java:72)
| 	at java.base/sun.nio.ch.FileChannelImpl.implWrite(FileChannelImpl.java:393)
| 	at java.base/sun.nio.ch.FileChannelImpl.write(FileChannelImpl.java:373)
| 	at java.base/sun.nio.ch.ChannelOutputStream.writeFully(ChannelOutputStream.java:68)
| 	at java.base/sun.nio.ch.ChannelOutputStream.write(ChannelOutputStream.java:105)
| 	at java.base/java.io.InputStream.transferTo(InputStream.java:796)
| 	at org.opensearch.tools.cli.plugin.InstallPluginCommand.unzip(InstallPluginCommand.java:736)
| 	at org.opensearch.tools.cli.plugin.InstallPluginCommand.execute(InstallPluginCommand.java:275)
| 	at org.opensearch.tools.cli.plugin.InstallPluginCommand.execute(InstallPluginCommand.java:251)
| 	at org.opensearch.common.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:110)
| 	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
| 	at org.opensearch.cli.MultiCommand.execute(MultiCommand.java:104)
| 	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)

»    ↓ errors and warnings from /__w/ml-commons/ml-commons/plugin/build/testclusters/integTest-0/logs/opensearch.stdout.log ↓

Issue with remote service, retry would fix this:

RestMLInferenceSearchResponseProcessorIT > testMLInferenceProcessorRemoteModelCustomPrompt FAILED
    org.opensearch.client.ResponseException: method [GET], host [http://127.0.0.1:54557/], URI [/daily_index/_search?search_pipeline=qa_pipeline], status line [HTTP/1.1 503 Service Unavailable]
    {"error":{"root_cause":[{"type":"status_exception","reason":"Error from remote service: {\"message\":\"Too many connections, please wait before trying again.\"}"}],"type":"status_exception","reason":"Error from remote service: {\"message\":\"Too many connections, please wait before trying again.\"}"},"status":503}
        at __randomizedtesting.SeedInfo.seed([A1197F163DA12958:FC0537DA2D8A8F6]:0)
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:501)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:384)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:390)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:359)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:199)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:172)
        at app//org.opensearch.ml.utils.TestHelper.makeRequest(TestHelper.java:161)
        at app//org.opensearch.ml.rest.MLCommonsRestTestCase.searchWithPipeline(MLCommonsRestTestCase.java:1018)
        at app//org.opensearch.ml.rest.RestMLInferenceSearchResponseProcessorIT.testMLInferenceProcessorRemoteModelCustomPrompt(RestMLInferenceSearchResponseProcessorIT.java:297)

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.

3 participants