PySCeS / pysces

The official PySCeS project source code repository.
https://pysces.github.io
Other
34 stars 10 forks source link

Build wheels with github actions #67

Closed jmrohwer closed 2 years ago

jmrohwer commented 2 years ago

I've managed to set up a github action using cibuildwheel that will successfully build wheels for all three platforms (linux, windows, macOS). All works and tested. Automated uploading to PyPI is also possible (I have the code to do that) but not yet implemented or tested.

This was tested on the ci-test branch, but the workflow has now been edited to be triggered on push to master and when a new tag is created (i.e. upon new releases). It currently fails because the workflow is only on this ci-test branch but should work once merged.

We have to agree on when to trigger this, but on releases seems sensible. CI time is also not unlimited on free Github account.

I'm creating this as a draft PR, waiting for your comments @bgoli. The workflow is also currently manually disabled in Github actions.

This actually went easier than I had anticipated :grin:

jmrohwer commented 2 years ago

This is all set up and ready to go! :100: I'm marking it as ready for review. There are two workflows:

  1. Makes portable wheels for all 3 OS's and Python 3.6-3.9, uploads to PyPI.
  2. Makes Anaconda packages for all 3 OS's and Python 3.6-3.9, uploads to anaconda.org/pysces

I tested it all in the current branch with a trigger set to every push. The trigger is now set to run only on a release. However, not to mess up anything there are 2 checks still in place.

Quite excited about this :+1:

bgoli commented 2 years ago

@jmrohwer this looks fantastic, I've been thinking about the automatic upload and the only problem is that this only really works of you have a comprehensive set of unit tests that cover every aspect of the code. So what about if we use these pipelines for builds but still do manual tests on the binaries before upload (or use the existing pipeline with test.pypi)?

One question I have is with connecting the action with a release. In principle this is fine but we may also want to test binaries before the release (especially if we go with a build-->upload pipeline or change code that may be platform specific).

I have no idea if this is feasible but do the build systems allow you to do something like:

Just an idea (I haven't worked with GitHub actions), the current release based trigger is great.

(Edit. Perhaps we could also mix the two so we have a full release triggered build on main which we can run after commit triggered testing on a build-test branch?)

jmrohwer commented 2 years ago

@bgoli replies inline below.

@jmrohwer this looks fantastic, I've been thinking about the automatic upload and the only problem is that this only really works of you have a comprehensive set of unit tests that cover every aspect of the code. So what about if we use these pipelines for builds but still do manual tests on the binaries before upload (or use the existing pipeline with test.pypi)?

Thanks, I was impressed myself with what is possible! The questions about testing is: up to now we have not really done that extensive testing before uploading to PyPI or Anaconda either. Of course that is not an ideal situation and it should change in future. But I can't fully see the difference between manual or automatic upload to the repos.

The important thing is:

Personally I am not keen on manual uploads. The point of CI is to save work. If I have to download all the tarballs and wheels manually to upload them to PyPI and Anaconda by hand, this is missing the point. I have the builds set up on my machine locally, and then I might just as well do everything by hand. Part of the reason I invested the effort to get all OS builds build off the same recipe is to get the CI to work.

But I have another idea to address your concerns (see below).

One question I have is with connecting the action with a release. In principle this is fine but we may also want to test binaries before the release (especially if we go with a build-->upload pipeline or change code that may be platform specific).

I have no idea if this is feasible but do the build systems allow you to do something like:

* create say a _build-test_ branch which will build on commit

* add all new code to main/development (as we are now)

* when a test is required, do a local update of _build-test_ to main/development

* commit the update to _build-test_ which would trigger a build

Just an idea (I haven't worked with GitHub actions), the current release based trigger is great.

GitHub actions is very versatile with triggers. IMO the extra build-test branch will unnecessarily complicate things. There are simpler options.

(Edit. Perhaps we could also mix the two so we have a full release triggered build on main which we can run after commit triggered testing on a build-test branch?)

My suggestion basically comes to this but we don't need a separate build-test branch.

If in the meantime you want to test some of the builds, they can be downloaded as artifacts from the workflow runs. I haven't always built all the Python versions to save time and resources, here are some links so you don't have to search all over the place.

Wheels

Conda packages

bgoli commented 2 years ago

Of course I get that you don't want to upload binaries, but, I do not want to publish broken binary packages. That being said, your suggestions for either conditional builds and eventually using the development/main seem like good solutions.

I'm going to test the Linux wheels on Fedora, CentOS, FreeBSD and whatever other VM's I have around and see what happens.

jmrohwer commented 2 years ago

Let me know how the testing goes. These are manylinux2014 wheels since I figured we don't really need earlier support than that. This supports CentOS 7+. CentOS 6 is EOL in any case so should be good to go.

I have pushed the manual dispatch and changes to GH releases to the workflow scripts. The manual dispatch will only feature under Actions once this has been merged to master. I have not yet updated the upload locations in the workflow scripts.

I have also added a bunch of NLEQ2 tests (basically the level 2 steady state tests, but calling NLEQ2), pushed directly to master. At least we are then testing both externally built modules.

jmrohwer commented 2 years ago

@bgoli Okay, so it turns out the Anaconda binaries on macOS weren't really portable because the build doesn't run something like delocate as it does for the wheels. The wheels seem to be truly portable. The only way I could get this to work on macOS was to actually use the Anaconda compilers for the build (various gfortran versions from xcode or homebrew would not work as the symbols would not be found on an install on a system where these specific compiler libraries were not installed).

It would help a lot if you could test the new versions on any Anaconda installs you may have lying around, in a freshly created environment. You can get the builds here:

The last link has older builds, where for the Windows one it was still using the Windows native compilers. So use the separate build.

To test, unzip the downloaded build_outputs.zip to a folder, say packages. The internal directory structure should remain intact (linux-64, osx-64, win-64). We first have to index the folder so it can be used during install.

conda activate base
conda index packages
conda create -n py38test python=3.8
conda activate py38test
conda install -c packages pysces
python -c "import pysces; pysces.test(3)"

Would be great if you could test on linux and win, not sure if you have access to a macOS machine.

As a side consequence, the build should now run locally only using the Anaconda compilers :smile:, on this branch (ci-test). Navigate to packaging/conda and run conda build . or if you have installed mamba mamba build . (it's MUCH faster).

bgoli commented 2 years ago

@jmrohwer sounds good, I managed to do some testing:

Looks fine to me, I will try some of the PyPI packages but don't expect there to be a problem and will try get some more SBML fixes done by Friday and then afaik we can wrap a release.

jmrohwer commented 2 years ago

@bgoli Okay if I merge this to master or do you want to hold off? I also added a commit to implement #43, this will only run on release published.