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

Updates the release workflow to support python 3.11 and MacOS arm64. #338

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

cheqianh
Copy link
Contributor

@cheqianh cheqianh commented Feb 13, 2024

Description

This PR adds support for wheels on Python 3.11 and MacOS-arm64.

In detail, this PR:

  1. Adds wheels for Python 3.11.
  2. Adds wheels for MacOS-arm64.
  3. Drops support for wheels built with Python 3.6 to 3.8.
  4. Updates required dependencies and build tools.

Test

  1. A successful wheel build workflow from my fork can be found at: https://github.com/cheqianh/ion-python/actions/runs/7880967937.
  2. All tests have passed on GitHub Actions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cheqianh cheqianh marked this pull request as draft February 13, 2024 02:48
@cheqianh cheqianh marked this pull request as ready for review February 13, 2024 03:11
Copy link
Contributor

@rmarrowstone rmarrowstone left a comment

Choose a reason for hiding this comment

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

I realize that we were already using 3.10 for the python version for building the wheels, but let's make sure we understand what's going on there.

Comment on lines 110 to 114
- name: Set up Python 3.10
uses: actions/setup-python@v2
with:
python-version: '3.10'

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the relationship between this python 3.10 setup step and the built wheels? Is 3.10 used to build wheels for all the versions? Are we only building wheels for 3.10?

Copy link
Contributor

Choose a reason for hiding this comment

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

And why not 3.11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This python version will be used for setting up virtual environment and install dependencies including cibuildwheel. We used 3.10 back then because cibuildwheel=2.4.0 only supported wheels for python versions up 3.10 so I used the same version as it's the latest version of python we supported to build wheels.

To avoid the confusion, I updated the PR to use general python3 command to let tasks use the default python versions that these images are using. For example, MacOs 14 is using python 3.11 and Ubuntu-latest is using python 3.10. But both versions are tested in our main test workflow.

Copy link
Contributor Author

@cheqianh cheqianh Feb 13, 2024

Choose a reason for hiding this comment

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

A successful workflow test without python 3.10 setup is available in my fork - https://github.com/cheqianh/ion-python/actions/runs/7891635687

Copy link
Contributor

Choose a reason for hiding this comment

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

Well that still doesn't really answer the question. I dug into the docs and the answer is that the CIBW flags control which architectures and python versions one builds for. I added a comment to the current skip setup because I think we need to be explicit about we include not exclude there. It would also make it clearer what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will include this change in the next commit.

Copy link
Contributor Author

@cheqianh cheqianh Feb 13, 2024

Choose a reason for hiding this comment

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

Is 3.10 used to build wheels for all the versions?

No, cibuildwheel will build wheels using different python versions respectively

Are we only building wheels for 3.10?

Nope.

CIBW_ARCHS_WINDOWS: "auto"
CIBW_ARCHS_LINUX: "auto aarch64"
CIBW_ARCHS_MACOS: "x86_64"
CIBW_SKIP: "pp* *-win32 cp36-* cp37-* cp38-*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer explicit including instead of accepting the cibuildwheel defaults and skipping some. That way we know that we are only building for arch/python version combinations that we have tested for and it won't change with cibuildwheel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a comment about what these apparently arcane flags mean for the next dev that has to change this.

Copy link
Contributor Author

@cheqianh cheqianh Feb 13, 2024

Choose a reason for hiding this comment

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

Added a commit to

  1. Use the specific arch names instead of alias "auto",
  2. Use the specific python versions 3.9 to 3.11
  3. Add a link to the cibuildwheel flag document

I tried to add python 3.12 but the build failed due to a dependency issue. I'll open a new issue to track the 3.12 build issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a new issue for adding 3.12 to the build workflow - #339

CIBW_ARCHS_WINDOWS: "auto"
CIBW_ARCHS_LINUX: "auto aarch64"
CIBW_ARCHS_MACOS: "x86_64"
CIBW_BUILD: "cp39-* cp310-* cp311-*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment. It is really non-obvious that "cp39" means "C-Python 3.9".

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean... until it is, of course! ;-)

@cheqianh cheqianh merged commit 193e86b into amazon-ion:master Feb 14, 2024
18 checks passed
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.

3 participants