Skip to content
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

fix cannot specify model access control parameters error #1068

Merged
merged 3 commits into from
Jul 11, 2023

Conversation

ylwu-amzn
Copy link
Collaborator

@ylwu-amzn ylwu-amzn commented Jul 11, 2023

Description

See such error when register pretrained model without model group id,

{
	"error": {
		"root_cause": [
			{
				"type": "illegal_argument_exception",
				"reason": "You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster."
			}
		],
		"type": "illegal_argument_exception",
		"reason": "You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster."
	},
	"status": 400
}

The reason is input.getBackendRoles() is not null, but it's empty.
This PR fixed the bug.

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.

if (input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null) {
if (input.getModelAccessMode() != null
|| input.getIsAddAllBackendRoles() != null
|| !CollectionUtils.isEmpty(input.getBackendRoles())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems strange, we better not allow backend_roles field show up in user's request instead of passing a empty list, can you change to this behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, think the same. I submit a new commit which add unit test for model group and fix some bug

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can change this back to input.getBackEndRoles() != null, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think keep as CollectionUtils.isEmpty is safer. Do you see any bug if we keep CollectionUtils.isEmpty?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean we better make the field not showing up when access control is not enabled, user can pass the backend_roles field with empty list when using CollectionUtils.isEmpty, I prefer to make the field not shown.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #1068 (9012a96) into 2.x (1602aea) will increase coverage by 0.01%.
The diff coverage is 33.33%.

❗ Current head 9012a96 differs from pull request most recent head e20dc0b. Consider uploading reports for the commit e20dc0b to get more accurate results

@@             Coverage Diff              @@
##                2.x    #1068      +/-   ##
============================================
+ Coverage     77.32%   77.33%   +0.01%     
- Complexity     2052     2054       +2     
============================================
  Files           167      167              
  Lines          8532     8534       +2     
  Branches        846      848       +2     
============================================
+ Hits           6597     6600       +3     
+ Misses         1557     1555       -2     
- Partials        378      379       +1     
Flag Coverage Δ
ml-commons 77.33% <33.33%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...a/org/opensearch/ml/model/MLModelGroupManager.java 45.53% <33.33%> (+0.08%) ⬆️

... and 2 files with indirect coverage changes

@ylwu-amzn ylwu-amzn merged commit 6294a76 into opensearch-project:2.x Jul 11, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 12, 2023
* fix cannot specify model access control parameters error

Signed-off-by: Yaliang Wu <[email protected]>

* add unit test for model group class and fix some bug

Signed-off-by: Yaliang Wu <[email protected]>

* fix failed ut

Signed-off-by: Yaliang Wu <[email protected]>

---------

Signed-off-by: Yaliang Wu <[email protected]>
(cherry picked from commit 6294a76)
rbhavna pushed a commit that referenced this pull request Jul 12, 2023
* fix cannot specify model access control parameters error

Signed-off-by: Yaliang Wu <[email protected]>

* add unit test for model group class and fix some bug

Signed-off-by: Yaliang Wu <[email protected]>

* fix failed ut

Signed-off-by: Yaliang Wu <[email protected]>

---------

Signed-off-by: Yaliang Wu <[email protected]>
(cherry picked from commit 6294a76)

Co-authored-by: Yaliang Wu <[email protected]>
zane-neo pushed a commit to zane-neo/ml-commons that referenced this pull request Sep 1, 2023
…project#1068)

* fix cannot specify model access control parameters error

Signed-off-by: Yaliang Wu <[email protected]>

* add unit test for model group class and fix some bug

Signed-off-by: Yaliang Wu <[email protected]>

* fix failed ut

Signed-off-by: Yaliang Wu <[email protected]>

---------

Signed-off-by: Yaliang Wu <[email protected]>
zane-neo pushed a commit that referenced this pull request Sep 1, 2023
* fix cannot specify model access control parameters error

Signed-off-by: Yaliang Wu <[email protected]>

* add unit test for model group class and fix some bug

Signed-off-by: Yaliang Wu <[email protected]>

* fix failed ut

Signed-off-by: Yaliang Wu <[email protected]>

---------

Signed-off-by: Yaliang Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants