Prevents Locale Code build failures #2967 #2980

brianf-aws · 2024-09-24T00:33:15Z

Description

Depending on the GitHub CI workflow executes it will invoke ./gradlew build which will run tests on random parameters every time, there are locale-code's that will break a build one of them being az-Cyrl. The solution was to update the codebase that involved string operations such as toUpperCase(), toLowerCase() to toUpperCase(Locale.Root), toLowerCase(Locale.Root) In doing this we make the string operations language agnostic meaning it wont be affected on a computer that is set to a different language like Spanish or Turkish.

Testing

./gradlew build -DTests.Locale=az-Cyrl which previously failed now works
./gradlew build builds successfully too.

TLDR:

The change was to update String operations to use Locale.Root as a argument to avoid default Locale (which may depend on one that is not supported) you can see this here .

Related Issues

Resolves #2967

Check List

New functionality includes testing.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

Depending on the GitHub CI workflow executes it will invoke ./gradlew build which will run tests on random parameters everytime, there are local-code's that will break a build one of them being az-Cyrl. The solution here was preventing (explicitly writing the local to be set to en-US) Github actions building on these random parameters that will interupt a build and lose time. Manual testing was done to prove that using this flag breaks a build, you can check the issue opensearch-project#2967 or you can run ./gradlew build -Dtests.local=az-Cyrl to see it breaks Signed-off-by: Brian Flores <[email protected]>

dbwiddis

This seems like the wrong fix for the issue.

There's clearly a locale-based bug somewhere but instead of fixing it you're ignoring it for the entire build, even for tests unaffected by the issue. This would miss issues such as #1734

All of the failing tests in the linked issue are in the RestMLPredictionActionTests class, so if those tests require en-US locale that should be scoped only to those tests.

However, locale based bugs are very frequently using upper- or lower-case string transformations. This line has a toUpperCase() transformation without a locale and is very likely the bug that needs fixing by adding Locale.ROOT. (There may be others.)

ml-commons/plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java

Line 132 in 6a6cac1

    
           } else if (FunctionName.isDLModel(FunctionName.from(algorithm.toUpperCase())) && !mlFeatureEnabledSetting.isLocalModelEnabled()) {

A brief search of the entire repo shows several similar potential locale issues, you should probably fix them all!

If you want to go a step further, converting a string by upper/lower casing just for a comparison is inefficient as it creates a whole new string object which is immediately discarded. If done frequently this adds to the heap/GC load. Use .equalsIgnoreCase() when you can to avoid this extra object creation.

dbwiddis · 2024-09-24T08:30:43Z

Update: this is the specific bug location, but as noted above you should fix all of them.

ml-commons/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java

Line 211 in 6a6cac1

ActionType.valueOf(action.toUpperCase());

brianf-aws · 2024-09-24T17:16:29Z

Hey Dan! Thanks for the insight, I agree with you I did feel skeptical making the change Ill get to work with updating the string methods to use Locale.ROOT

The previous commit made a hard change on the build while ignoring the root problem, which was making sure that our codebase currently supports string operations regardless of the locale code. In this new commit String operations like toUpperCase have a extra argument of Locale.Root making the codebase agnostic to the rules of other langugages such as Spanish or Turkish. Manual testing was done like raised in the GitHub issue opensearch-project#2967 also ./gradlew build -Dtests.Local=az-Cyrl passes Signed-off-by: Brian Flores <[email protected]>

dhrubo-os · 2024-09-25T00:44:04Z

Thanks for making the changes.

dbwiddis

LGTM Someone should probably do an org-wide search for these. :)

(Edit: just did 172 of them! Holy cow!)

* Prevents future UT build issues addresses #2967 Depending on the GitHub CI workflow executes it will invoke ./gradlew build which will run tests on random parameters everytime, there are local-code's that will break a build one of them being az-Cyrl. The solution here was preventing (explicitly writing the local to be set to en-US) Github actions building on these random parameters that will interupt a build and lose time. Manual testing was done to prove that using this flag breaks a build, you can check the issue #2967 or you can run ./gradlew build -Dtests.local=az-Cyrl to see it breaks Signed-off-by: Brian Flores <[email protected]> * Updates current codebase with Local.Root to string operations The previous commit made a hard change on the build while ignoring the root problem, which was making sure that our codebase currently supports string operations regardless of the locale code. In this new commit String operations like toUpperCase have a extra argument of Locale.Root making the codebase agnostic to the rules of other langugages such as Spanish or Turkish. Manual testing was done like raised in the GitHub issue #2967 also ./gradlew build -Dtests.Local=az-Cyrl passes Signed-off-by: Brian Flores <[email protected]> --------- Signed-off-by: Brian Flores <[email protected]> (cherry picked from commit cd83590)

ylwu-amzn · 2024-09-26T17:49:55Z

LGTM Someone should probably do an org-wide search for these. :)

(Edit: just did 172 of them! Holy cow!)

Thanks Dan, can you create issues for the impacted repos ?

dbwiddis · 2024-09-27T05:23:00Z

LGTM Someone should probably do an org-wide search for these. :)
(Edit: just did 172 of them! Holy cow!)

Thanks Dan, can you create issues for the impacted repos ?

Yep, sometime next week. Filtering to just Java files it's only 66.

* Prevents future UT build issues addresses #2967 Depending on the GitHub CI workflow executes it will invoke ./gradlew build which will run tests on random parameters everytime, there are local-code's that will break a build one of them being az-Cyrl. The solution here was preventing (explicitly writing the local to be set to en-US) Github actions building on these random parameters that will interupt a build and lose time. Manual testing was done to prove that using this flag breaks a build, you can check the issue #2967 or you can run ./gradlew build -Dtests.local=az-Cyrl to see it breaks Signed-off-by: Brian Flores <[email protected]> * Updates current codebase with Local.Root to string operations The previous commit made a hard change on the build while ignoring the root problem, which was making sure that our codebase currently supports string operations regardless of the locale code. In this new commit String operations like toUpperCase have a extra argument of Locale.Root making the codebase agnostic to the rules of other langugages such as Spanish or Turkish. Manual testing was done like raised in the GitHub issue #2967 also ./gradlew build -Dtests.Local=az-Cyrl passes Signed-off-by: Brian Flores <[email protected]> --------- Signed-off-by: Brian Flores <[email protected]> (cherry picked from commit cd83590) Co-authored-by: Brian Flores <[email protected]>

brianf-aws requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27, sam-herman and xinyual as code owners September 24, 2024 00:33

brianf-aws temporarily deployed to ml-commons-cicd-env September 24, 2024 00:33 — with GitHub Actions Inactive

opensearch-trigger-bot bot added the infra label Sep 24, 2024

brianf-aws had a problem deploying to ml-commons-cicd-env September 24, 2024 00:33 — with GitHub Actions Failure

dbwiddis suggested changes Sep 24, 2024

View reviewed changes

brianf-aws temporarily deployed to ml-commons-cicd-env September 24, 2024 19:07 — with GitHub Actions Inactive

brianf-aws temporarily deployed to ml-commons-cicd-env September 24, 2024 20:06 — with GitHub Actions Inactive

brianf-aws requested a review from dbwiddis September 24, 2024 20:51

brianf-aws changed the title ~~Prevents future UT build issues addresses #2967~~ Prevents Locale Code build failures #2967 Sep 25, 2024

dhrubo-os approved these changes Sep 25, 2024

View reviewed changes

dbwiddis approved these changes Sep 25, 2024

View reviewed changes

rbhavna approved these changes Sep 26, 2024

View reviewed changes

dhrubo-os added the backport 2.x label Sep 26, 2024

dhrubo-os merged commit cd83590 into opensearch-project:main Sep 26, 2024
7 checks passed

opensearch-trigger-bot bot mentioned this pull request Sep 26, 2024

[Backport 2.x] Prevents Locale Code build failures #2967 #2991

Merged

brianf-aws deleted the gh-ci-build-issue branch September 26, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevents Locale Code build failures #2967 #2980

Prevents Locale Code build failures #2967 #2980

brianf-aws commented Sep 24, 2024 •

edited

Loading

dbwiddis left a comment

dbwiddis commented Sep 24, 2024

brianf-aws commented Sep 24, 2024 •

edited

Loading

dhrubo-os commented Sep 25, 2024

dbwiddis left a comment •

edited

Loading

ylwu-amzn commented Sep 26, 2024 •

edited

Loading

dbwiddis commented Sep 27, 2024

Prevents Locale Code build failures #2967 #2980

Prevents Locale Code build failures #2967 #2980

Conversation

brianf-aws commented Sep 24, 2024 • edited Loading

Description

Testing

Related Issues

Check List

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis commented Sep 24, 2024

brianf-aws commented Sep 24, 2024 • edited Loading

dhrubo-os commented Sep 25, 2024

dbwiddis left a comment • edited Loading

Choose a reason for hiding this comment

ylwu-amzn commented Sep 26, 2024 • edited Loading

dbwiddis commented Sep 27, 2024

brianf-aws commented Sep 24, 2024 •

edited

Loading

brianf-aws commented Sep 24, 2024 •

edited

Loading

dbwiddis left a comment •

edited

Loading

ylwu-amzn commented Sep 26, 2024 •

edited

Loading