-
Notifications
You must be signed in to change notification settings - Fork 146
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
enable prebuilt model #729
Conversation
Signed-off-by: Yaliang Wu <ylwu@amazon.com>
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.
Don't we need to add any test for this?
common/src/main/java/org/opensearch/ml/common/transport/upload/MLUploadInput.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/opensearch/ml/common/model/TextEmbeddingModelConfig.java
Show resolved
Hide resolved
// } | ||
// } | ||
if (url != null) { | ||
if (modelFormat == null) { |
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.
Why do we need to check modelFormat
two times?
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.
Good catch, I will remove this.
return String.format("https://ci.opensearch.org/ci/dbc/models/ml-models/%s/%s/config.json", modelName, version, Locale.ROOT); | ||
public String getPrebuiltModelConfigPath(String modelName, String version, MLModelFormat modelFormat) { | ||
String format = modelFormat.name().toLowerCase(Locale.ROOT); | ||
return String.format("%s/%s/%s/%s/config.json", MODEL_REPO, modelName, version, format, Locale.ROOT); |
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.
Should we check if the file is actually there? May be raise an exception if the model config path is not valid?
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.
The download function will throw exception if file doesn't exist.
// /huggingface/sentence-transformers/msmarco-distilbert-base-tas-b/1.0.0/onnx/config.json | ||
String format = modelFormat.name().toLowerCase(Locale.ROOT); | ||
String modelZipFileName = modelName.substring(index).replace("/", "_") + "-" + version + "-" + format; | ||
return String.format("%s/%s/%s/%s/%s.zip", MODEL_REPO, modelName, version, format, modelZipFileName, Locale.ROOT); |
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 here, may be we can check if the file is actually there? if not raise an exception saying invalid file path?
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.
The download function will throw exception if file doesn't exist.
I had another comment about testing. Don't we need any corresponding testing for this code? |
This PR is to enable pre-built model feature. |
Codecov Report
📣 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 #729 +/- ##
============================================
+ Coverage 84.95% 85.05% +0.10%
- Complexity 1076 1078 +2
============================================
Files 100 100
Lines 3922 3922
Branches 370 370
============================================
+ Hits 3332 3336 +4
+ Misses 433 430 -3
+ Partials 157 156 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
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>
Signed-off-by: Yaliang Wu <ylwu@amazon.com>
* enable prebuilt model Signed-off-by: Yaliang Wu <ylwu@amazon.com> * address comments Signed-off-by: Yaliang Wu <ylwu@amazon.com> * add unit test for prebuilt model url Signed-off-by: Yaliang Wu <ylwu@amazon.com> --------- Signed-off-by: Yaliang Wu <ylwu@amazon.com>
* enable prebuilt model Signed-off-by: Yaliang Wu <ylwu@amazon.com> * address comments Signed-off-by: Yaliang Wu <ylwu@amazon.com> * add unit test for prebuilt model url Signed-off-by: Yaliang Wu <ylwu@amazon.com> --------- Signed-off-by: Yaliang Wu <ylwu@amazon.com>
* enable prebuilt model Signed-off-by: Yaliang Wu <ylwu@amazon.com> * address comments Signed-off-by: Yaliang Wu <ylwu@amazon.com> * add unit test for prebuilt model url Signed-off-by: Yaliang Wu <ylwu@amazon.com> --------- Signed-off-by: Yaliang Wu <ylwu@amazon.com>
Description
As we have published prebuilt models to OpeSearch repo, we can enable uploading prebuilt model function now.
Example:
Issues Resolved
#745
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.