Skip to content

adding tenantId to the connector executor when this is inline connector #3837

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
Merged
merged 3 commits into from
May 7, 2025
Merged

adding tenantId to the connector executor when this is inline connector #3837

merged 3 commits into from
May 7, 2025
+59 −7

Conversation

dhrubo-os
Copy link
Collaborator

Description

[adding tenantId to the connector executor when this is inline connector]

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 temporarily deployed to ml-commons-cicd-env May 6, 2025 23:27 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 6, 2025 23:27 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 6, 2025 23:27 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 6, 2025 23:27 — with GitHub Actions Inactive
@codecov Codecov
Copy link

codecov bot commented May 7, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.98%. Comparing base (0cba0ed) to head (d2973ff).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #3837      +/-   ##
============================================
- Coverage     77.99%   77.98%   -0.02%     
- Complexity     7316     7318       +2     
============================================
  Files           655      655              
  Lines         33032    33037       +5     
  Branches       3706     3708       +2     
============================================
- Hits          25764    25763       -1     
- Misses         5681     5687       +6     
  Partials       1587     1587
Flag Coverage Δ
ml-commons 77.98% <100.00%> (-0.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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 00:29 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 00:29 — with GitHub Actions Inactive
jngz-es
jngz-es reviewed May 7, 2025
View reviewed changes
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteModel.java
@@ -103,6 +103,10 @@ public void initModel(MLModel model, Map<String, Object> params, Encryptor encry
Connector connector = model.getConnector().cloneConnector();
connector
.decrypt(PREDICT.name(), (credential, tenantId) -> encryptor.decrypt(credential, model.getTenantId()), model.getTenantId());
// This situation can only happen for inline connector where we don't provide tenant id.
if (connector.getTenantId() == null && model.getTenantId() != null) {
connector.setTenantId(model.getTenantId());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering why we don't set tenantId for the inline connector when registering the model which can make a consistent connector to use anywhere. Now the cloned connector and connectorExecutor has the tenantId but the original connector doesn't has it. If we want to use model.getConnector in code to reference, we have to check tenantId again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Model already has the tenantId information. So in the same entity to have the tenant id twice seems like redundant to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's pros and cons. I am OK to merge it for now, we can have more discussion later.

Zhangxunmt
Zhangxunmt reviewed May 7, 2025
View reviewed changes
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteModel.java
@@ -103,6 +103,10 @@ public void initModel(MLModel model, Map<String, Object> params, Encryptor encry
Connector connector = model.getConnector().cloneConnector();
connector
.decrypt(PREDICT.name(), (credential, tenantId) -> encryptor.decrypt(credential, model.getTenantId()), model.getTenantId());
// This situation can only happen for inline connector where we don't provide tenant id.
if (connector.getTenantId() == null && model.getTenantId() != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if model.getTenantId == null, this code won't do anything. But if that is an edge case to avoid in serverLess, you should check it and throw an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If both are null, that means this is a single tenant environment, so we don't need this anyway.

Zhangxunmt
Zhangxunmt previously approved these changes May 7, 2025
View reviewed changes
Copy link
Collaborator

@Zhangxunmt Zhangxunmt left a comment

Choose a reason for hiding this comment

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

Approved due to time constrain. From the Code coverage it shows 50.00000% with 1 line in your changes missing coverage.

@dhrubo-os
added more unit tests
f71d879 
Signed-off-by: Dhrubo Saha <[email protected]>
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 17:43 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 17:43 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 17:44 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 17:44 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 18:47 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 18:47 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env May 7, 2025 19:02 — with GitHub Actions Failure
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 19:02 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env May 7, 2025 19:02 — with GitHub Actions Error
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 19:02 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 20:05 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 20:05 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os merged commit 71e6365 into opensearch-project:main May 7, 2025
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zhangxunmt Zhangxunmt Zhangxunmt approved these changes

@jngz-es jngz-es jngz-es approved these changes

@arjunkumargiri arjunkumargiri arjunkumargiri approved these changes

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

@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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dhrubo-os @Zhangxunmt @jngz-es @arjunkumargiri