#289 - Add register, update and delete model group functionality to support Model Access Control #332

rawwar · 2023-11-03T20:23:19Z

Description

Add model group registry, update, search and deletion functionality

Issues Resolved

closes #289
closes #290
closes #292
closes #333
closes #117

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.

Signed-off-by: kalyan <[email protected]>

opensearch_py_ml/ml_commons/model_access_control.py

rawwar · 2023-11-03T20:25:31Z

@dhrubo-os , can you please review and let me know if you have any feedback. I am working on adding tests

opensearch_py_ml/ml_commons/ml_commons_client.py

opensearch_py_ml/ml_commons/model_access_control.py

dhrubo-os · 2023-11-03T20:42:15Z

Just one suggestion, you can also mark a PR as draft if you think PR is not ready to review yet.

Signed-off-by: kalyan <[email protected]>

codecov · 2023-11-04T09:25:21Z

Codecov Report

Merging #332 (075a763) into main (fc698a7) will increase coverage by 0.11%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
+ Coverage   91.36%   91.47%   +0.11%     
==========================================
  Files          38       40       +2     
  Lines        4237     4339     +102     
==========================================
+ Hits         3871     3969      +98     
- Misses        366      370       +4

Files	Coverage Δ
opensearch_py_ml/ml_commons/ml_commons_client.py	`84.84% <100.00%> (+0.18%)`	⬆️
...y_ml/ml_commons/validators/model_access_control.py	`98.00% <98.00%> (ø)`
...pensearch_py_ml/ml_commons/model_access_control.py	`94.00% <94.00%> (ø)`

…ch-py-ml into kalyan/289-model-access-control-

Signed-off-by: kalyan <[email protected]>

tests/ml_commons/test_model_access_control.py

.github/workflows/integration.yml

Signed-off-by: kalyan <[email protected]>

rawwar · 2023-11-06T02:23:05Z

@dhrubo-os , I fixed the issues. Can you review the PR and let me know if I need to change anything? I will work on to increase the coverage.

.ci/opensearch/Dockerfile

opensearch_py_ml/ml_commons/model_access_control.py

opensearch_py_ml/ml_commons/validators/model_access_control.py

.ci/opensearch/Dockerfile

…ch-py-ml into kalyan/289-model-access-control-

Signed-off-by: kalyan <[email protected]>

opensearch_py_ml/ml_commons/model_access_control.py

opensearch_py_ml/ml_commons/validators/model_access_control.py

tests/common.py

tests/ml_commons/test_model_access_control.py

dhrubo-os · 2023-11-06T18:09:36Z

@rbhavna Could you please review this PR as you implemented the model access control. Thanks.

Signed-off-by: kalyan <[email protected]>

.ci/opensearch/Dockerfile

.github/workflows/integration.yml

opensearch_py_ml/ml_commons/model_access_control.py

tests/ml_commons/test_model_access_control.py

dhrubo-os · 2023-11-13T01:32:02Z

Overall Looks good to me

@rbhavna could you also please review this PR as you know more about model access control?

Signed-off-by: kalyanr <[email protected]>

rbhavna · 2023-11-13T17:56:40Z

opensearch_py_ml/ml_commons/validators/model_access_control.py

+        raise ValueError("operation needs to be a string")
+
+
+def validate_create_model_group_parameters(


Any specific reason we are not validating all the exception cases while creating model group? Like the ones we have here: https://github.com/opensearch-project/ml-commons/blob/6efb1ebeefb8a7930f6767262457d0d5b568ac75/plugin/src/main/java/org/opensearch/ml/model/MLModelGroupManager.java#L148

@rbhavna , since, these validations are already being done by ml-commons, having them here as well felt like double validations. Most checks I wrote here just check for type issues and helps to make a valid API call.

Just curious, the three exception cases that we are testing here are also being validated by ml-commons. Any reason we still have them here and not the others?

Right. I do feel we shouldn't be having these checks in opensearch-py-ml. Because, if checks on ml-commons change, we again need to update this on the client as well. I was thinking, client libraries only purpose would be to make the calls and if any errors from the opensearch, it should be able to pass them to user without any change. Also, maybe add few utility functions to make things easier for the user.

@dhrubo-os , @rbhavna , what do you think about removing these validations form opensearch-py-ml? Or Should I go ahead with implementing the validations from ml-commons ?

As opensearch-py-ml is a client plugin for ml-commons, we should have only the client side testing similar like any web application. Some basic input validations which can be easily caught before the request reaching to the server. We don't need to add that type of validations for which we need to make a back-end call like testing if the user is admin or not.

Yeah we need to have the same basic validations in ml-commons too, as py-ml isn't the only client for ml-commons.

rbhavna · 2023-11-13T17:58:39Z

opensearch_py_ml/ml_commons/validators/model_access_control.py

+            )
+
+
+def validate_update_model_group_parameters(update_query: dict, model_group_id: str):


Same doubt. Update model group also validates similar exception cases that are missing here. If we are not validating them at any other place, I would suggest we have them here.

I agree, but I think the limitation is, during update we don't know what are the values already for model group in index. So either we need to get the model group with a server call and then check or we call leave it to the server validation.

In this case I would prefer the second not to complicate things.

Yes agreed @dhrubo-os

Signed-off-by: kalyanr <[email protected]>

init

a2ffe54

Signed-off-by: kalyan <[email protected]>

rawwar requested review from dhrubo-os, greaa-aws, ylwu-amzn, b4sjoo, jngz-es and rbhavna as code owners November 3, 2023 20:23

rawwar commented Nov 3, 2023

View reviewed changes

opensearch_py_ml/ml_commons/model_access_control.py Outdated Show resolved Hide resolved

rawwar changed the title ~~#289 - Add register model group functionality to support Model Access Control~~ #289 - Add register, update and delete model group functionality to support Model Access Control Nov 3, 2023

dhrubo-os reviewed Nov 3, 2023

View reviewed changes

opensearch_py_ml/ml_commons/ml_commons_client.py Show resolved Hide resolved

opensearch_py_ml/ml_commons/model_access_control.py Outdated Show resolved Hide resolved

rawwar marked this pull request as draft November 3, 2023 20:54

add search, delete and update

7bef831

Signed-off-by: kalyan <[email protected]>

rawwar added 12 commits November 4, 2023 15:43

Merge branch 'main' of https://github.com/opensearch-project/opensear…

61c3090

…ch-py-ml into kalyan/289-model-access-control-

add tests for register model group

a89bb39

Signed-off-by: kalyan <[email protected]>

update cluster to 2.11

3bbea93

Signed-off-by: kalyan <[email protected]>

test skipif

ca765ec

Signed-off-by: kalyan <[email protected]>

fix

4c6a7fd

Signed-off-by: kalyan <[email protected]>

add tests

af60958

Signed-off-by: kalyan <[email protected]>

update matrix

cde4e1f

Signed-off-by: kalyan <[email protected]>

cancel in progress

28040ee

Signed-off-by: kalyan <[email protected]>

update concurrency

d8fc9b2

Signed-off-by: kalyan <[email protected]>

job level concurrency

cda4c08

Signed-off-by: kalyan <[email protected]>

fix

1ebeb1a

Signed-off-by: kalyan <[email protected]>

fix

1b72089

Signed-off-by: kalyan <[email protected]>

dhrubo-os reviewed Nov 4, 2023

View reviewed changes

tests/ml_commons/test_model_access_control.py Show resolved Hide resolved

dhrubo-os reviewed Nov 4, 2023

View reviewed changes

.github/workflows/integration.yml Show resolved Hide resolved

fix tests

e27daa3

Signed-off-by: kalyan <[email protected]>

dhrubo-os reviewed Nov 6, 2023

View reviewed changes

.ci/opensearch/Dockerfile Show resolved Hide resolved

rawwar added 11 commits November 6, 2023 08:52

Merge branch 'main' of https://github.com/opensearch-project/opensear…

286716e

…ch-py-ml into kalyan/289-model-access-control-

include reference

24cb85c

Signed-off-by: kalyan <[email protected]>

pr fixes

078443f

Signed-off-by: kalyan <[email protected]>

lint fix

c5cbde6

Signed-off-by: kalyan <[email protected]>

fix lint

c8dd748

Signed-off-by: kalyan <[email protected]>

fix tests

05a42b2

Signed-off-by: kalyan <[email protected]>

skip

4e47809

Signed-off-by: kalyan <[email protected]>

fix lint

bdf1201

Signed-off-by: kalyan <[email protected]>

fix lint and increase coverage

df6c069

Signed-off-by: kalyan <[email protected]>

fix lint

62b5601

Signed-off-by: kalyan <[email protected]>

fix

1993945

Signed-off-by: kalyan <[email protected]>

rawwar requested a review from dhrubo-os November 6, 2023 17:56

dhrubo-os reviewed Nov 6, 2023

View reviewed changes

feedback fixes

2c6b346

Signed-off-by: kalyan <[email protected]>

rawwar mentioned this pull request Nov 9, 2023

[FEATURE] Need to add backward compatibility test. #117

Closed

rawwar added 3 commits November 9, 2023 20:56

fix

3b16fed

Signed-off-by: kalyan <[email protected]>

lint fix

d8479ff

Signed-off-by: kalyan <[email protected]>

fix test cases

99a3660

Signed-off-by: kalyan <[email protected]>

dhrubo-os reviewed Nov 13, 2023

View reviewed changes

pr feedback fixes

ba60da7

Signed-off-by: kalyanr <[email protected]>

rbhavna reviewed Nov 13, 2023

View reviewed changes

revert

075a763

Signed-off-by: kalyanr <[email protected]>

rbhavna approved these changes Nov 13, 2023

View reviewed changes

dhrubo-os approved these changes Nov 13, 2023

View reviewed changes

dhrubo-os merged commit 2c3b744 into opensearch-project:main Nov 13, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#289 - Add register, update and delete model group functionality to support Model Access Control #332

#289 - Add register, update and delete model group functionality to support Model Access Control #332

rawwar commented Nov 3, 2023 •

edited

Loading

rawwar commented Nov 3, 2023

dhrubo-os commented Nov 3, 2023

codecov bot commented Nov 4, 2023 •

edited

Loading

rawwar commented Nov 6, 2023

dhrubo-os commented Nov 6, 2023

dhrubo-os commented Nov 13, 2023

rbhavna Nov 13, 2023

rawwar Nov 13, 2023 •

edited

Loading

rbhavna Nov 13, 2023

rawwar Nov 13, 2023

dhrubo-os Nov 13, 2023 •

edited

Loading

rbhavna Nov 13, 2023

dhrubo-os Nov 13, 2023 •

edited

Loading

rbhavna Nov 13, 2023

		raise ValueError("operation needs to be a string")


		def validate_create_model_group_parameters(

		)


		def validate_update_model_group_parameters(update_query: dict, model_group_id: str):

#289 - Add register, update and delete model group functionality to support Model Access Control #332

#289 - Add register, update and delete model group functionality to support Model Access Control #332

Conversation

rawwar commented Nov 3, 2023 • edited Loading

Description

Issues Resolved

Check List

rawwar commented Nov 3, 2023

dhrubo-os commented Nov 3, 2023

codecov bot commented Nov 4, 2023 • edited Loading

Codecov Report

rawwar commented Nov 6, 2023

dhrubo-os commented Nov 6, 2023

dhrubo-os commented Nov 13, 2023

rbhavna Nov 13, 2023

Choose a reason for hiding this comment

rawwar Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

rbhavna Nov 13, 2023

Choose a reason for hiding this comment

rawwar Nov 13, 2023

Choose a reason for hiding this comment

dhrubo-os Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

rbhavna Nov 13, 2023

Choose a reason for hiding this comment

dhrubo-os Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

rbhavna Nov 13, 2023

Choose a reason for hiding this comment

rawwar commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading

rawwar Nov 13, 2023 •

edited

Loading

dhrubo-os Nov 13, 2023 •

edited

Loading

dhrubo-os Nov 13, 2023 •

edited

Loading