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

Generate manylinux wheels #13

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

DanShort12
Copy link
Contributor

This PR allows the plasma source to be installed without requiring a local build. It makes use of manylinux (as defined under pep 513) to create the build on a container that supports forward compatibility. In particular the Python install used is as provided by the container, not the GitHub version. The Python module auditwheel is used to ensure that the manylinux wheel is created successfully. This should be compatible with any CentOS 7 (or equivalent) and higher Linux operating system, by using the manylinux2014 standard.

@shimwell
Copy link
Contributor

shimwell commented Sep 25, 2020

This is just what we needed, thanks for figuring all this out

I would be tempted to put the published stuff back in but perhaps with these usernames PYPI_USERNAME and password PYPI_PASSWORD which @makeclean has added to the repository settings

    - name: Publish wheel artifact
      env:
        TWINE_USERNAME: ${{ secrets.TEST_PYPI_USERNAME }}
        TWINE_PASSWORD: ${{ secrets.TEST_PYPI_PASSWORD }}
      run: |
        export PYVER=${{ matrix.python-version }}
        alias python=$(ls -d /opt/python/* | grep ${PYVER//.})/bin/python
        python -m pip install twine
        python -m twine upload --repository testpypi wheelhouse/*

@shimwell
Copy link
Contributor

Does it need something like so that the uploading only triggers when merging into master. Also I think we need to rename master to main branch at some point

on:
  push:
    branches:
      - master

@shimwell
Copy link
Contributor

@DanShort12 I can confirm that any has added github secrets PYPI_USERNAME and PYPI_PASSWORD

@DanShort12
Copy link
Contributor Author

This is just what we needed, thanks for figuring all this out

I would be tempted to put the published stuff back in but perhaps with these usernames PYPI_USERNAME and password PYPI_PASSWORD which @makeclean has added to the repository settings

    - name: Publish wheel artifact
      env:
        TWINE_USERNAME: ${{ secrets.TEST_PYPI_USERNAME }}
        TWINE_PASSWORD: ${{ secrets.TEST_PYPI_PASSWORD }}
      run: |
        export PYVER=${{ matrix.python-version }}
        alias python=$(ls -d /opt/python/* | grep ${PYVER//.})/bin/python
        python -m pip install twine
        python -m twine upload --repository testpypi wheelhouse/*

The accounts used would depend on where we're uploading to. This snippet uploads to TestPyPI so it may not be the case that this shares credentials with production PyPI - although of course the same credentials could be used for both repos, in which case the same secrets could be used. I'm also not sure if a suitable account exists on TestPyPI (I used an account that I created myself when I checked the upload to TestPyPI).

@DanShort12
Copy link
Contributor Author

Does it need something like so that the uploading only triggers when merging into master. Also I think we need to rename master to main branch at some point

on:
  push:
    branches:
      - master

I don't think we'd want this exactly - we still want to be able to run the rest of the workflow on a PR, otherwise the build and tests won't be applied to PRs, so there won't be any checks. I think we'd want something like the below so that the upload only happens on a tag:

if: startsWith(github.ref, 'refs/tags')

around the "Publish wheel artifact" action. PyPA actually have a guide with a suggestion of how to do that, which we could adopt (to upload to TestPyPI if the action isn't a tag, or to upload to PyPI if the action is a tag).

However, that still raises the question of how the PyPI packages will be versioned. Since TestPyPI and PyPI need unique versions for each upload we'd need some logic to generate clean versions on upload to PyPI and unique (timestamped?) versions when uploading to TestPI. How would you prefer to do that?

@shimwell
Copy link
Contributor

In the past I've used bump to update the version number automatically, but it might be best for the pull request to contain the incremented version number in the setup.py as the contributor knows if it is a major, minor or patch update

To upload to PyPi instead of TestPyPi we could replace

python -m twine upload --repository testpypi wheelhouse/*

with

python -m twine upload wheelhouse/*

But I see the example uploads to TestPyPi every time and PyPi only if the commit is tagged which is a bit nicer.

    - name: Publish distribution 📦 to Test PyPI
      uses: pypa/gh-action-pypi-publish@master
      with:
        password: ${{ secrets.test_pypi_password }}
        repository_url: https://test.pypi.org/legacy/
    - name: Publish distribution 📦 to PyPI
      if: startsWith(github.ref, 'refs/tags')
      uses: pypa/gh-action-pypi-publish@master
      with:
        password: ${{ secrets.pypi_password }}

But then we get the version number problem. Do you want to use the patch part of the version number for PyPi and keen major and minor increments for PyPi uploads

@DanShort12
Copy link
Contributor Author

I think we it will be tricky to use semantic versioning for the PR part, if pushes to a PR are going to generate an upload to TestPyPI - I think it's fair to assume that a developer wouldn't remember to bump a version number each time they are ready to push. Also IMO a PR shouldn't correspond to a new released version of the code - it's a potential new released version, but the actual released versions should align to tags in git (hence why PyPA suggest to only upload to PyPI on a new tag) e.g. a released version should be version x.y.z but a pre-release version in a PR should be x.y.z-some-metadata. If that's a suitable versioning method then we could just use the version described by git?

git describe --always --tags

That gives the release version if the commit corresponds to a tag (if annotated, which it should be for a release), else it gives the number of commits since the last release and the short git hash of the commit. Basically this. That seems like a nice way to keep versions distinct for TestPyPI, while still having some control over tagging actual releases.

Formats the version in a way that handles Python versioning standard
(PEP 440) by appending the number of commits since the last tag to a dev
pre-release identifier.
Allows override if release not yet found (we don't have tags yet).
@DanShort12
Copy link
Contributor Author

DanShort12 commented Sep 28, 2020

Hi @makeclean, after a bit of GitHub wrangling I think this is nearly ready to run with a full CI chain (build, test, publish). Would it be possible to create a TestPyPI account that can be used to upload new versions of the package to TestPyPI when working under a PR? We would then use that account with the TEST_PYPI_USERNAME and TEST_PYPI_PASSWORD secrets to be able to upload new packages.

TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
run: |
python3 -m pip install twine
python3 -m twine upload dist/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add this, just to provide more info should it ever break

Suggested change
python3 -m twine upload dist/*
python3 -m twine upload dist/* --verbose

TWINE_PASSWORD: ${{ secrets.TEST_PYPI_PASSWORD }}
run: |
python3 -m pip install twine
python3 -m twine upload --repository testpypi dist/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add this, just to provide more info should it ever break

Suggested change
python3 -m twine upload --repository testpypi dist/*
python3 -m twine upload --repository testpypi dist/* --verbose

@DanShort12
Copy link
Contributor Author

Updated to use latest version on PyPI rather than by getting number of git commits. Should be more stable if multiple PRs are open.

@DanShort12
Copy link
Contributor Author

It's not clear to me why the publish still fails - have the secrets in this repo been set up correctly?

@shimwell
Copy link
Contributor

shimwell commented Oct 5, 2020

It's not clear to me why the publish still fails - have the secrets in this repo been set up correctly?

Ah yes I see what you are saying, the github actions is reporting the pypi username password is not correct. This needs access to the repository settings to check

@shimwell
Copy link
Contributor

shimwell commented Oct 6, 2020

Sorry to be a pain is there any chance this PR can be redirect to the new repository

https://github.com/open-radiation-sources/parametric-plasma-source

@DanShort12

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