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

fix predict issue #3643

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix predict issue #3643

wants to merge 2 commits into from

Conversation

jngz-es
Copy link
Collaborator

@jngz-es jngz-es commented Mar 13, 2025

Description

We got a failure for the test case testPredictWithReadOnlyMLAccess which is blocking 3.0-alpha release and caused by a recent pr #3597.

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.

@jngz-es
fix predict issue
4b9afec 
Signed-off-by: Jing Zhang <[email protected]>
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 00:25 — with GitHub Actions Failure
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 00:25 — with GitHub Actions Error
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 00:25 — with GitHub Actions Error
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 00:25 — with GitHub Actions Failure
@mingshl
Copy link
Collaborator

mingshl commented Mar 13, 2025

trying to verifying if it solves the issue #3640

I just run the integ test on a 3.0 alpha tarball, it's passing with this fix

 ./gradlew integTest --tests "org.opensearch.ml.rest.SecureMLRestIT.testTrainWithReadOnlyMLAccess"  -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername="opensearch" -Dhttps=true -Duser=admin -Dpassword=myStrongPassword123!
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 8.11.1
  OS Info               : Mac OS X 15.3.1 (x86_64)
  JDK Version           : 21 (Amazon Corretto JDK)
  JAVA_HOME             : /Users/mingshl/Library/Java/JavaVirtualMachines/corretto-21.0.4/Contents/Home
  Random Testing Seed   : 958D6313604AEDEB
  In FIPS 140 mode      : false
=======================================
> Task :opensearch-ml-plugin:integTest

SecureMLRestIT STANDARD_ERROR
    Mar 12, 2025 5:35:00 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.

SecureMLRestIT > testTrainWithReadOnlyMLAccess STANDARD_OUT
    [2025-03-13T00:35:01,367][INFO ][o.o.m.r.SecureMLRestIT   ] [testTrainWithReadOnlyMLAccess] before test
    [2025-03-13T00:35:01,599][INFO ][o.o.m.r.SecureMLRestIT   ] [testTrainWithReadOnlyMLAccess] initializing REST clients against [https://localhost:9200]
    [2025-03-13T00:35:20,195][INFO ][o.o.m.r.SecureMLRestIT   ] [testTrainWithReadOnlyMLAccess] after test
[Incubating] Problems report is available at: file:///Users/mingshl/Documents/GitHub/mlcommons-repo/ml-commons/build/reports/problems/problems-report.html

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.11.1/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD SUCCESSFUL in 2m 34s

@jngz-es
fix UTs
f87230e 
Signed-off-by: Jing Zhang <[email protected]>
@jngz-es jngz-es force-pushed the predict_permission branch from 6e8b850 to f87230e Compare March 13, 2025 01:23
@jngz-es jngz-es temporarily deployed to ml-commons-cicd-env March 13, 2025 01:24 — with GitHub Actions Inactive
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 01:24 — with GitHub Actions Error
@jngz-es jngz-es temporarily deployed to ml-commons-cicd-env March 13, 2025 01:24 — with GitHub Actions Inactive
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 01:24 — with GitHub Actions Failure
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 02:16 — with GitHub Actions Failure
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 02:16 — with GitHub Actions Error
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 03:36 — with GitHub Actions Failure
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 03:36 — with GitHub Actions Error
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 14:19 — with GitHub Actions Error
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 14:19 — with GitHub Actions Failure
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 15:28 — with GitHub Actions Failure
@jngz-es jngz-es had a problem deploying to ml-commons-cicd-env March 13, 2025 15:28 — with GitHub Actions Error
@Zhangxunmt
Copy link
Collaborator

With this fix, there is no need to change the SecurityIT anymore. But the securityIT was changed before this PR is merged?

akolarkunnu
akolarkunnu reviewed Mar 14, 2025
View reviewed changes
plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java
@@ -143,9 +149,10 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
MLPredictionTaskRequest getRequest(String modelId, String modelType, String userAlgorithm, RestRequest request) throws IOException {
String tenantId = getTenantID(mlFeatureEnabledSetting.isMultiTenancyEnabled(), request);
ActionType actionType = ActionType.from(getActionTypeFromRestRequest(request));
if (FunctionName.REMOTE.name().equals(modelType) && !mlFeatureEnabledSetting.isRemoteInferenceEnabled()) {
if (modelType != null && FunctionName.REMOTE.name().equals(modelType) && !mlFeatureEnabledSetting.isRemoteInferenceEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding null check for modelType here won't make any difference, since modelType is inside equals()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akolarkunnu akolarkunnu akolarkunnu left review comments

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

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@mingshl mingshl Awaiting requested review from mingshl mingshl 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

@ylwu-amzn ylwu-amzn Awaiting requested review from ylwu-amzn ylwu-amzn 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

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jngz-es @mingshl @Zhangxunmt @akolarkunnu