Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
#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
Changes from all commits
a2ffe54
7bef831
61c3090
a89bb39
3bbea93
ca765ec
4c6a7fd
af60958
cde4e1f
28040ee
d8fc9b2
cda4c08
1ebeb1a
1b72089
e27daa3
7edafe9
2e0f9c4
f9bc8c8
24ea966
84848e2
08256f6
9547db3
06f097d
594baad
203aa86
19fdf80
6bdcfcd
76fcd84
50bacd4
b5133b9
2b529a3
b61c1e3
d5d5830
8f46cf9
d2f832e
286716e
24cb85c
078443f
c5cbde6
c8dd748
05a42b2
4e47809
bdf1201
df6c069
62b5601
1993945
2c6b346
3b16fed
d8479ff
99a3660
ba60da7
075a763
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 93 in opensearch_py_ml/ml_commons/model_access_control.py
Codecov / codecov/patch
opensearch_py_ml/ml_commons/model_access_control.py#L91-L93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I do feel we shouldn't be having these checks in
opensearch-py-ml
. Because, if checks onml-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 theopensearch
, 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 fromml-commons
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Check warning on line 72 in opensearch_py_ml/ml_commons/validators/model_access_control.py
Codecov / codecov/patch
opensearch_py_ml/ml_commons/validators/model_access_control.py#L72
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agreed @dhrubo-os