BUG: Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used #47570

smarie · 2022-07-01T14:47:13Z

closes BUG: tm.set_locale does not correctly set back the initial locale when locale.LC_ALL is used. tm.can_set_locale is impacted by the leak #46595
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

…ategory LC_ALL was used. Fixed pandas-dev#46595

smarie · 2022-07-01T14:52:07Z

It would be great to add a test reproducing the initial problem, before fixing it.
However it is a bit tricky because we need to execute this test only when we know the locale is available. And of course we can't use can_set_locale for this :)
#46595 (comment)

…ocale on it_IT and zh_CN runners, as `strict=False`.

…ure/46405_set_locale_bug

… avoid misuses of `getlocale` (see pandas-dev#46595). Added tests.

smarie · 2022-07-06T12:20:14Z

@jbrockmendel @mroeschke : Please review this very small PR to fix a leakage problem in set_locale (#46595).

When the problem was first fixed, then a few tests started to fail on workers it_IT and zh_CN (see https://github.com/pandas-dev/pandas/actions/runs/2597381464). Looking more closely at the problem, I saw that this was tests marked as xfail if the locale is... it_IT or zh_CN :)

https://github.com/pandas-dev/pandas/runs/7152086548?check_suite_focus=true#step:7:54

=================================== FAILURES ===================================
_ TestTimeConversionFormats.test_to_datetime_format_time[True-01/10/2010 08:14 PM-%m/%d/%Y %I:%M %p-dt3] _
[gw0] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_ TestTimeConversionFormats.test_to_datetime_format_time[True-01/10/2010 07:40 AM-%m/%d/%Y %I:%M %p-dt4] _
[gw0] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_ TestTimeConversionFormats.test_to_datetime_format_time[True-01/10/2010 09:12:56 AM-%m/%d/%Y %I:%M:%S %p-dt5] _
[gw0] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_ TestTimeConversionFormats.test_to_datetime_format_time[False-01/10/2010 08:14 PM-%m/%d/%Y %I:%M %p-dt3] _
[gw0] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_ TestTimeConversionFormats.test_to_datetime_format_time[False-01/10/2010 07:40 AM-%m/%d/%Y %I:%M %p-dt4] _
[gw0] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_ TestTimeConversionFormats.test_to_datetime_format_time[False-01/10/2010 09:12:56 AM-%m/%d/%Y %I:%M:%S %p-dt5] _
[gw0] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_____________________ TestToTime.test_parsers_time[2:15pm] _____________________
[gw1] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
_____________________ TestToTime.test_parsers_time[0215pm] _____________________
[gw1] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
___________________ TestToTime.test_parsers_time[2:15:00pm] ____________________
[gw1] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8
____________________ TestToTime.test_parsers_time[021500pm] ____________________
[gw1] linux -- Python 3.8.13 /home/runner/micromamba/envs/test/bin/python
[XPASS(strict)] fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8

This was actually a good thing: it proves that the fix worked !

I simply added the strict=False flag to the corresponding xfail marks, because currently pytest is run with strict xfail mode.
I also added a few tests, as well as a convenience function get_current_locale to ease long-term maintenance if this kind of very hard-to-debug issue.

When this will be merged, we will be able to proceed with PR #46405 so stay tuned :)

…ure/46405_set_locale_bug

pandas/tests/config/test_localization.py

pandas/_config/localization.py

jbrockmendel · 2022-07-06T15:56:10Z

pandas/tests/tools/test_to_datetime.py

@@ -291,6 +291,7 @@ def test_to_datetime_format_microsecond(self, cache):
                marks=pytest.mark.xfail(
                    locale.getlocale()[0] in ("zh_CN", "it_IT"),
                    reason="fail on a CI build with LC_ALL=zh_CN.utf8/it_IT.utf8",
+                    strict=False,


can we just remove these xfails entirely?

No, these tests are executed on other workers, and they pass. So they are important :) We should actually try to solve the issue (datetime parsing, separate topic) if we wish to remove the xfail mark

Can we be make the xfail condition more specific? strict=False is really undesirable

This is already quite specific: the test will be marked as xfail only if locale.getlocale()[0] in ("zh_CN", "it_IT"). So for all CI workers except the 2 workers zh_CN and it_IT, the tests are actually correctly executed (and they pass)

Note that I did not write this code, it was already there. But since there was a locale leakage bug, the zh_CN and it_IT locale was actually never active (even on the 2 corresponding CI workers) when hitting the code. This is why noone every needed to add the strict=False flag.

It's not clear to me why strict=False is needed i.e. why does it sometimes pass? Is it because the locale setting isn't threadsafe? If so, these tests can be decorated with @pytest.mark.single_cpu

This is less specific IMO. See below : on test_to_time you use a fails_on_non_english mark (for readability) but with still a test about zh_CN and it_IT (because for other locales we actually do not know if it would pass or fail, therefore we do not want to use the xfail mark too prematurely). Let me know

I'm okay with strict=False for now then especially if there's a path to know how we can remove these in the future.

I'm okay with strict=False for now then especially if there's a path to know how we can remove these in the future.

im not clear on what this path is. can you elaborate on this?

solve the issue (datetime parsing, separate topic)

From your investigation @smarie fixing the datetime parsing would allow us to remove the xfail?

Yes, fixing the parsing for these two locales would remove the xfail.

pandas/_config/localization.py

mroeschke

LGTM given the tests passing

jreback · 2022-07-08T22:57:02Z

thanks @smarie

smarie · 2022-07-11T07:16:05Z

Thanks to both of you for the fast review !

…hen category LC_ALL was used (pandas-dev#47570)

Fixed tm.set_locale context manager, it could error and leak when c…

1cba2e9

…ategory LC_ALL was used. Fixed pandas-dev#46595

smarie mentioned this pull request Jul 1, 2022

BUG: Fixed Unicode decoding error in Period.strftime when a locale-specific directive is used #46405

Merged

4 tasks

Sylvain MARIE added 4 commits July 6, 2022 09:25

Marked xfailing tests that now correctly run with the appropriate l…

149556f

…ocale on it_IT and zh_CN runners, as `strict=False`.

Merge branch 'main' of https://github.com/pandas-dev/pandas into feat…

59cd733

…ure/46405_set_locale_bug

Added a get_current_locale function, compliant with setlocale, to…

f692b5f

… avoid misuses of `getlocale` (see pandas-dev#46595). Added tests.

Added get_current_locale to the testing module for convenience

a651167

smarie changed the title ~~Fixed tm.set_locale context manager, it could error and leak when category LC_ALL was used~~ BUG: Fixed tm.set_locale context manager, it could error and leak when category LC_ALL was used Jul 6, 2022

Merge branch 'main' of https://github.com/pandas-dev/pandas into feat…

efcc805

…ure/46405_set_locale_bug

smarie commented Jul 6, 2022

View reviewed changes

pandas/tests/config/test_localization.py Outdated Show resolved Hide resolved

Minor improvement in test_set_locale

d1f9cf9

jbrockmendel reviewed Jul 6, 2022

View reviewed changes

pandas/_config/localization.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jul 6, 2022

View reviewed changes

pandas/_config/localization.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jul 6, 2022

View reviewed changes

pandas/_config/localization.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jul 6, 2022

View reviewed changes

mroeschke reviewed Jul 6, 2022

View reviewed changes

pandas/_config/localization.py Outdated Show resolved Hide resolved

Sylvain MARIE added 3 commits July 7, 2022 12:12

Code review: improved get_current_locale

b53796b

Code review: removed get_current_locale

6b863c3

Code review: comment

38b7e26

mroeschke added Testing pandas testing functions or related to the test suite Localization Internationalization of data labels Jul 8, 2022

mroeschke added this to the 1.5 milestone Jul 8, 2022

mroeschke approved these changes Jul 8, 2022

View reviewed changes

jreback merged commit 8e6ca28 into pandas-dev:main Jul 8, 2022

smarie deleted the feature/46405_set_locale_bug branch July 11, 2022 07:15

yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022

BUG: Fixed tm.set_locale context manager, it could error and leak w…

123855c

…hen category LC_ALL was used (pandas-dev#47570)

smarie mentioned this pull request Dec 12, 2022

[WIP] perf improvements for strftime #46116

Closed

5 tasks

smarie mentioned this pull request May 14, 2023

[READY] perf improvements for strftime #51298

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used #47570

BUG: Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used #47570

smarie commented Jul 1, 2022 •

edited

Loading

smarie commented Jul 1, 2022 •

edited

Loading

smarie commented Jul 6, 2022 •

edited

Loading

jbrockmendel Jul 6, 2022

smarie Jul 7, 2022 •

edited

Loading

jbrockmendel Jul 7, 2022

smarie Jul 7, 2022 •

edited

Loading

mroeschke Jul 7, 2022

smarie Jul 8, 2022 •

edited

Loading

mroeschke Jul 8, 2022

jbrockmendel Jul 9, 2022

mroeschke Jul 10, 2022

smarie Jul 11, 2022

mroeschke left a comment

jreback commented Jul 8, 2022

smarie commented Jul 11, 2022

BUG: Fixed tm.set_locale context manager, it could error and leak when category LC_ALL was used #47570

BUG: Fixed tm.set_locale context manager, it could error and leak when category LC_ALL was used #47570

Conversation

smarie commented Jul 1, 2022 • edited Loading

smarie commented Jul 1, 2022 • edited Loading

smarie commented Jul 6, 2022 • edited Loading

jbrockmendel Jul 6, 2022

Choose a reason for hiding this comment

smarie Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

jbrockmendel Jul 7, 2022

Choose a reason for hiding this comment

smarie Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

mroeschke Jul 7, 2022

Choose a reason for hiding this comment

smarie Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

mroeschke Jul 8, 2022

Choose a reason for hiding this comment

jbrockmendel Jul 9, 2022

Choose a reason for hiding this comment

mroeschke Jul 10, 2022

Choose a reason for hiding this comment

smarie Jul 11, 2022

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

jreback commented Jul 8, 2022

smarie commented Jul 11, 2022

BUG: Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used #47570

BUG: Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used #47570

smarie commented Jul 1, 2022 •

edited

Loading

smarie commented Jul 1, 2022 •

edited

Loading

smarie commented Jul 6, 2022 •

edited

Loading

smarie Jul 7, 2022 •

edited

Loading

smarie Jul 7, 2022 •

edited

Loading

smarie Jul 8, 2022 •

edited

Loading