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.

Sign up for GitHub

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

Jump to bottom

Fix string.format wrong parameter position #1921

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Fix string.format wrong parameter position #1921

merged 1 commit into from
Jan 30, 2024

Conversation

Hailong-am
Copy link
Contributor

Description

Fix string.format wrong parameter position, the Locale should be first parameter

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.

@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 02:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 02:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 02:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 02:59 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 02:59 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 02:59 — with GitHub Actions Inactive
@Hailong-am
Copy link
Contributor Author

@ylwu-amzn Search the whole codebase also, there have few places use string.format without specifying Locale, do you think we should add for those?

@codecov Codecov
Copy link

codecov bot commented Jan 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d0895bb) 82.94% compared to head (6cfb33c) 82.95%.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #1921   +/-   ##
=========================================
  Coverage     82.94%   82.95%           
- Complexity     5414     5415    +1     
=========================================
  Files           521      521           
  Lines         21709    21709           
  Branches       2213     2213           
=========================================
+ Hits          18007    18009    +2     
+ Misses         2807     2804    -3     
- Partials        895      896    +1
Flag Coverage Δ
ml-commons 82.95% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 03:25 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 03:25 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env January 25, 2024 03:25 — with GitHub Actions Inactive
sam-herman
sam-herman requested changes Jan 26, 2024
View reviewed changes
ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java
@@ -58,7 +58,7 @@ public String getPrebuiltModelMetaListPath() {

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);
return String.format(Locale.ROOT, "%s/%s/%s/%s/config.json", MODEL_REPO, modelName, version, format);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a simple UT that shows the behavior that is fixed? It's usually a common industry practice when fixing a bug to make sure that there is going forward a UT that clearly document the behavior change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this fix didn't change the behavior of the method. Put locale at last that will ignored by String.format method as parameters length is greater than conversions length, the superfluous one will ignored.

For UT since there already have one which will cover this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, another comment, might not be directly related to this change, but as a future FYI
this method seem like it should be static because it has no dependency on object level instance.
If we want to keep it non-static we should leverage existing parameters in the object instance instead.

sam-herman
sam-herman approved these changes Jan 29, 2024
View reviewed changes
ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java
@@ -58,7 +58,7 @@ public String getPrebuiltModelMetaListPath() {

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);
return String.format(Locale.ROOT, "%s/%s/%s/%s/config.json", MODEL_REPO, modelName, version, format);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, another comment, might not be directly related to this change, but as a future FYI
this method seem like it should be static because it has no dependency on object level instance.
If we want to keep it non-static we should leverage existing parameters in the object instance instead.

@ylwu-amzn
Copy link
Collaborator

@ylwu-amzn Search the whole codebase also, there have few places use string.format without specifying Locale, do you think we should add for those?

Thanks, we should add this

@ylwu-amzn ylwu-amzn merged commit 991193c into opensearch-project:main Jan 30, 2024
15 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 30, 2024
@Hailong-am @github-actions
fix string.foramt wrong parameter position (#1921)
ee630f6 
Signed-off-by: Hailong Cui <[email protected]>
(cherry picked from commit 991193c)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 30, 2024
Merged
ylwu-amzn pushed a commit that referenced this pull request Feb 1, 2024
@opensearch-trigger-bot @Hailong-am
fix string.foramt wrong parameter position (#1921) (#1960)
e98d6ec 
Signed-off-by: Hailong Cui <[email protected]>
(cherry picked from commit 991193c)

Co-authored-by: Hailong Cui <[email protected]>
austintlee pushed a commit to austintlee/ml-commons that referenced this pull request Mar 19, 2024
@Hailong-am @austintlee
fix string.foramt wrong parameter position (opensearch-project#1921)
59eacc5 
Signed-off-by: Hailong Cui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-herman sam-herman sam-herman approved these changes

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@HenryL27 HenryL27 HenryL27 approved these changes

@b4sjoo b4sjoo Awaiting requested review from b4sjoo b4sjoo is a code owner

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

Assignees
No one assigned
Labels
backport 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Hailong-am @ylwu-amzn @HenryL27 @sam-herman