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

adding tenantID to the request + implement multi-tenancy in undeploy api #3425

Merged
merged 1 commit into from
Jan 24, 2025
Merged

adding tenantID to the request + implement multi-tenancy in undeploy api #3425

merged 1 commit into from
Jan 24, 2025

Conversation

dhrubo-os
Copy link
Collaborator

@dhrubo-os dhrubo-os commented Jan 23, 2025 •
edited
Loading

Description

[adding tenantID to the request + implement multi-tenancy in undeploy api]

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.

@dhrubo-os dhrubo-os changed the title adding tenantID to the request adding tenantID to the request + undeploy request Jan 23, 2025
@dhrubo-os dhrubo-os changed the title adding tenantID to the request + undeploy request adding tenantID to the request + implement multi-tenancy in undeploy api Jan 23, 2025
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env January 23, 2025 22:05 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env January 23, 2025 22:05 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env January 23, 2025 23:04 — with GitHub Actions Inactive
@codecov Codecov
Copy link

codecov bot commented Jan 23, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 52.27273% with 63 lines in your changes missing coverage. Please review.

Project coverage is 80.29%. Comparing base (f28bb74) to head (c697a5b).
Report is 173 commits behind head on main.

Files with missing lines Patch % Lines
...action/undeploy/TransportUndeployModelsAction.java 27.02% 25 Missing and 2 partials ⚠️
...tion/prediction/TransportPredictionTaskAction.java 51.11% 18 Missing and 4 partials ⚠️
...n/java/org/opensearch/ml/cluster/MLSyncUpCron.java 36.36% 1 Missing and 6 partials ⚠️
...on/transport/undeploy/MLUndeployModelsRequest.java 76.92% 3 Missing ⚠️
...ransport/undeploy/MLUndeployModelNodesRequest.java 60.00% 0 Missing and 2 partials ⚠️
...opensearch/ml/engine/indices/MLIndicesHandler.java 0.00% 0 Missing and 1 partial ⚠️
...earch/ml/engine/indices/MLInputDatasetHandler.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #3425      +/-   ##
============================================
- Coverage     81.31%   80.29%   -1.02%     
- Complexity     6094     6695     +601     
============================================
  Files           573      599      +26     
  Lines         25268    29137    +3869     
  Branches       2666     3224     +558     
============================================
+ Hits          20547    23397    +2850     
- Misses         3601     4340     +739     
- Partials       1120     1400     +280
Flag Coverage Δ
ml-commons 80.29% <52.27%> (-1.02%) ⬇️

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.

@dhrubo-os
adding tenantID to the request + undeploy request
c697a5b 
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env January 23, 2025 23:37 — with GitHub Actions Inactive
dbwiddis
dbwiddis approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM with one nit

ml-algorithms/src/main/java/org/opensearch/ml/engine/indices/MLInputDatasetHandler.java
@@ -68,7 +68,7 @@ public void parseSearchQueryInput(MLInputDataset mlInputDataset, ActionListener<
listener.onResponse(dfInputDataset);
return;
}, e -> {
log.error("Failed to search" + e);
log.error("Failed to search{}", e);
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing to substitute here?

@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env January 24, 2025 00:58 — with GitHub Actions Inactive
b4sjoo
b4sjoo reviewed Jan 24, 2025
View reviewed changes
plugin/src/main/java/org/opensearch/ml/cluster/MLSyncUpCron.java
);

client.execute(MLUndeployModelsAction.INSTANCE, mlUndeployModelsRequest, ActionListener.wrap(r -> {
MLUndeployModelNodesResponse mlUndeployModelNodesResponse = r.getResponse();
if (mlUndeployModelNodesResponse.failures() != null && mlUndeployModelNodesResponse.failures().size() != 0) {
if (mlUndeployModelNodesResponse.failures() != null && !mlUndeployModelNodesResponse.failures().isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for this change? To avoid NPE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

better for Readability

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 also nanoseconds faster to check empty vs. actually getting the size which requires iteration.

Here's a benchmark. The improvement is trivial but it's generally a good change. :)
https://salesforce.stackexchange.com/questions/68728/efficiency-of-list-size-0-vs-list-isempty

@dhrubo-os dhrubo-os merged commit af96fe0 into opensearch-project:main Jan 24, 2025
8 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 24, 2025
@dhrubo-os @github-actions
adding tenantID to the request + undeploy request (#3425)
2aa950f 
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
(cherry picked from commit af96fe0)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 24, 2025
Merged
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env January 24, 2025 01:57 — with GitHub Actions Inactive
dhrubo-os added a commit that referenced this pull request Jan 24, 2025
@opensearch-trigger-bot @dhrubo-os
adding tenantID to the request + undeploy request (#3425) (#3429)
76f0f3b 
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
(cherry picked from commit af96fe0)

Co-authored-by: Dhrubo Saha <dhrubo@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbwiddis dbwiddis dbwiddis approved these changes

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@b4sjoo b4sjoo b4sjoo approved these changes

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

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

@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

Assignees
No one assigned
Labels
backport 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dhrubo-os @dbwiddis @ylwu-amzn @b4sjoo