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

Adapt CI/CD for python pgenlib to test against numpy 2.0 #272

Merged
merged 94 commits into from
Jun 19, 2024
Merged

Adapt CI/CD for python pgenlib to test against numpy 2.0 #272

merged 94 commits into from
Jun 19, 2024

Conversation

aryarm
Copy link
Contributor

@aryarm aryarm commented Jun 17, 2024

This PR builds upon the work in #262 and proposes changes to both workflows that were created there. It also adds support for numpy 2.0 to pgenlib, and it tests pgenlib with numpy 2.0 using the updated workflows

@pettyalex, if you have a chance, I would greatly appreciate your feedback! This was my first time working with cibuildwheel, so I tried to use the pysam wheel-building workflow as an example

For the wheel-building workflow

  1. The main change is that the sdist-building step gets moved to a different job in the workflow, since we only need to build it on a single platform once
  2. I also split each architecture and python version into its own actions runner to parallelize the entire wheel-building process and speed things up. It should only take about 15 minutes to build all of the wheels now
  3. I removed Windows wheel builds because I couldn't get them to work
  4. Rather than build separate arm64 and x86_64 wheels for MacOS, I opted to build a single universal2 wheel which should work on both platforms. This slightly speeds up the wheel-building process and might make things a bit simpler. To install wheels on MacOS, users will need Pip 20.3+, so I've updated the README to mention that
  5. I added a new step at the end of the workflow which allows for automatically uploading the sdist and wheels to PyPI when the workflow is executed on the master branch of the original repository (and not a fork). @chrchang will need to set up trusted publishing (with an empty environment name) for this to work
  6. Finally, I added integrated testing of the sdist and each wheel on each platform via cibuildwheel's CIBW_TEST_COMMAND. This allowed me to test that wheels built against numpy 2.0 work on a variety of architectures

This workflow must be executed manually. You can go to the "Actions" tab, pick the "Release pgenlib" workflow, and click on the "Run workflow" button.
image

For the CI workflow

Since GitHub action runners are free for public repositories, I decided to expand testing to a variety of python versions and OSs. The new CI workflow runs pytest with wheels on ubuntu, macOS x86_64, and macOS arm64 using python versions 3.7-3.12. It also tests the sdist on python 3.8 (since that's the oldest actively maintained version) and 3.12

I also created a step in the workflow that tests pgenlib against numpy 2.0 per the advice in the numpy2 transition documentation

This workflow is triggered automatically whenever a change is made to code in the 2.0/Python directory. That way, developers of pgenlib will be able to test that their changes aren't breaking the code when they fork and open pull requests back into the original repository.

Regarding the dependency on numpy

This PR adds an explicit (runtime) dependency on numpy to the install_requires section of the setup.py file. That way, users won't need to manually install numpy; it will be installed automatically with the package. This also allows for automatic installation of numpy in the CI/CD steps. In my testing, I found that numpy 1.21.0 was the oldest version that worked on all platforms, so I updated the dev-requirements.txt file and setup.py file, accordingly.

As I mentioned above, pgenlib is no longer compatible with numpy upon the release of 2.0 on June 16. To fix this, I added numpy 2.0 as a build-time dependency per the advice in the numpy2 transition documentation. Then, I used the CI/CD workflows to test that pgenlib will continue to work with numpy 2.0

@aryarm aryarm changed the title Adapt CI/CD for python pgenlib package to test against numpy 2.0 Adapt CI/CD for python pgenlib to test against numpy 2.0 Jun 17, 2024
@chrchang
Copy link
Owner

I have performed the requested trusted-publishing setup, and will try the "Release pgenlib" workflow tomorrow if there are no further comments.

@aryarm
Copy link
Contributor Author

aryarm commented Jun 17, 2024

ok, thanks! There is actually one mistake that I just realized:

The default workflow name under the trusted publishing setup directions is "release.yml" but I called the workflow "release.yaml"

Hopefully you caught that already when you were setting it up?

As you can imagine, I didn't have a chance to test the upload_pypi step of the workflow, so there might be hiccups when you run it...

@chrchang
Copy link
Owner

Yes, I entered "release.yaml" on PyPI.

@aryarm
Copy link
Contributor Author

aryarm commented Jun 18, 2024

Last-minute change: I reverted to numpy v1.19 (not v1.21) as the oldest supported runtime version. v1.19 was not working for me before because it was failing on macos arm64, since numpy only started supporting macos arm64 in v1.21. However, v1.19 still works on Ubuntu, so I decided to just skip the macos tests against v1.19 in the CI.

In my testing, I actually found that we could even set the lowest numpy version to v1.16 (see here). But that would be even lower than the version that you originally had in your dev-requirements.txt, and I didn't want to change things too much. I figured there might be other reasons that you set it to v1.19 that I might be unaware of.

Also, please feel free to squash these commits if you merge them. I unfortunately didn't maintain the cleanest commit history with this PR.

@chrchang chrchang merged commit f6273ab into chrchang:master Jun 19, 2024
1 check passed
@aryarm
Copy link
Contributor Author

aryarm commented Jun 19, 2024

@chrchang, thanks for merging this!

I saw the release workflow failed. Did you also set up trusted publishing for testpypi? If so, make sure that you're leaving out the "Environment name", or alternatively, you can specify an environment name in the workflow like this:

  upload_pypi:
    needs: [build_wheels, build_sdist]
    runs-on: ubuntu-latest
    if: github.repository_owner == 'chrchang'
+   environment: 'release'

    permissions:
      id-token: write

    steps:

Leaving out the environment name is probably fine in situations where only you or just a few people have push access to this repository.

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.

2 participants