-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Change LC_NUMERIC for CentOS CI jobs to verify locale invariance #18097
Change LC_NUMERIC for CentOS CI jobs to verify locale invariance #18097
Conversation
Hey @nickguletskii , Thanks for submitting the PR
CI supported jobs: [unix-gpu, centos-gpu, clang, unix-cpu, sanity, windows-gpu, miscellaneous, centos-cpu, website, edge, windows-cpu] Note: |
@mxnet-bot run ci [centos-cpu, centos-gpu] |
Jenkins CI successfully triggered : [centos-cpu, centos-gpu] |
Is there some other method than inserting it at the top of every test file? That's a lot of copy&paste. |
@nickguletskii it should be easier to address the code duplication problem once #18025 is merged. I suggest you wait until it's merged. |
@nickguletskii the pytest PR is merged now. I also consolidated the CentOS dockerfiles into |
c2c8e9d
to
2bbbc66
Compare
@leezu Thank you, I've rewritten this PR to utilize pytest's conftests.py and updated the CentOS dockerfile accordingly. |
42 failed tests on centos-cpu. Seems there are still quite a few locale-unsafe parts |
@nickguletskii do you know how to reproduce the CI runs locally? For the one you're interested in, just run
|
@leezu It's actually even easier than that. It's sufficient to set the However, I don't think that fixing these issues is in scope of this PR. As I've already mentioned in the PR that introduces a hack to fix at least some of these issues (#17177), a permanent solution is to port all old operators to the new FFI. |
@nickguletskii yes, the scope of this PR doesn't need to be "fix all the issues" ;) But for the PR to be merged, you need to find a way for it to pass the CI. One idea is to use the pytest xfail features, marking the tests invoking broken behavior. You'd want to set the condition on when the test is xfail based on the operating system (centos) or the environment variable in question. Reference https://docs.pytest.org/en/latest/reference.html#pytest-mark-xfail-ref Then the PR will pass the CI and can be merged. At the same time @yzhliu and other contributors will have more data on prioritizing which operators to port to the new FFI. |
2bbbc66
to
fba512c
Compare
Hmm, it seems that something has changed in how the CentOS image is built since the last CI run. The required locale doesn't seem to be available anymore. I will try experimenting with the Docker image locally once I find some free time. |
d15ff2b
to
09d3bc0
Compare
1e93c88
to
22255cb
Compare
@mxnet-bot run ci [windows-gpu] |
Jenkins CI successfully triggered : [windows-gpu] |
@leezu Should be ready for review now. Sorry for the delay and thanks for the advice! |
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.
Thank you @nickguletskii!
…che#18097) * Load the user's locale before performing tests * Change the locale for the CentOS CI jobs to weed out locale-related bugs * Mark tests that fail due to the decimal point with xfail * Run localedef when generating the CentOS CI image * Cancel some Scala tests when C locale uses a non-standard decimal sep. * Rename xfail helper to xfail_when_nonstandard_decimal_separator * Fix scalastyle errors * Disable more Python tests that fail due to locale-related issues * Move assumeStandardDecimalSeparator into separate object to fix scaladoc * Disable the "symbol pow" test when running with non-standard decimal sep * Disable new tests that fail due to locale-related issues
Description
This pull request changes all tests to load the user's locale and changes the CentOS CI jobs to run with
LC_NUMERIC
set toen_DK.UTF8
.en_DK.UTF8
is a locale that uses,
as the decimal separator. This should weed out any bugs similar to #16134, #17140 and #18079. A partial fix for v1.x is available in #17177. Migrating operators and other API calls to the new FFI in MXNet 2.0 should resolve most locale-related issues related to API calls.This pull request makes many tests fail because they only pass on systems with
LC_NUMERIC
set to a locale that uses.
as the decimal separator.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
locale.setlocale
on initialization.LC_NUMERIC
set toen_DK.UTF8
.Comments