Skip to content

BLD: Try installing older Cython for windows free threading build #61249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 9, 2025

Conversation

mroeschke
Copy link
Member

@mroeschke mroeschke added the Build Library building on various platforms label Apr 7, 2025
@mroeschke mroeschke added this to the 3.0 milestone Apr 7, 2025
@mroeschke
Copy link
Member Author

Note: Older ninja or numpy failures versions are exhibiting the same failures. I suspect a Cython change might be the culprit

@mroeschke mroeschke changed the title BLD: Try pinning ninja<1.11.1.4 for windows free threading build BLD: Try installing older Cython for windows free threading build Apr 8, 2025
@WillAyd
Copy link
Member

WillAyd commented Apr 8, 2025

The traceback showing up in the test suite is suspicious:

2025-04-08T19:28:28.2539512Z Traceback (most recent call last):
2025-04-08T19:28:28.2554584Z   File "pandas/_libs/tslibs/tzconversion.pyx", line 128, in pandas._libs.tslibs.tzconversion.Localizer.utc_val_to_local_val
2025-04-08T19:28:28.2555644Z   File "pandas/_libs/tslibs/tzconversion.pyx", line 759, in pandas._libs.tslibs.tzconversion._tz_localize_using_tzinfo_api
2025-04-08T19:28:28.2556397Z   File "pandas/_libs/tslibs/tzconversion.pyx", line 791, in pandas._libs.tslibs.tzconversion._astimezone
2025-04-08T19:28:28.2557288Z   File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-t5af6k12\cp313t-win_amd64\venv-test\Lib\site-packages\dateutil\tz\_common.py", line 144, in fromutc
2025-04-08T19:28:28.2557899Z     return f(self, dt)
2025-04-08T19:28:28.2558668Z   File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-t5af6k12\cp313t-win_amd64\venv-test\Lib\site-packages\dateutil\tz\_common.py", line 261, in fromutc
2025-04-08T19:28:28.2559658Z     _fold = self._fold_status(dt, dt_wall)
2025-04-08T19:28:28.2560385Z   File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-t5af6k12\cp313t-win_amd64\venv-test\Lib\site-packages\dateutil\tz\_common.py", line 196, in _fold_status
2025-04-08T19:28:28.2561049Z     if self.is_ambiguous(dt_wall):
2025-04-08T19:28:28.2561335Z        ~~~~~~~~~~~~~~~~~^^^^^^^^^
2025-04-08T19:28:28.2561988Z   File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-t5af6k12\cp313t-win_amd64\venv-test\Lib\site-packages\dateutil\tz\tz.py", line 254, in is_ambiguous
2025-04-08T19:28:28.2562646Z     naive_dst = self._naive_is_dst(dt)
2025-04-08T19:28:28.2563346Z   File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-t5af6k12\cp313t-win_amd64\venv-test\Lib\site-packages\dateutil\tz\tz.py", line 260, in _naive_is_dst
2025-04-08T19:28:28.2564061Z     return time.localtime(timestamp + time.timezone).tm_isdst
2025-04-08T19:28:28.2564432Z            ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
2025-04-08T19:28:28.2564754Z OSError: [Errno 22] Invalid argument

I wonder if this is an issue with the container itself and not necessarily any of the Python packages

@mroeschke
Copy link
Member Author

The traceback showing up in the test suite is suspicious:

That traceback goes back to this dateutil issue: dateutil/dateutil#197

@WillAyd
Copy link
Member

WillAyd commented Apr 8, 2025

Hmm possibly - but wouldn't that show up on the other Windows builds?

My other guess would be a bug in CPython - the datetime modules have historically used a global singleton for storing datetime state. On the free-threaded build that may be a likely issue area (assuming it hasn't changed upstream - I have not looked at all)

@mroeschke
Copy link
Member Author

Hmm possibly - but wouldn't that show up on the other Windows builds?

Yup, we do e.g. https://github.com/pandas-dev/pandas/actions/runs/14319739950/job/40133986072 but they get swallowed somewhere

My poor-man's git bisect here I think is pointing to a Cython bug (5016bf7 shows that an earlier Cython commit does not fail the Python 3.13t Windows tests). My uneducated guess might be due to cython/cython#6726

@WillAyd
Copy link
Member

WillAyd commented Apr 8, 2025

I also found this note in CPython about needing to define Py_GIL_DISABLED=1 for Windows to work with free threaded builds:

https://github.com/python/cpython/blob/main/Doc/howto/free-threading-extensions.rst#windows

Certainly possible I am overlooking, but I don't see that anywhere in our current setup. Might be worth adding:

add_project_arguments('-DPy_GIL_DISABLED', language : 'c')

To our current Meson configuration

@WillAyd
Copy link
Member

WillAyd commented Apr 8, 2025

My poor-man's git bisect here I think is pointing to a Cython bug (5016bf7 shows that an earlier Cython commit does not fail the Python 3.13t Windows tests). My uneducated guess might be due to cython/cython#6726

Ah OK cool. Ignore what I said then - nice find

@mroeschke
Copy link
Member Author

I also found this note in CPython about needing to define Py_GIL_DISABLED=1 for Windows to work with free threaded builds:

Ah that would be good to add. I think I saw some warnings in the logs about some free threading option not being set, not sure if it was Windows specific

@WillAyd
Copy link
Member

WillAyd commented Apr 8, 2025

It is possible that Meson already sets it for us in Windows. One way to check would be to add -Ccompile-args="-v" to get the verbose output of each compilation step

@mroeschke
Copy link
Member Author

I think I've narrowed it down to this Cython change cython/cython#6717

@WillAyd
Copy link
Member

WillAyd commented Apr 9, 2025

Wow nice find. From an initial glance that seems really tangential - I wonder how we can form an MRE out of that for a bug report (just thinking out loud - haven't looked deeply)

@mroeschke
Copy link
Member Author

Going to merge as the Windows Python 3.13t builds are passing now.

@mroeschke mroeschke merged commit d1c6404 into pandas-dev:main Apr 9, 2025
94 checks passed
@mroeschke mroeschke deleted the bld/ninja/windows branch April 9, 2025 17:56
Copy link

lumberbot-app bot commented Apr 9, 2025

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 2.3.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 d1c64045921d7f5b4fe0609b5bc428219c279e5e
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #61249: BLD: Try installing older Cython for windows free threading build'
  1. Push to a named branch:
git push YOURFORK 2.3.x:auto-backport-of-pr-61249-on-2.3.x
  1. Create a PR against branch 2.3.x, I would have named this PR:

"Backport PR #61249 on branch 2.3.x (BLD: Try installing older Cython for windows free threading build)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@mroeschke
Copy link
Member Author

I think it might be easier if I manually do this backport as opposed to cherry picking this commit

@lithomas1
Copy link
Member

Wow, really nice debugging, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Windows free-threaded wheel available in scientific-python-nightly-wheels
3 participants