Skip to content
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

GH-43964: [Python] Build macOS and manylinux wheels for free-threading #43965

Merged
merged 19 commits into from
Sep 18, 2024

Conversation

lysnikolaou
Copy link
Contributor

@lysnikolaou lysnikolaou commented Sep 5, 2024

Rationale for this change

Building free-threaded wheels is necessary to support the free-threaded build. We probably want to upload these wheels as nightlies somewhere as well, so that downstream users can test the free-threading-related changes.

What changes are included in this PR?

  • Add necessary configuration to build 3.13 free-threading wheels on manylinux.
  • Do necessary changes to build free-threaded wheels on macOS as well.

Are these changes tested?

I tested the manylinux wheel builds. macOS is still untested, since it's not dockerized.

Are there any user-facing changes?

No.

Related to #43536.

Copy link

github-actions bot commented Sep 5, 2024

⚠️ GitHub issue #43964 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting review Awaiting review label Sep 5, 2024
@jorisvandenbossche
Copy link
Member

We probably want to upload these wheels as nightlies somewhere as well, so that downstream users can test the free-threading-related changes.

We are uploading the nightly wheels the scientific-python-nightly-wheels index like other package do (since yesterday, and we still have our own index on gemfury as well), so this should happen automatically

@jorisvandenbossche
Copy link
Member

@github-actions crossbow submit -g wheel

This comment was marked as outdated.

@jorisvandenbossche
Copy link
Member

For linux (where it already succeeds to the testing step), we will also need to get a 3.13t build of the test image (python-wheel-manylinux-test-unittests)

@lysnikolaou
Copy link
Contributor Author

For linux (where it already succeeds to the testing step), we will also need to get a 3.13t build of the test image (python-wheel-manylinux-test-unittests)

Yeah, I'm working on that as well. My understanding was that both python-wheel-manylinux-test-unittests and python-wheel-manylinux-test-imports need some changes, but they both depend on the Docker official Python images, which do not have a free-threaded variant.

It might make sense for me to work on that, too, but for now, I'm trying to see how to best circumvent that. Maybe start with an ubuntu images and install a Python from deadsnakes?

@jorisvandenbossche
Copy link
Member

Maybe start with an ubuntu images and install a Python from deadsnakes?

I assume it could be similar as the one you are adding for #43671? (that's also using deadsnakes I think)

@lysnikolaou
Copy link
Contributor Author

That's one option. To basically add a new service that installs python3.13-nogil and then uses that to test the wheels, however that would mean that we have to call different test services depending on the python version in github.linux.yml. I was trying to avoid that by adjusting the already existing services, but it might be too complicated for no good value.

@lysnikolaou
Copy link
Contributor Author

@jorisvandenbossche Could you rerun the wheel building jobs?

@jorisvandenbossche
Copy link
Member

@github-actions crossbow submit -g wheel

This comment was marked as outdated.

@lysnikolaou
Copy link
Contributor Author

Not sure why the install_python script fails. The failure seems like the $PYTHON environment variable is not defined, but I'm pretty sure it is defined in github.osx.yml. I've pushed a commit that falls back to explicitly declaring the $python bash variable inside the script.

Let's please rerun the jobs. (maybe only the macos ones?)

@raulcd
Copy link
Member

raulcd commented Sep 9, 2024

@github-actions crossbow submit wheel-macos-*

This comment was marked as outdated.

@lysnikolaou
Copy link
Contributor Author

And another wheel-macos-monterey-cp313-cp313t-* might be needed. Sorry for the back and forth, but this is difficult to test locally, cause it might mess up my Python installation.

@raulcd
Copy link
Member

raulcd commented Sep 9, 2024

Sorry for the back and forth, but this is difficult to test locally, cause it might mess up my Python installation.

No worries at all, I understand the need of testing back and forth on CI

@raulcd
Copy link
Member

raulcd commented Sep 9, 2024

@github-actions crossbow submit wheel-macos-monterey-cp313-cp313t-*

Copy link

github-actions bot commented Sep 9, 2024

Revision: 7fd6c08

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d24c7a697

Task Status
wheel-macos-monterey-cp313-cp313t-amd64 GitHub Actions
wheel-macos-monterey-cp313-cp313t-arm64 GitHub Actions

@jorisvandenbossche
Copy link
Member

Based on the wheel file name in the upload step in the arm64 build, it seems it tries to upload a non-free-threaded cp313 wheel (instead of cp313t), but a normal one. Although it certainly did build the cp313t wheel (maybe that is related to why it is failing to upload?)

There is also still a warning about cffi enabling the GIL, so there needs to be a PYTHON_GIL=0 somewhere?

@lysnikolaou
Copy link
Contributor Author

lysnikolaou commented Sep 17, 2024

I see this error on a regular Python 3.13 wheel test:

Fatal Python error: config_read_gil: Disabling the GIL is not supported by this build
Python runtime state: preinitialized

https://github.com/ursacomputing/crossbow/actions/runs/10906089565/job/30266520154#step:17:39

Hmm, good catch. Fixed, along with removing --no-build-isolation from the macOS job now that pandas wheels for macOS are back up.

@pitrou
Copy link
Member

pitrou commented Sep 17, 2024

@github-actions crossbow submit wheelcp312 wheelcp313

@pitrou
Copy link
Member

pitrou commented Sep 17, 2024

@github-actions crossbow submit -g wheel

@lysnikolaou
Copy link
Contributor Author

crossbow submit wheel_cp312_ wheel_cp313_

Do you see the statuses from this somewhere?

This comment was marked as outdated.

@pitrou
Copy link
Member

pitrou commented Sep 17, 2024

Do you see the statuses from this somewhere?

Well, it took some time, but they've just appeared above :-)

This comment was marked as outdated.

@lysnikolaou
Copy link
Contributor Author

lysnikolaou commented Sep 17, 2024

Turns out the following construct was indeed needed in github.linux.yml:

if: |
  '{{ python_abi_tag }}' == 'cp313t'

Otherwise a syntax error is reported. Probably something to do with how Github evaluates expressions.

@pitrou
Copy link
Member

pitrou commented Sep 17, 2024

Ahah! Nice find, and sorry for the disruption.

@pitrou
Copy link
Member

pitrou commented Sep 17, 2024

@github-actions crossbow submit wheellinux

This comment was marked as outdated.

@pitrou
Copy link
Member

pitrou commented Sep 18, 2024

@github-actions crossbow submit wheel-macos-* wheellinux

@pitrou
Copy link
Member

pitrou commented Sep 18, 2024

@lysnikolaou Do you also plan to submit a PR for Windows wheels?

Copy link

Revision: b966c78

Submitted crossbow builds: ursacomputing/crossbow @ actions-70086a21bd

Task Status
wheel-macos-monterey-cp310-cp310-amd64 GitHub Actions
wheel-macos-monterey-cp310-cp310-arm64 GitHub Actions
wheel-macos-monterey-cp311-cp311-amd64 GitHub Actions
wheel-macos-monterey-cp311-cp311-arm64 GitHub Actions
wheel-macos-monterey-cp312-cp312-amd64 GitHub Actions
wheel-macos-monterey-cp312-cp312-arm64 GitHub Actions
wheel-macos-monterey-cp313-cp313-amd64 GitHub Actions
wheel-macos-monterey-cp313-cp313-arm64 GitHub Actions
wheel-macos-monterey-cp313-cp313t-amd64 GitHub Actions
wheel-macos-monterey-cp313-cp313t-arm64 GitHub Actions
wheel-macos-monterey-cp39-cp39-amd64 GitHub Actions
wheel-macos-monterey-cp39-cp39-arm64 GitHub Actions
wheel-manylinux-2-28-cp310-cp310-amd64 GitHub Actions
wheel-manylinux-2-28-cp310-cp310-arm64 GitHub Actions
wheel-manylinux-2-28-cp311-cp311-amd64 GitHub Actions
wheel-manylinux-2-28-cp311-cp311-arm64 GitHub Actions
wheel-manylinux-2-28-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2-28-cp312-cp312-arm64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313-arm64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-arm64 GitHub Actions
wheel-manylinux-2-28-cp39-cp39-amd64 GitHub Actions
wheel-manylinux-2-28-cp39-cp39-arm64 GitHub Actions
wheel-manylinux-2014-cp310-cp310-amd64 GitHub Actions
wheel-manylinux-2014-cp310-cp310-arm64 GitHub Actions
wheel-manylinux-2014-cp311-cp311-amd64 GitHub Actions
wheel-manylinux-2014-cp311-cp311-arm64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-amd64 GitHub Actions
wheel-manylinux-2014-cp312-cp312-arm64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313-arm64 GitHub Actions
wheel-manylinux-2014-cp313-cp313t-amd64 GitHub Actions
wheel-manylinux-2014-cp313-cp313t-arm64 GitHub Actions
wheel-manylinux-2014-cp39-cp39-amd64 GitHub Actions
wheel-manylinux-2014-cp39-cp39-arm64 GitHub Actions

@lysnikolaou
Copy link
Contributor Author

@lysnikolaou Do you also plan to submit a PR for Windows wheels?

Yes, I was planning to do so, as soon as NumPy has a Windows wheel for the cp313t ABI.

@pitrou
Copy link
Member

pitrou commented Sep 18, 2024

It seems that compiling Pandas' dev version on arm64 can sometimes fail:
https://github.com/ursacomputing/crossbow/actions/runs/10918109629/job/30302833788#step:11:2259

Hopefully they will soon produce wheels? @lysnikolaou

@lysnikolaou
Copy link
Contributor Author

I'm not sure exactly what the plan is about that, but there's definitely challenges to producing aarch64 wheels. NumPy produces its aarch64 wheels on CirrusCI, because Github does not provide aarch64 Linux runners to non-entrprise accounts. However, they've indicated that they'll do so by the end of the year, so maybe it makes sense for pandas to wait until then and not introduce another CI service. Either way, I'll open an upstream issue about it.

@pitrou
Copy link
Member

pitrou commented Sep 18, 2024

Ok, hopefully it won't break too often then...

@pitrou
Copy link
Member

pitrou commented Sep 18, 2024

I'm going to merge now.

@pitrou pitrou merged commit 1c1f7f3 into apache:main Sep 18, 2024
60 of 61 checks passed
@pitrou pitrou removed the awaiting change review Awaiting change review label Sep 18, 2024
@lysnikolaou lysnikolaou deleted the cp313t-wheel-build branch September 18, 2024 09:10
@jorisvandenbossche
Copy link
Member

It seems that compiling Pandas' dev version on arm64 can sometimes fail:

We could potentially also skip installing pandas when testing the arm64 cp313t wheels, if it turns out to be unstable (given it's not a required test dependency)

Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 1c1f7f3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Successfully merging this pull request may close these issues.

4 participants