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

Speed up mypy wheel builds #9536

Closed
JukkaL opened this issue Oct 5, 2020 · 8 comments · Fixed by #9570
Closed

Speed up mypy wheel builds #9536

JukkaL opened this issue Oct 5, 2020 · 8 comments · Fixed by #9570
Assignees
Labels
bug mypy got something wrong priority-0-high topic-developer Issues relevant to mypy developers

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 5, 2020

Travis mypy release wheel builds may take around 1.5 hours to complete: https://travis-ci.org/github/mypyc/mypy_mypyc-wheels/builds

This makes iteration on mypy release builds very slow.

Ideas about how to make these faster:

  • Move at least some of the jobs to GitHub actions. It should give us more parallelism.
  • Skip some tests that are slow and relatively low signal, at least in the slowest jobs.

Any help here would be appreciated, as this is one of the main causes of friction when making mypy releases. If we could get these to around 30 minutes, things would already be much better.

Note that the build scripts live in a different repository: https://github.com/mypyc/mypy_mypyc-wheels

@JukkaL JukkaL added bug mypy got something wrong priority-0-high topic-developer Issues relevant to mypy developers labels Oct 5, 2020
@hauntsaninja
Copy link
Collaborator

It seems just moving everything to Github Actions would do it. They allow for 5 concurrent macOS jobs, as opposed to Travis' 2. They also allow 20 concurrent jobs as opposed to Travis' 5.

While we're making changes here, we should also do what's needed for Python 3.9.

@gvanrossum
Copy link
Member

I'm not objecting to the move, but I note that perhaps this is an attempt by GitHub to kill the competition? After all nothing says GitHub can't reduce these limits after Travis is out of business (or even just heading in that direction).

@hauntsaninja
Copy link
Collaborator

It's true; the change to have "details" click through to Github Actions for Travis build status rather than linking to Travis was something I found striking. More conspiratorially I think some of the difficulties Travis has had reporting PR status back to Github might be somewhat intentional.

I'd worry about it more if mypy_mypyc_wheels contributed positively to Travis' bottom line (I didn't even know that the project existed until today, so it isn't even providing them marketing value).

hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this issue Oct 8, 2020
I was trying to build wheels for Python 3.9 as part of python#9536, but ran
into this issue. You'll notice a couple hundred lines up msullivan
points out that mypyc can't handle conditional method definition, so
that's not an option here.
hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this issue Oct 8, 2020
I was trying to build wheels for Python 3.9 as part of python#9536, but ran
into this issue. You'll notice a couple hundred lines up msullivan
points out that mypyc can't handle conditional method definition, so
that's not an option here.
hauntsaninja added a commit that referenced this issue Oct 8, 2020
I was trying to build wheels for Python 3.9 as part of #9536, but ran
into this issue. You'll notice a couple hundred lines up msullivan
points out that mypyc can't handle conditional method definition, so
that's not an option here.

Co-authored-by: hauntsaninja <>
@hauntsaninja hauntsaninja self-assigned this Oct 8, 2020
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Oct 9, 2020

I put together a working version of mypy_mypyc-wheels on Github Actions at https://github.com/hauntsaninja/mypy_mypyc-wheelsv2 . It currently takes pretty much exactly half an hour to run:
https://github.com/hauntsaninja/mypy_mypyc-wheelsv2/actions/runs/296713743
https://github.com/hauntsaninja/mypy_mypyc-wheelsv2/releases/tag/tag-7131a66aeed027daa6d83818870c437dbcbebaef

It’s basically a rewrite, using cibuildwheel to take care of and unify a lot of the logic for us. cibuildwheel is well maintained and makes this really easy. Eg, it takes care of installing python versions, manylinux docker, running auditwheel / delocate, etc. Previously we did our own thing on windows, another thing on linux and mostly used multibuild for mac os. This reduces our repo to one fairly short yaml file. I also build an sdist and python wheel. The rewrite will necessitate some changes in mypy/misc, mostly simplifications / code deletion.

We previously didn’t seem to run any tests on windows wheels. I had some failures running tests on windows: all but one of them were issues with tearing down stubgen tests (it appears stubgen tests have previously had issues on windows, maybe ethanhs remembers more). The other looked like some relative path / absolute path confusion. I decided it was still an improvement to just run testcheck (if we were to do this for other builds as well, we’d get our time down to 20m).

Another windows note is that I don’t have an equivalent of https://github.com/mypyc/mypy_mypyc-wheels/blob/5638de415c9fc7fa9076b2d9a429e887817943a0/appveyor.yml#L32-L39 it sounds like these were meant to work around build failures, but my builds succeeded, so I didn’t look further. Maybe msullivan knows if these are needed?

An os x note is that cibuildwheel uses 10.9 as the minimum os x target. We seem to have more or less made that change too, except for python 3.5, which we currently seem to struggle with.

I made some fixes to mypy, so python3.9 wheels now build. However, python3.9 test requirements fail to install because lxml doesn’t have a wheel yet. It looks like this was a problem for python3.8 as well; in fact, to this day we don’t test python3.8 wheels because of this. It seems hard to subtract the lxml tests, so maybe I'll follow suit and disable tests for python3.9 wheels for now. (edit: I changed it so we skip tests if we fail to install lxml, so tests will start running automatically once lxml releases windows and linux wheels. Here's a complete build: https://github.com/hauntsaninja/mypy_mypyc-wheelsv2/releases/tag/tag-e0d761effdcbefc1d0a8f3ba26271c7d5f2a8b02 )

Take a look and let me know if there are concerns (feels like this change should sit out until the 0.790 release); I'll open PRs against mypy_mypyc-wheels and mypy

@vbarbaresi
Copy link
Contributor

I also tried to port the Linux jobs to GitHub yesterday, I got them working with the same steps as Travis:

A little too late though, but maybe it can be useful in some way.
I note that you also had to resort to a JS Script to upload wildcard files to a release because of the same issue.

I initially thought that moving linux jobs on GitHub, keeping OSX on Travis and Windows on Appveyor could be a good idea. And would avoid vendor lock-in if GitHub starts reducing capacity, like gvanrossum noted.
But on the other hand maintaining 3 different CI flavors is a pain, so I like hauntsaninja's solution

@JukkaL
Copy link
Collaborator Author

JukkaL commented Oct 9, 2020

@hauntsaninja

Take a look and let me know if there are concerns (feels like this change should sit out until the 0.790 release); I'll open PRs against mypy_mypyc-wheels and mypy

Awesome, this all sounds good! The slow wheel builds have been a big pain point. Let's wait until 0.790 has been out for a few days and we can then start deploying the changes.

An os x note is that cibuildwheel uses 10.9 as the minimum os x target. We seem to have more or less made that change too, except for python 3.5, which we seem to struggle with.

10.9 as the minimum target seems fine if we can still support 3.5. I think I had some issues with using it with Python 3.5, but if you can make it work, it would be good since it would reduce the number of different configurations we'd support. (Also I fear that we'll need to support Python 3.5 for some time still, since Ubuntu 16.04 ships with 3.5 and it's end of life is only April 2021. Some companies are known postpone Ubuntu upgrades until the last minute.)

@vbarbaresi

Thanks for looking into this!

I initially thought that moving linux jobs on GitHub, keeping OSX on Travis and Windows on Appveyor could be a good idea. And would avoid vendor lock-in if GitHub starts reducing capacity, like gvanrossum noted. But on the other hand maintaining 3 different CI flavors is a pain, so I like hauntsaninja's solution

Appveyor has been occasionally problematic in the past so I'm kind of happy if we don't need to use them. We are still using Travis for mypy CI, and I think that it makes sense to continue to do this, for the reasons you mentioned.

@hauntsaninja
Copy link
Collaborator

As far as I can tell, the OS X Python 3.5 wheel works. I think that's something cibuildwheel takes care of for us: https://github.com/joerick/cibuildwheel/blob/97dd4ee1c06f3cf271a26c61a59305f7598a0fef/cibuildwheel/macos.py#L81
(Related, as long as the pure Python wheel works, isn't Python 3.5 still supported?)

Thanks so much for helping out here vbarbaresi! Yeah, it's crazy that there isn't a better way of uploading files to a release.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 7, 2021

Looks like things mostly work! https://github.com/mypyc/mypy_mypyc-wheels/actions/runs/467969868 finished in under 29m, and we'll get a couple minutes back soon when we drop Python 3.5 (although we'll eventually lose time with Python's yearly cadence)

Here's a small checklist (for my own reference):

Future improvements:

  • Figure out how to turn off AppVeyor. Not sure I have the permissions to do this.
  • Add mypyc requirements to pyproject.toml and use isolated builds
  • Run some tests against sdist and Python wheel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong priority-0-high topic-developer Issues relevant to mypy developers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants