CI: Fix npdev build post Cython annotation change #47670

mroeschke · 2022-07-11T18:33:03Z

Tests added and passed if fixing a bug or adding a new feature
All code checks passed.

Appears the new build failure was due to cython/cython#4885

Major changes:

Period._from_ordinal is annotated def _from_ordinal(oridinal: int, freq) which now strictly only accepts Python ints. Need to change to def _from_ordinal(oridinal: int64_t, freq)

mroeschke · 2022-07-11T20:59:11Z

cc @jbrockmendel could use your insight into another failure.

e.g. np.repeat(EA, 2) is returning an ndarray instead of an EA. What code path would that go through?

jbrockmendel · 2022-07-11T21:27:23Z

Period._from_ordinal is annotated def _from_ordinal(oridinal: int, freq), so ordinal needs to always be a Python int and not a np.int64

We end up casting to int64_t, so this casting feels wasteful. Could we just make the annotation int64_t?

jbrockmendel · 2022-07-11T21:33:23Z

e.g. np.repeat(EA, 2) is returning an ndarray instead of an EA. What code path would that go through?

np.repeat uses array_function_dispatch, which I think means it checks for __array_function__, which we don't implement anywhere. Without __array_function__, it isn't that surprising that it returns an ndarray. If anything I'm surprised it used to (?).

mroeschke · 2022-07-11T21:50:30Z

Found it:

We define def repeat(self, repeats, axis: int = 0) which np.repeat dispatches to

https://github.com/numpy/numpy/blob/54c52f13713f3d21795926ca4dbb27e16fada171/numpy/core/fromnumeric.py#L52-L66=

Since axis can be a np.int64 it hit's numpy's except TypeError block which probably caused the different behavior.

Looks like we need to be careful with annotating cython code with Python types as they will be strict now and raise a TypeError otherwise.

jbrockmendel · 2022-07-11T21:56:30Z

Since axis can be a np.int64 it hit's numpy's except TypeError block which probably caused the different behavior

Sure its int64 and not intp?

mroeschke · 2022-07-11T21:58:26Z

Since axis can be a np.int64 it hit's numpy's except TypeError block which probably caused the different behavior

Sure its int64 and not intp?

Hmm good point. intp is probably safer here

mroeschke · 2022-07-12T16:42:29Z

I decided to remove the : int annotation for repeat(..., axis: int). intp_t was failing in cython 0.29.x. We can revist the tying in a followup

…as-dev#47670.

smarie · 2022-07-13T12:38:23Z

Since axis can be a np.int64 it hit's numpy's except TypeError block which probably caused the different behavior.

Thanks @mroeschke for finding this ! I can stop investigating in #46405 :)

mroeschke · 2022-07-13T16:58:00Z

Np @smarie, sorry I didn't realized you were investigating it in tandem.

Does it look okay to you @jbrockmendel?

jbrockmendel · 2022-07-13T18:04:16Z

no objections

jbrockmendel · 2022-07-13T18:05:46Z

pandas/_libs/arrays.pyx

@@ -157,7 +157,7 @@ cdef class NDArrayBacked:
        return self._from_backing_data(res_values)

    # TODO: pass NPY_MAXDIMS equiv to axis=None?
-    def repeat(self, repeats, axis: int = 0):
+    def repeat(self, repeats, axis = 0):


what happens if we make this int | np.integer?

This looks to have worked. Changed

mroeschke · 2022-07-13T20:58:45Z

Merging to get the CI to green. Can followup if needed

…ion change

…nnotation change) (#47709) Backport PR #47670: CI: Fix npdev build post Cython annotation change Co-authored-by: Matthew Roeschke <[email protected]>

…specific directive is used (#46405) * Added test representative of #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 #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 #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 #46595 can be demonstrated on windows targets (even if today these do not run on windows targets, see #46597) * Fixed `tm.set_locale` context manager, it could error and leak when category LC_ALL was used. Fixed #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 #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]>

…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]>

mroeschke added 2 commits July 11, 2022 11:32

CI: Fix npdev build post Cython annotation change

d45d716

Ensure _from_ordinal gets int

5779104

Type as int64_t instead

2c4dec4

change another int typing

efb7e02

change another int typing

7682b42

mroeschke added 3 commits July 11, 2022 15:01

Change to intp

e2df478

Don't type?

f76226d

Undo import

0080dbf

mroeschke added CI Continuous Integration Compat pandas objects compatability with Numpy or Python functions labels Jul 12, 2022

mroeschke added this to the 1.4.4 milestone Jul 12, 2022

smarie mentioned this pull request Jul 13, 2022

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

Merged

4 tasks

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

Reverted debugging asserts and applied fix for numpy repeat from pand…

e5bca9f

…as-dev#47670.

jbrockmendel reviewed Jul 13, 2022

View reviewed changes

test or typing

0a61db7

mroeschke merged commit 13e859f into pandas-dev:main Jul 13, 2022

mroeschke deleted the ci/np_dev_cython_fix branch July 13, 2022 20:58

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

Backport PR pandas-dev#47670: CI: Fix npdev build post Cython annotat…

79ee7b0

…ion change

meeseeksmachine mentioned this pull request Jul 13, 2022

Backport PR #47670 on branch 1.4.x (CI: Fix npdev build post Cython annotation change) #47709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Fix npdev build post Cython annotation change #47670

CI: Fix npdev build post Cython annotation change #47670

mroeschke commented Jul 11, 2022 •

edited

Loading

mroeschke commented Jul 11, 2022

jbrockmendel commented Jul 11, 2022

jbrockmendel commented Jul 11, 2022

mroeschke commented Jul 11, 2022

jbrockmendel commented Jul 11, 2022

mroeschke commented Jul 11, 2022

mroeschke commented Jul 12, 2022

smarie commented Jul 13, 2022

mroeschke commented Jul 13, 2022

jbrockmendel commented Jul 13, 2022

jbrockmendel Jul 13, 2022

mroeschke Jul 13, 2022

mroeschke commented Jul 13, 2022

CI: Fix npdev build post Cython annotation change #47670

CI: Fix npdev build post Cython annotation change #47670

Conversation

mroeschke commented Jul 11, 2022 • edited Loading

mroeschke commented Jul 11, 2022

jbrockmendel commented Jul 11, 2022

jbrockmendel commented Jul 11, 2022

mroeschke commented Jul 11, 2022

jbrockmendel commented Jul 11, 2022

mroeschke commented Jul 11, 2022

mroeschke commented Jul 12, 2022

smarie commented Jul 13, 2022

mroeschke commented Jul 13, 2022

jbrockmendel commented Jul 13, 2022

jbrockmendel Jul 13, 2022

Choose a reason for hiding this comment

mroeschke Jul 13, 2022

Choose a reason for hiding this comment

mroeschke commented Jul 13, 2022

mroeschke commented Jul 11, 2022 •

edited

Loading