#289 - Add register, update and delete model group functionality to support Model Access Control #332
Conversation
rawwar
requested review from
dhrubo-os,
greaa-aws,
ylwu-amzn,
b4sjoo,
jngz-es and
rbhavna
as code owners
|
@dhrubo-os , can you please review and let me know if you have any feedback. I am working on adding tests |
rawwar
changed the title
|
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 <kalyan.ben10@live.com>
Codecov Report
@@ 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
|
…ch-py-ml into kalyan/289-model-access-control-
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
|
@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. |
…ch-py-ml into kalyan/289-model-access-control-
Signed-off-by: kalyan <kalyan.ben10@live.com>
Signed-off-by: kalyan <kalyan.ben10@live.com>
|
@rbhavna Could you please review this PR as you implemented the model access control. Thanks. |
Signed-off-by: kalyan <kalyan.ben10@live.com>
|
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 <kalyan.ben10@live.com>
| raise ValueError("operation needs to be a string") | ||
|
|
||
|
|
||
| def validate_create_model_group_parameters( |
There was a problem hiding this comment.
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.
@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.
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.
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 ?
There was a problem hiding this comment.
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.
| ) | ||
|
|
||
|
|
||
| def validate_update_model_group_parameters(update_query: dict, model_group_id: str): |
There was a problem hiding this comment.
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.
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.
Description
Add model group registry, update, search and deletion functionality
Issues Resolved
closes #289
closes #290
closes #292
closes #333
closes #117
Check List
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.