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: Reconfigure build matrix for Python 3.11 and PyPy 3.9 #456

Merged
merged 4 commits into from May 25, 2022

Conversation

EwoutH
Copy link
Contributor

@EwoutH EwoutH commented May 24, 2022

Reconfigures the build matrix to include Python 3.11 (which is currently in beta) and PyPy 3.9. PyPy 3.8 is also added and 3.7 is removed.

All Python versions (3.7 to 3.11, and PyPy 3.8 and 3.9) are tested on Ubuntu, and the latest stable versions (currently Python 3.10 and PyPy 3.9) also on macOS and Windows.

A weekly scheduled run is also added to catch changes in dependencies and environments (each Monday at 06:00 UTC), and workflow_dispatch is added to be able to manually trigger the CI workflow.

The used actions are updated to v3 and caching is now configured in the setup-python action.

Reconfigures the build matrix to include Python 3.11 (which is currently in beta) and PyPy 3.9. PyPy 3.8 is also added and 3.7 is removed.

All Python versions (3.7 to 3.11, and PyPy 3.8 and 3.9) are tested on Ubuntu, and the latest stable versions (currently Python 3.10 and PyPy 3.9) also on macOS and Windows.

A weekly scheduled run is also added to catch changes in dependencies and environments (each Monday at 06:00 UTC), and workflow_dispatch is added to be able to manually trigger the CI workflow.
It might crash the python setup process, because it relies on a requirements.txt file being present.
@EwoutH
Copy link
Contributor Author

EwoutH commented May 24, 2022

The CI failed because caching pip dependencies with setup-python only currently works when a requirements.txt file is present. I commented that line out, the CI now passes on my fork.

@agronholm
Copy link
Contributor

The documentation also does not specify what the generated cache key is, which is why I've avoided that feature so far. How do you know it works correctly in every situation?

@agronholm
Copy link
Contributor

You've removed caching entirely, and this is not acceptable. Unless you can replace the old caching setup with a superior one, please restore the old setup.

I see that you've added a lot of jobs w/ macOS and Windows. What is the intended function of the include section which replaced my old exclude section?

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #456 (b672a1c) into main (a64c6bb) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
+ Coverage   67.57%   67.68%   +0.10%     
==========================================
  Files          12       12              
  Lines         916      916              
==========================================
+ Hits          619      620       +1     
+ Misses        297      296       -1     
Impacted Files Coverage Δ
src/wheel/bdist_wheel.py 50.00% <0.00%> (+0.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a64c6bb...b672a1c. Read the comment docs.

The extra Windows and macOS builds were supposed to be included extra aside from the Ubuntu jobs (which run with each Python version). This creates a sparse matrix.
@EwoutH
Copy link
Contributor Author

EwoutH commented May 24, 2022

You've removed caching entirely, and this is not acceptable. Unless you can replace the old caching setup with a superior one, please restore the old setup.

I noticed installing the test dependencies only takes about 5 seconds. Is it so essential to have a cache (using bandwidth and storage) for that?

I added it back in the latest commit.

I see that you've added a lot of jobs w/ macOS and Windows. What is the intended function of the include section which replaced my old exclude section?

Instead of creating a full-factorial test setup spanning two dimensions (with m*n jobs) and excluding some, this test setup is sparse, testing only all Python versions on one OS and only certain Python versions on the other OSes. The previous test setup was 11 jobs with 5 Python versions, this configuration configuration has also 11 jobs, but tests 7 different versions of Python.

I only forgot to remove the macOS and Windows OSes, that is now fixed.

@agronholm
Copy link
Contributor

You've removed caching entirely, and this is not acceptable. Unless you can replace the old caching setup with a superior one, please restore the old setup.

I noticed installing the test dependencies only takes about 5 seconds. Is it so essential to have a cache (using bandwidth and storage) for that?

I added it back in the latest commit.

Not for wheel per se, but given its position in the Python packaging infrastructure, I expect lots of developers to peek at its CI configuration so I think wheel should try to be an example to follow, by adopting the best available practices.

I see that you've added a lot of jobs w/ macOS and Windows. What is the intended function of the include section which replaced my old exclude section?

Instead of creating a full-factorial test setup spanning two dimensions (with m*n jobs) and excluding some, this test setup is sparse, testing only all Python versions on one OS and only certain Python versions on the other OSes. The previous test setup was 11 jobs with 5 Python versions, this configuration configuration has also 11 jobs, but tests 7 different versions of Python.

I only forgot to remove the macOS and Windows OSes, that is now fixed.

Okay, now it makes perfect sense.

Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

I feel that the oldest supported Python versions should be tested on macOS and Windows too, as they were before – just to be sure. Occasionally these OS specific issues pop up in refactored code. Beyond that, this PR looks good.

Python 3.7 is the lowest version supported
@EwoutH
Copy link
Contributor Author

EwoutH commented May 25, 2022

Done!

@agronholm agronholm merged commit c2357d8 into pypa:main May 25, 2022
@agronholm
Copy link
Contributor

Thanks! The scheduled run should prove useful.

@mayeut
Copy link
Member

mayeut commented Jun 4, 2022

@agronholm,

Not for wheel per se, but given its position in the Python packaging infrastructure, I expect lots of developers to peek at its CI configuration so I think wheel should try to be an example to follow, by adopting the best available practices.

I have to disagree with what's considered the best available practices here. IMHO, the best available practice is not to enable cache as a default.
You have to check some pre-requisites for it to be effective and this will be on a project by project basis. Rather than "If you don't know (i.e. default), enable it", I'd prefer "if you know what you're doing, enable it". c.f. my rationale in the following comment: pypa/packaging#517 (comment)

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.

None yet

3 participants