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

rename API/function/variable names #822

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

ylwu-amzn
Copy link
Collaborator

@ylwu-amzn ylwu-amzn commented Mar 23, 2023

Description

Rename APIs

  • upload model -> register model
  • load model -> deploy model
  • unload model -> undeploy model

With these changes, we also changed other places

  • model state, for example LOADED -> DEPLOYED
  • functions names, for example uploadModel -> deployModel
  • variable names, for example uploadInput -> registerModelInput

Issues Resolved

#823

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: Yaliang Wu <ylwu@amazon.com>
@ylwu-amzn ylwu-amzn requested a review from a team March 23, 2023 08:27
@@ -29,22 +29,22 @@
@ToString
public class MLUploadModelChunkRequest extends ActionRequest {

MLUploadModelChunkInput mlUploadInput;
MLUploadModelChunkInput uploadModelChunkInput;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are using register for full model model, why not use register model chunk here?

Copy link
Collaborator Author

@ylwu-amzn ylwu-amzn Mar 23, 2023

Choose a reason for hiding this comment

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

We change to register mainly for extending the API scope to support pre-trained model on model repo and virtual remote models. This upload model chunk API doesn't need to extend its scope. But I will reconfirm with UX/PM.

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'd suggest go ahead and review other parts. We can create a new PR for the model chunk APIs if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still waiting for more comments from UX/PM. Will merge this PR for now. May cut a new PR when get finalized decision.

Signed-off-by: Yaliang Wu <ylwu@amazon.com>
Zhangxunmt
Zhangxunmt previously approved these changes Mar 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

Merging #822 (1d2ba9f) into 2.x (4df13e3) will decrease coverage by 0.16%.
The diff coverage is 84.89%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##                2.x     #822      +/-   ##
============================================
- Coverage     84.90%   84.74%   -0.16%     
+ Complexity     1626     1622       -4     
============================================
  Files           135      135              
  Lines          6036     6059      +23     
  Branches        589      592       +3     
============================================
+ Hits           5125     5135      +10     
- Misses          659      668       +9     
- Partials        252      256       +4     
Flag Coverage Δ
ml-commons 84.74% <84.89%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
...l/engine/algorithms/ad/AnomalyDetectionLibSVM.java 86.84% <0.00%> (ø)
...va/org/opensearch/ml/task/MLPredictTaskRunner.java 82.14% <0.00%> (ø)
.../main/java/org/opensearch/ml/task/MLTaskCache.java 93.75% <ø> (ø)
.../java/org/opensearch/ml/utils/RestActionUtils.java 81.63% <ø> (ø)
...h/ml/action/models/DeleteModelTransportAction.java 85.07% <25.00%> (-3.99%) ⬇️
...ain/java/org/opensearch/ml/task/MLTaskManager.java 68.33% <50.00%> (ø)
.../opensearch/ml/rest/RestMLRegisterModelAction.java 73.07% <73.07%> (ø)
...n/java/org/opensearch/ml/model/MLModelManager.java 78.92% <76.62%> (ø)
...c/main/java/org/opensearch/ml/engine/MLEngine.java 83.58% <80.00%> (ø)
...n/java/org/opensearch/ml/cluster/MLSyncUpCron.java 76.92% <80.00%> (+0.51%) ⬆️
... and 18 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Yaliang Wu <ylwu@amazon.com>
@ylwu-amzn ylwu-amzn merged commit 158d445 into opensearch-project:2.x Mar 24, 2023
ylwu-amzn added a commit to ylwu-amzn/ml-commons that referenced this pull request Mar 24, 2023
* rename API/function/variable names

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* remove deprecated fields from model index

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* fix some variable name

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

---------

Signed-off-by: Yaliang Wu <ylwu@amazon.com>
ylwu-amzn added a commit to ylwu-amzn/ml-commons that referenced this pull request Mar 24, 2023
* rename API/function/variable names

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* remove deprecated fields from model index

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* fix some variable name

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

---------

Signed-off-by: Yaliang Wu <ylwu@amazon.com>
ylwu-amzn added a commit that referenced this pull request Mar 24, 2023
* rename API/function/variable names



* remove deprecated fields from model index



* fix some variable name



---------

Signed-off-by: Yaliang Wu <ylwu@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants