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

smarie · 2022-03-17T13:57:38Z

closes BUG: UnicodeError when using Period.strftime with non-utf8 locale #46319 , closes Not tested: Period.strftime and PeriodIndex.strftime with non-ascii char present in the formatting string #46468
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

Note: this was extracted from #46116 to decompose the big PR into reviewable bits. More to come soon

smarie · 2022-03-17T15:26:01Z

I have an issue: I can reproduce the bug locally but not on CI.
It seems that there is no CI runner as of today that uses non-utf8 encoded locale.

The issue can only be reproduced when the current locale encoding is not utf8:

(zh_CN defaults encoding is gb2312)

What is the recommended way to proceed ?

fix the issue without having the CI reproduce it ? (not recommended...)
add a workflow in posix.yml
other ?

I'll try the second option for now (when github issues are fixed, it seems down for now)

smarie · 2022-03-17T15:43:25Z

gha runner was not triggered apparently :( re-trying

smarie · 2022-03-17T16:04:26Z

Unnfortunately it does not work :(

/usr/bin/bash: warning: setlocale: LC_ALL: cannot change locale (zh_CN.GB2312)

EDIT: retrying with zh_CN.gb2312

smarie · 2022-03-17T16:34:02Z

ok with lower case zh_CN.gb2312 it is worse: the runner does not even start :(
Can anyone help me to define a runner configured with this locale+encoding ? (@jreback maybe ?) Otherwise I am not able to reproduce the issue on CI, which is a bit problematic to prevent regression once I push the fix.

…e the issue.

.github/workflows/posix.yml

pandas/tests/io/formats/test_format.py

…ure/46319_locale_strftime_period � Conflicts: � pandas/tests/io/formats/test_format.py

…ht encoding !

smarie · 2022-03-18T20:27:22Z

I made progress: I was able to install the missing locale. However this leads to a segmentation fault !

https://github.com/pandas-dev/pandas/runs/5606041220?check_suite_focus=true#step:12:22

I will not try to resolve this in this PR.

Instead, my new attempt will be to install (but not activate) the zh_CN locale in the zh_ZH.utf-8 worker. It will then only be activated in the test requiring it.

smarie · 2022-03-19T09:53:08Z

It worked ! :)

https://github.com/pandas-dev/pandas/runs/5606395924?check_suite_focus=true#step:12:57

Now that the bug can be reproduced on CI, I will push the fix, probably monday.

…` to conftest

smarie · 2022-03-19T13:21:49Z

All set - eventually I did it today :)
Ready for review. Maybe @mroeschke or @jbrockmendel ?

pandas/tests/io/formats/test_format.py

smarie · 2022-03-19T13:25:53Z

pandas/conftest.py

+    params=[
+        pytest.param(None, id=str(locale.getlocale())),
+        "it_IT.utf8",
+        "zh_CN",


Maybe

Suggested change

"zh_CN",

"zh_CN", # Note: encoding is automatically 'gb2312'

What do you think ?

I think it would be good to add zh_CH.utf8 and zh_CH.gb2312 explicitly

Ok, I'll add zh_CH.utf8.
Strangely enough, zh_CN.gb2312 did not seem to be working, but zh_CN does involve gb2312 .

I will also add it_IT by the way, so that both locales have their utf8 AND non-utf8 encoding activated in the test

.github/workflows/posix.yml

mroeschke · 2022-03-19T18:10:17Z

.github/workflows/posix.yml

+    - name: Generate extra locales
+      # These extra locales will be available for locale.setlocale() calls in tests
+      run: |
+        sudo locale-gen ${{ env.EXTRA_LOC }}


Does this mean the Locale: it_IT.utf8 isnt working either?

It depends what you mean by "working". Do you refer to the bug #46319 or to something more general ?

The issue BUG: UnicodeError when using Period.strftime with non-utf8 locale #46319 happens only when the current locale uses a non-unicode encoding (no utf-8). So it does not happen in any of the workers before this PR

So in the test fixture parameters I added the zh_CN locale, that has a non-utf-8 encoding.

Unfortunately this particular parameter value was never used in any of the workers (the test was skipped), since locale.setlocale was raising an error (locale not available)

I finally found a way to make the locale available on linux workers: sudo locale-gen <locale>

Does that answer your question ?

I think it was assumed setting the environment variables

LANG: ${{ matrix.lang || '' }} LC_ALL: ${{ matrix.lc_all || '' }}

would ensure the locale for the test run would not be the default (en_US.UTF-8) for the other locale builds.

So IIUC, this is necessary if setting a non-utf8 locale?

You are right, this is what I thought too.
Unfortunately when I tried setting LANG and LC_ALL to zh_CN (non-utf8), then there was a strange segmentation fault appearing (see #46405 (comment))

So this is why I now do not set it through these env variables but I only install it.

Maybe the problem is that when the env variables are set, many side effects happen in the build chain, that I do not quite understand.

If we're setting the locale this way, can we remove setting the LANG and LC_ALL environment variables here?

Honestly I have no idea :) At least for the tests in this PR, yes these env variables are not used.

However these env vars are probably related to other locale-related tests, including being able to compile pandas on alternate locale, etc. It seems that in the CI those env variables are used to even add a specific line at the beginning of the init file of pandas ! (see ci/setup_env.sh)

This seems a bit beyond the scope of this PR. However it is maybe worth keeping track of this, in a dedicated ticket.

…ure/46319_locale_strftime_period � Conflicts: � .github/workflows/ubuntu.yml

smarie · 2022-09-04T08:28:54Z

@mroeschke @jreback @jbrockmendel all set now. Sorry for the delay.
Note that the only failing CI build seems completely unrelated (the issue is about a TLS certificate bundle)

Maybe still on time for 1.5 milestone ?

pandas/tests/io/parser/test_quoting.py

mroeschke · 2022-09-06T18:56:19Z

Maybe still on time for 1.5 milestone ?

Sorry, since we released the rc already, we are only backporting fixes that are uncovered in the 1.5.0rc. Could you move the whatsnew note to 1.6.0.rst?

pep8speaks · 2022-09-06T21:07:15Z

Hello @smarie! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file pandas/tests/io/parser/test_quoting.py:

Line 43:36: E261 at least two spaces before inline comment

pandas/tests/io/parser/test_quoting.py

smarie · 2022-09-06T21:10:11Z

@mroeschke this should be ok now.

mroeschke · 2022-09-08T15:20:26Z

Thanks @smarie for your patience and persistence! Very nice fix

smarie · 2022-09-08T20:23:43Z

Thanks a lot @mroeschke !
I can not proceed back to #46116

…specific directive is used (pandas-dev#46405) * Added test representative of pandas-dev#46319. Should fail on CI * Added a gha worker with non utf 8 zh_CN encoding * Attempt to fix the encoding so that locale works * Added the fix, but not using it for now, until CI is able to reproduce the issue. * Crazy idea: maybe simply removing the .utf8 modifier will use the right encoding ! * Hopefully fixing the locale not available error * Now simply generating the locale, not updating the ubuntu one * Trying to install the locale without enabling it * Stupid mistake * Testing the optional locale generator condition * Put back all runners * Added whatsnew * Now using the fix * As per code review: moved locale-switching fixture `overridden_locale` to conftest * Flake8 * Added comments on the runner * Added a non-utf8 locale in the `it_IT` runner. Added the zh_CN.utf8 locale in the tests * Improved readability of fixture `overridden_locale` as per code review * Added two comments on default encoding * Fixed pandas-dev#46319 by adding a new `char_to_string_locale` function in the `tslibs.util` module, able to decode char* using the current locale. * As per code review: modified the test to contain non-utf8 chars. Fixed the resulting issue. * Split the test in two for clarity * Fixed test and flake8 error. * Updated whatsnew to ref pandas-dev#46468 . Updated test name * Removing wrong whatsnew bullet * Nitpick on whatsnew as per code review * Fixed build error rst directive * Names incorrectly reverted in last merge commit * Fixed test_localization so that pandas-dev#46595 can be demonstrated on windows targets (even if today these do not run on windows targets, see pandas-dev#46597) * Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used. Fixed pandas-dev#46595 * Removed the fixture as per code review, and added corresponding parametrization in tests. * Dummy mod to trigger CI again * reverted dummy mod * Attempt to fix the remaining error on the numpy worker * Fixed issue in `_from_ordinal` * Added asserts to try to understand * Reverted debugging asserts and applied fix for numpy repeat from pandas-dev#47670. * Fixed the last issue on numpy dev: a TypeError message had changed * Code review: Removed `EXTRA_LOC` * Code review: removed commented line * Code review: reverted out of scope change * Code review: reverted out of scope change * Fixed unused import * Fixed revert mistake * Moved whatsnew to 1.6.0 * Update pandas/tests/io/parser/test_quoting.py Co-authored-by: Sylvain MARIE <[email protected]>

Added test representative of pandas-dev#46319. Should fail on CI

d177852

smarie changed the title ~~Fixed Unicode decoding error in Period.strftime and PeriodIndex.strftime when a locale-specific directive is used~~ Fixed Unicode decoding error in Period.strftime when a locale-specific directive is used Mar 17, 2022

Added a gha worker with non utf 8 zh_CN encoding

c09b76d

Attempt to fix the encoding so that locale works

9dff153

Added the fix, but not using it for now, until CI is able to reproduc…

2fa0736

…e the issue.

jreback requested changes Mar 18, 2022

View reviewed changes

.github/workflows/posix.yml Outdated Show resolved Hide resolved

pandas/tests/io/formats/test_format.py Outdated Show resolved Hide resolved

Sylvain MARIE added 5 commits March 18, 2022 15:45

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

28a0921

…ure/46319_locale_strftime_period � Conflicts: � pandas/tests/io/formats/test_format.py

Crazy idea: maybe simply removing the .utf8 modifier will use the rig…

6b85fb2

…ht encoding !

Hopefully fixing the locale not available error

409d196

Now simply generating the locale, not updating the ubuntu one

4d09db9

Trying to install the locale without enabling it

3c1b80c

Stupid mistake

4c687a8

Sylvain MARIE added 7 commits March 19, 2022 11:04

Testing the optional locale generator condition

e01e051

Put back all runners

a535249

Added whatsnew

b4ad6dd

Now using the fix

0eafd80

As per code review: moved locale-switching fixture `overridden_locale…

b721238

…` to conftest

Flake8

cebed78

Added comments on the runner

a25f3a9

smarie commented Mar 19, 2022

View reviewed changes

pandas/tests/io/formats/test_format.py Outdated Show resolved Hide resolved

smarie commented Mar 19, 2022

View reviewed changes

mroeschke reviewed Mar 19, 2022

View reviewed changes

.github/workflows/posix.yml Outdated Show resolved Hide resolved

mroeschke reviewed Mar 19, 2022

View reviewed changes

mroeschke removed this from the 1.5 milestone Aug 22, 2022

Sylvain MARIE added 6 commits September 3, 2022 14:39

Code review: Removed EXTRA_LOC

4d026cf

Code review: removed commented line

cf3be80

Code review: reverted out of scope change

8f65bd9

Code review: reverted out of scope change

7b414a9

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

cb43fd5

…ure/46319_locale_strftime_period � Conflicts: � .github/workflows/ubuntu.yml

Fixed unused import

59b4fe3

smarie requested a review from mroeschke September 6, 2022 06:50

mroeschke reviewed Sep 6, 2022

View reviewed changes

pandas/tests/io/parser/test_quoting.py Outdated Show resolved Hide resolved

Sylvain MARIE added 2 commits September 6, 2022 23:03

Fixed revert mistake

4950e06

Moved whatsnew to 1.6.0

14f854b

smarie commented Sep 6, 2022

View reviewed changes

pandas/tests/io/parser/test_quoting.py Outdated Show resolved Hide resolved

Update pandas/tests/io/parser/test_quoting.py

90c31cb

Merge branch 'main' into feature/46319_locale_strftime_period

5d467e2

smarie requested a review from mroeschke September 8, 2022 09:17

mroeschke approved these changes Sep 8, 2022

View reviewed changes

mroeschke removed the Stale label Sep 8, 2022

mroeschke added this to the 1.6 milestone Sep 8, 2022

mroeschke merged commit ae6dc97 into pandas-dev:main Sep 8, 2022

smarie deleted the feature/46319_locale_strftime_period branch September 8, 2022 20:23

mroeschke modified the milestones: 1.6, 2.0 Oct 13, 2022

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 Unicode decoding error in `Period.strftime` when a locale-specific directive is used #46405

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

smarie commented Mar 17, 2022 •

edited

Loading

smarie commented Mar 17, 2022

smarie commented Mar 17, 2022

smarie commented Mar 17, 2022 •

edited

Loading

smarie commented Mar 17, 2022 •

edited

Loading

smarie commented Mar 18, 2022

smarie commented Mar 19, 2022

smarie commented Mar 19, 2022

smarie Mar 19, 2022 •

edited

Loading

mroeschke Mar 19, 2022

smarie Mar 19, 2022 •

edited

Loading

smarie Mar 20, 2022

smarie Mar 21, 2022

mroeschke Mar 19, 2022

smarie Mar 19, 2022 •

edited

Loading

mroeschke Mar 19, 2022

smarie Mar 20, 2022 •

edited

Loading

mroeschke Mar 26, 2022

smarie Apr 1, 2022 •

edited

Loading

smarie commented Sep 4, 2022 •

edited

Loading

mroeschke commented Sep 6, 2022

pep8speaks commented Sep 6, 2022

smarie commented Sep 6, 2022

mroeschke commented Sep 8, 2022

smarie commented Sep 8, 2022

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

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

Conversation

smarie commented Mar 17, 2022 • edited Loading

smarie commented Mar 17, 2022

smarie commented Mar 17, 2022

smarie commented Mar 17, 2022 • edited Loading

smarie commented Mar 17, 2022 • edited Loading

smarie commented Mar 18, 2022

smarie commented Mar 19, 2022

smarie commented Mar 19, 2022

smarie Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

mroeschke Mar 19, 2022

Choose a reason for hiding this comment

smarie Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

smarie Mar 20, 2022

Choose a reason for hiding this comment

smarie Mar 21, 2022

Choose a reason for hiding this comment

mroeschke Mar 19, 2022

Choose a reason for hiding this comment

smarie Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

mroeschke Mar 19, 2022

Choose a reason for hiding this comment

smarie Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

mroeschke Mar 26, 2022

Choose a reason for hiding this comment

smarie Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

smarie commented Sep 4, 2022 • edited Loading

mroeschke commented Sep 6, 2022

pep8speaks commented Sep 6, 2022

smarie commented Sep 6, 2022

mroeschke commented Sep 8, 2022

smarie commented Sep 8, 2022

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

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

smarie commented Mar 17, 2022 •

edited

Loading

smarie commented Mar 17, 2022 •

edited

Loading

smarie commented Mar 17, 2022 •

edited

Loading

smarie Mar 19, 2022 •

edited

Loading

smarie Mar 19, 2022 •

edited

Loading

smarie Mar 19, 2022 •

edited

Loading

smarie Mar 20, 2022 •

edited

Loading

smarie Apr 1, 2022 •

edited

Loading

smarie commented Sep 4, 2022 •

edited

Loading