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

Automated Wheel Creation using Github Actions #1409

Merged
merged 33 commits into from
May 27, 2020

Conversation

mrmundt
Copy link
Contributor

@mrmundt mrmundt commented Apr 23, 2020

Fixes N/A

Summary/Motivation:

Cutting new releases would be made significantly better with automation. In addition, we would be able to create more than just a universal wheel - we'd be able to release cythonized wheels to improve user performance.

Changes proposed in this PR:

  • Github Actions Workflow to create wheels and make them available as artifacts
  • Manylinux wheel creation using a Github-user created action

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #1409 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1409   +/-   ##
=======================================
  Coverage   72.40%   72.40%           
=======================================
  Files         633      633           
  Lines       90228    90228           
=======================================
  Hits        65334    65334           
  Misses      24894    24894           

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 c3c1f03...c3c1f03. Read the comment docs.

@mrmundt
Copy link
Contributor Author

mrmundt commented Apr 23, 2020

@jsiirola @blnicho - I have some questions about the release process and how to make appropriate changes to accommodate. I will email you. Let me, however, detail other changes that need to happen (and please provide input if you have any):

  • (Windows) Currently, I can only get wheels to build for 3.6+. This is an issue with the version of Microsoft Visual Studio on the GA runners.
  • (Manylinux) The Mac/Windows jobs run fairly quickly because they happen in parallel. The way the Manylinux action works, however, is to create all wheels in serial (Py27, Py35, etc.). It's probably not a big deal since releases aren't being cut all the time (plus it's still faster than doing anything manually), but I wanted you to be aware.
  • (PyPy) I'm not sure if this works yet for PyPy. Haven't tried it.
  • (Secrets) Uploading to PyPi requires a username/password. The best way I can tell to do this is to use Github Secrets. I put my username/password combo for TestPyPi as a secret on my own fork and it worked beautifully.

@jsiirola
Copy link
Member

Some thoughts:

  • I am OK if we only provide the "easy to generate" wheels. People using older / stranger platforms can perform their own build as part of setup.py install / pyomo build-extensions
  • I agree that speed is not a real concern; at least not yet. 22 minutes is not a huge deal.
  • Actually, I would be a bit surprised if the cythonization did work for pypy. I think it would be interesting to look for pypy wheel download stats for some other packages (numpy?) to get an idea of demand before we invest too much time in supporting it.
  • As @blnicho mentioned elsewhere, I don't think that I would want to automate the push to pypi ... at least not until we were much more confident in the automated build process.

@mrmundt
Copy link
Contributor Author

mrmundt commented Apr 24, 2020

@jsiirola

  • Sounds good to me! Makes things a lot easier.
  • Great. I won't focus on it.
  • It doesn't! I tried. Cythonization only works for CPython's - which is not pypy.
  • Sounds good. I converted it to an upload-artifact action, so we can download the wheels and do as we please with them.

@blnicho blnicho marked this pull request as draft May 19, 2020 18:36
@blnicho blnicho marked this pull request as ready for review May 19, 2020 18:37
@mrmundt mrmundt changed the title [WIP] Automated Wheel Creation using Github Actions Automated Wheel Creation using Github Actions May 26, 2020
@jsiirola
Copy link
Member

@mrmundt: I was just looking into the Linux wheels. Is there a reason we generate linux, manylinux1, and manylinux2010? does the 2010 wheel provide anything that the 1 does not?

@mrmundt mrmundt requested review from blnicho and jsiirola May 26, 2020 16:51
@mrmundt
Copy link
Contributor Author

mrmundt commented May 26, 2020

@mrmundt: I was just looking into the Linux wheels. Is there a reason we generate linux, manylinux1, and manylinux2010? does the 2010 wheel provide anything that the 1 does not?

The main difference between 1 and 2010 is what they're built on - CentOS 5 vs. CentOS 6 (PEP 571). The action we are using to make the manylinux wheels just makes all of them by default, and we can pick which ones we want to upload. Currently, numpy uses manylinux1, and if we're trying to line up with their practices, we don't need the other ones.

EDIT: It doesn't look like manylinux2010 has actually been fully released for use, but their tracking issue for its release has indicated that it's flagged for a final state. So maybe it's ACTUALLY going to be released soon, and then we can migrate to using those ones.

EDIT EDIT: Oh, actually, reading through all of the notes in that tracking issue (and there are a LOT of them), they consider it rolled out. So. Up to you if we want to use the manylinux1 or manylinux2010 wheels.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

This looks great.

python-versions: 'cp27-cp27mu cp35-cp35m cp36-cp36m cp37-cp37m cp38-cp38'
build-requirements: 'cython'
package-path: ''
pip-wheel-args: '--no-deps'
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what is the --no-deps doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per pip wheel documentation:

--no-deps
    Don’t install package dependencies.

I'm not sure if that's necessary for us. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Does that apply to setting up the build environment, or is that somehow embedded into the resulting wheel? Regardless, I think that you are correct and that it is not necessary (there are only a few Pyomo dependencies and they all install reliably).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's somehow embedded into the resulting wheel. Documentation for the manylinux action states that the default is --no-deps (hence why I didn't change it), but I'll remove that.

(NOTE: Documentation also specifies that the linux wheels that are created NOT be uploaded to PyPi. Not sure why the Action creator doesn't just remove them in his action, but I can add a middle step to remove all linux files before we upload the artifacts, if you'd like.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsiirola - Ah, I remember why I had --no-deps now - my local testing fails because it can't find the right PyUtilib distribution (because PyUtilib 5.8.1.dev0 isn't on PyPi, doy). This isn't a problem if we do PyPi uploads first and THEN do the Pyomo wheel creation, however.

@mrmundt
Copy link
Contributor Author

mrmundt commented May 26, 2020

@jsiirola , @blnicho - It is now good to go! Feel free to merge.

@blnicho blnicho merged commit 429225b into Pyomo:master May 27, 2020
@mrmundt mrmundt deleted the wheel_creation branch May 27, 2020 13:48
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