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

ci(python): Add cibuildwheel setup for Python wheels #353

Merged
merged 35 commits into from
Jan 10, 2024

Conversation

paleolimbot
Copy link
Member

@paleolimbot paleolimbot commented Jan 5, 2024

This PR adds a weekly/workflow dispatch job for building and testing Python wheels. This required a few housekeeping items:

  • Versioning the python package. I used the approach from ADBC, which is a modified 'miniver'. Basically, just set the version as a string using a regex replace when needed.
  • The bootstrap.py logic was updated to use a proper temporary directory
  • Tests were updated to skip instead of fail when pyarrow/numpy are not available (because I can never remember which platforms they will or won't install on and the default cibuildwheel grid is large).
  • I hadn't tested install from sdist, so a few files were missing from the manifest.

Two outstanding issues that don't necessarily need to be solved in this PR:

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (38b5201) 88.08% compared to head (540999b) 87.20%.
Report is 5 commits behind head on main.

Files Patch % Lines
python/src/nanoarrow/_static_version.py 0.00% 3 Missing ⚠️
python/src/nanoarrow/_version.py 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   88.08%   87.20%   -0.89%     
==========================================
  Files          72        6      -66     
  Lines       11677      125   -11552     
==========================================
- Hits        10286      109   -10177     
+ Misses       1391       16    -1375     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paleolimbot paleolimbot marked this pull request as ready for review January 6, 2024 19:17
Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workflow looks good rest also looks sensible but my python is mid ^^

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

I added a whole bunch of comments, but many are also small nitpicks that are not crucial at all

.github/workflows/python-wheels.yaml Show resolved Hide resolved

# Build wheels weekly or when requested
on:
pull_request:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also run in on every commit in main? (I agree it's certainly good to not let it run on every commit in every PR)

  push:
    branches:
      - main

.github/workflows/python-wheels.yaml Outdated Show resolved Hide resolved
.github/workflows/python-wheels.yaml Show resolved Hide resolved
.github/workflows/python-wheels.yaml Outdated Show resolved Hide resolved
- {os: "windows-2019", label: "windows"}
- {os: "macOS-11", label: "macOS"}
# Uncomment when apache/arrow-nanoarrow is added to the arrow runner group
# - {os: ["self-hosted", "arm"], label: "linux/arm64"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, we can build MacOS arm64 wheels here on github actions if we want (just not test them), that's what we do in other projects I am involved with

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, that's already happening (because you defined CIBW_ARCHS_MACOS to include both, I suppose)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also try emulation on github actions here for linux arm64 (given it's a small package, that might be possible and not too slow). Then we wouldn't need any other self-hosted runner or different CI platform.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took a second to figure it out, but it seems like the arm64 runner works!

Comment on lines +80 to +85
- name: Install cibuildwheel
run: python -m pip install cibuildwheel==2.15.0

- name: Build wheels
run: |
python -m cibuildwheel --output-dir wheelhouse python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Install cibuildwheel
run: python -m pip install cibuildwheel==2.15.0
- name: Build wheels
run: |
python -m cibuildwheel --output-dir wheelhouse python
- name: Build wheels
uses: pypa/cibuildwheel@v2.16.2

I am not sure why the cibuildwheel README shows the manual way, but the docs recommend using the action (https://cibuildwheel.readthedocs.io/en/stable/setup/#github-actions)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert to the previous version because of the arm self-hosted runner (for which the stock install-python action won't work).

.github/workflows/python-wheels.yaml Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@ repository = "https://github.com/apache/arrow-nanoarrow"
[build-system]
requires = [
"setuptools >= 61.0.0",
"setuptools-scm",
"versioneer[toml]==0.29",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed if you use the copied miniver helper?

paleolimbot and others added 9 commits January 10, 2024 09:52
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@paleolimbot paleolimbot merged commit b636a8f into apache:main Jan 10, 2024
11 checks passed
@paleolimbot paleolimbot deleted the python-cibuildwheel branch January 10, 2024 20:17
@paleolimbot paleolimbot added this to the nanoarrow 0.4.0 milestone Jan 26, 2024
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