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

Trim wheel builds #320

Merged
merged 3 commits into from
May 7, 2022
Merged

Trim wheel builds #320

merged 3 commits into from
May 7, 2022

Conversation

jakirkham
Copy link
Member

Partially addresses issue ( #312 )

The wheel build matrix is taking 2.5hrs to go through. Some of these builds are for novel architectures or deployments that are not really tested. Trim the build matrix to a more reasonable set of build to get the build time more manageable.

TODO:

  • Unit tests and/or doctests in docstrings
  • tox -e py39 passes locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • tox -e docs passes locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Coveralls passes)

jakirkham added 3 commits May 6, 2022 11:32
Have not been doing testing for 32-bit or supported it for some time. So
drop 32-bit builds from wheels.
This is a bit of an edge case that we are not otherwise testing on. So
skip these builds to lighten the build matrix.
This is also a bit of an edge case architecture wise. Also we are not
doing any other testing there. So drop it.
@jakirkham
Copy link
Member Author

jakirkham commented May 6, 2022

Am still seeing build (3.6) in the statuses above even though PR ( #318 ) was merged. Must be missing something still

Fixed. Please see comment ( #318 (comment) ).

cc @joshmoore

@jakirkham jakirkham mentioned this pull request May 6, 2022
7 tasks
@jakirkham
Copy link
Member Author

Much better. The longest running of these is macOS at 14mins.

@jakirkham jakirkham requested a review from joshmoore May 6, 2022 19:02
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

The change itself (i.e. syntactically) looks fine, but I don't have any particular thoughts on the potential coverage. Would running these on tag be a compromise?

@jakirkham
Copy link
Member Author

jakirkham commented May 6, 2022

Well the question from my perspective is who is asking for these other builds? Currently we have people asking about ARM in various cases, which can be good to include. There are often PPC users at HPC facilities. So that can matter. Neither of these is currently excluded, but we may need to do additional work to add them (ideally in a separate matrix item). They will also take additional resources.

However I'm not aware of folks asking for musl support or s390x. These are actually eating up a fair amount of time to build (this includes emulation cost for s390x) despite not having any demand AFAICT.

Also 32-bit was dropped a long time ago ( #156 ). So it is kind of weird that it is coming back through wheels when it isn't supported.

To summarize, the things being dropped here are things AFAIK no one has asked for and/or are not supported. The result is all PRs taking ~2.5hrs to run on CI, which is pretty significant friction for the average contributor, dropping to ~15mins, which is pretty reasonable. I don't have a problem adding and filtering new things as users request them, but spending a lot of time building things nobody is asking us for or we don't support doesn't make a lot of sense (at least not to me anyways).

@joshmoore
Copy link
Member

All fair. Worst case they'll let us know on twitter. 😉

@joshmoore joshmoore merged commit ab92198 into zarr-developers:master May 7, 2022
@jakirkham jakirkham deleted the trim_whl_blds branch May 7, 2022 08:02
@jakirkham
Copy link
Member Author

Thanks Josh 😀

And if they do happy to revisit then 😉

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