update connector API #1227

Zhangxunmt · 2023-08-21T22:53:20Z

Description

Update connector API to partially update a connector in the ml-connector index. Verified it works with updating existing fields, adding new fields and deleting fields.

Example:
POST /_plugins/_ml/connectors/_update/4s0vGooBTRq0xHburjWW
{
"credential": {
"openAI_key": "new key"
}
}

GET /_plugins/_ml/connectors/4s0vGooBTRq0xHburjWW
{
...
"credential": {
"openAI_key": "new key"
},
...
}

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

ylwu-amzn · 2023-08-22T00:57:30Z

Github CI failed

plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java

plugin/src/main/java/org/opensearch/ml/rest/RestMLUpdateConnectorAction.java

codecov · 2023-08-22T22:36:40Z

Codecov Report

Merging #1227 (e0c6276) into 2.x (8cdac91) will increase coverage by 0.17%.
The diff coverage is 98.71%.

@@             Coverage Diff              @@
##                2.x    #1227      +/-   ##
============================================
+ Coverage     79.23%   79.41%   +0.17%     
- Complexity     2246     2270      +24     
============================================
  Files           188      190       +2     
  Lines          9089     9167      +78     
  Branches        904      906       +2     
============================================
+ Hits           7202     7280      +78     
- Misses         1477     1478       +1     
+ Partials        410      409       -1

Flag	Coverage Δ
ml-commons	`79.41% <98.71%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...tion/connector/UpdateConnectorTransportAction.java	`98.30% <98.30%> (ø)`
...rg/opensearch/ml/plugin/MachineLearningPlugin.java	`96.21% <100.00%> (+0.02%)`	⬆️
...pensearch/ml/rest/RestMLUpdateConnectorAction.java	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

ylwu-amzn · 2023-09-12T22:33:04Z

plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java

+        SearchRequest searchRequest = new SearchRequest(ML_MODEL_INDEX);
+        SearchSourceBuilder sourceBuilder = new SearchSourceBuilder();
+        BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery();
+        boolQueryBuilder.must(QueryBuilders.matchQuery(MLModel.CONNECTOR_ID_FIELD, connectorId));


Should we also check model status ?

It's already included in this search. The line 100 makes sure that the search results matches the model_ids that are in the cache. The size of the model is a concern. As the cache size goes large, the performance may become an issue. But 1) this update API is not a frequent API like invoke, so should be fine for now, and 2) the auto deployment with TTL should resolve this concern in the future after we simplify the APIs.

Sorry for the confusion, I didn't clearly understand why line 100 is relevant here?

Why can't we just search in the model index: where CONNECTOR_ID_FIELD = connectorId and model status = deployed or partially deployed?

dhrubo-os · 2023-09-12T23:31:31Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLUpdateConnectorRequest.java

+
+    public static MLUpdateConnectorRequest parse(XContentParser parser, String connectorId) throws IOException {
+        Map<String, Object> dataAsMap = null;
+        dataAsMap = parser.map();


Let's add TODO in that case.

dhrubo-os · 2023-09-12T23:47:19Z

plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java

+        SearchRequest searchRequest = new SearchRequest(ML_MODEL_INDEX);
+        SearchSourceBuilder sourceBuilder = new SearchSourceBuilder();
+        BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery();
+        boolQueryBuilder.must(QueryBuilders.matchQuery(MLModel.CONNECTOR_ID_FIELD, connectorId));


Sorry for the confusion, I didn't clearly understand why line 100 is relevant here?

Why can't we just search in the model index: where CONNECTOR_ID_FIELD = connectorId and model status = deployed or partially deployed?

dhrubo-os · 2023-09-12T23:49:27Z

plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java

+            if (searchHits.length == 0) {
+                client.update(updateRequest, getUpdateResponseListener(connectorId, listener, context));
+            } else {
+                log.error(searchHits.length + " models are still using this connector, please undeploy the models first!");


I feel like best user experience will be, if we show the deployed model ids also to the customer. Otherwise with this message user again needs to query to find out the deployed models.

Customer might not have any idea which models are deployed now with this connector idea. If we show the model ids, that could be helpful for customer.

line 100 will use all the deployed and deploying models to search ML-Model index that match "connector_id=". The model id shouldn't be included in the message otherwise app sec reviewer will raise concerns about the customer info leakage in the logs.

not sure how a random model id can concern appsec which is not quite a customer info.
Anyway, I was mostly referring to the new MLValidationException( part where we can send these model ids.

The goal is to make this easy for customers. That shouldn't concern appsec at least.

all the deployed and deploying models

does this get a inclusive list of all the deployed and deploying models in all the nodes?

opensearch-trigger-bot · 2023-11-16T10:56:02Z

The backport to main failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-main main
# Navigate to the new working tree
cd .worktrees/backport-main
# Create a new branch
git switch --create backport/backport-1227-to-main
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0855241ed55523f88fd11f188a331a013b1c7128
# Push it to GitHub
git push --set-upstream origin backport/backport-1227-to-main
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-main

Then, create a pull request where the base branch is main and the compare/head branch is backport/backport-1227-to-main.

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 21, 2023 22:53 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 21, 2023 22:53 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 21, 2023 22:53 — with GitHub Actions Failure

dhrubo-os reviewed Aug 22, 2023

View reviewed changes

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:20 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:20 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:20 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:20 — with GitHub Actions Error

Zhangxunmt force-pushed the 2.x branch from 16c8ffc to ea25804 Compare August 22, 2023 17:22

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:22 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:22 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 17:22 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 22:01 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 22:01 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 22:01 — with GitHub Actions Failure

Zhangxunmt force-pushed the 2.x branch from 3a76a1e to b31d6d5 Compare August 22, 2023 22:17

Zhangxunmt temporarily deployed to ml-commons-cicd-env August 22, 2023 22:17 — with GitHub Actions Inactive

Zhangxunmt force-pushed the 2.x branch from b31d6d5 to 9421ac5 Compare August 22, 2023 22:49

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 22:49 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 22:49 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env August 22, 2023 22:49 — with GitHub Actions Failure

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 18:28 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 18:28 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 18:28 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 18:28 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 18:49 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 18:49 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 18:49 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 18:49 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 19:10 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 19:10 — with GitHub Actions Failure

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 19:10 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 19:10 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 20:00 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env September 12, 2023 20:00 — with GitHub Actions Error

Zhangxunmt temporarily deployed to ml-commons-cicd-env September 12, 2023 20:23 — with GitHub Actions Inactive

b4sjoo self-requested a review September 12, 2023 20:24

b4sjoo approved these changes Sep 12, 2023

View reviewed changes

jngz-es approved these changes Sep 12, 2023

View reviewed changes

Zhangxunmt merged commit 0855241 into opensearch-project:2.x Sep 12, 2023

ylwu-amzn reviewed Sep 12, 2023

View reviewed changes

dhrubo-os reviewed Sep 12, 2023

View reviewed changes

Zhangxunmt mentioned this pull request Sep 15, 2023

[Enhancement] Add Update Connector API to Ml-commons #1147

Closed

rbhavna added the backport main label Nov 16, 2023

rbhavna mentioned this pull request Nov 16, 2023

[Backport to main] update connector API #1651

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update connector API #1227

update connector API #1227

Zhangxunmt commented Aug 21, 2023 •

edited

Loading

ylwu-amzn commented Aug 22, 2023

codecov bot commented Aug 22, 2023 •

edited

Loading

ylwu-amzn Sep 12, 2023

Zhangxunmt Sep 12, 2023 •

edited

Loading

dhrubo-os Sep 12, 2023

dhrubo-os Sep 12, 2023

dhrubo-os Sep 12, 2023

dhrubo-os Sep 12, 2023

Zhangxunmt Sep 13, 2023

dhrubo-os Sep 13, 2023

dhrubo-os Sep 13, 2023

opensearch-trigger-bot bot commented Nov 16, 2023

update connector API #1227

update connector API #1227

Conversation

Zhangxunmt commented Aug 21, 2023 • edited Loading

Description

Issues Resolved

Check List

ylwu-amzn commented Aug 22, 2023

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Zhangxunmt Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Nov 16, 2023

Zhangxunmt commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

Zhangxunmt Sep 12, 2023 •

edited

Loading