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

Add GitHub action for publishing artifacts to PyPI #5244

Merged
merged 23 commits into from
May 6, 2021

Conversation

andersy005
Copy link
Member

@andersy005 andersy005 commented May 2, 2021

This addresses #5232 (comment), and will (hopefully) reduce the time it takes to cut a new release.

  • Passes pre-commit run --all-files

@max-sixty
Copy link
Collaborator

Excellent! Thanks a lot @andersy005 .

We could change the "how to release" steps too, in another PR is fine (or I can do it).

@andersy005
Copy link
Member Author

andersy005 commented May 2, 2021

We could change the "how to release" steps too, in another PR is fine (or I can do it).

+1 for updating the how to release doc in another PR... I should point out that there are steps that this action doesn't address. For instance, step 2 and step 16

  1. Confirm there are no commits on stable that are not yet merged
    (ref):

    git merge upstream/stable
    
  2. Update the stable branch (used by ReadTheDocs) and switch back to master:

     git switch stable
     git rebase master
     git push --force upstream stable
     git switch master

How should we address these steps as part of the semi-automated release? Ccing @pydata/xarray in case they want to chime in

@max-sixty
Copy link
Collaborator

I think both of those should be script-able as actions:

git merge upstream/stable

This could run on each commit to stable, opening a PR to master if required, I think

Update the stable branch (used by ReadTheDocs) and switch back to master:

This could run on each tag, like the push to PyPI.

There's one action of "update the release tag in RTD" which may still be manual

(though no need to do them now, I generally have tried to make the process 25% easier each release, I think these incremental PRs are ideal)

.github/workflows/pypi-release.yaml Outdated Show resolved Hide resolved
.github/workflows/pypi-release.yaml Outdated Show resolved Hide resolved
.github/workflows/pypi-release.yaml Show resolved Hide resolved
.github/workflows/pypi-release.yaml Outdated Show resolved Hide resolved
@andersy005
Copy link
Member Author

I think both of those should be script-able as actions:

git merge upstream/stable

This could run on each commit to stable, opening a PR to master if required, I think

Update the stable branch (used by ReadTheDocs) and switch back to master:

This could run on each tag, like the push to PyPI.

There's one action of "update the release tag in RTD" which may still be manual

(though no need to do them now, I generally have tried to make the process 25% easier each release, I think these incremental PRs are ideal)

👍🏽 for addressing these in separate PRs

@dopplershift
Copy link
Contributor

@andersy005 I'm curious, why do you go with multiple jobs within the workflow, and using artifacts to transfer state between them, rather than multiple steps in a single job?

@andersy005
Copy link
Member Author

@andersy005 I'm curious, why do you go with multiple jobs within the workflow, and using artifacts to transfer state between them, rather than multiple steps in a single job?

I have a tendency to split a workflow into multiple jobs because it makes reasoning about the workflow easy (at least for me :)) . However, I think using a single job here would reduce overhead since the logic isn't complex to warrant a need for multiple jobs...

@keewis
Copy link
Collaborator

keewis commented May 3, 2021

if it's possible, it might still make sense to create and test the wheels for uploaded v0.x.y tags in a job and only upload to PyPI if a release was created through the webpage (which would save us from re-creating a release if the build failed).

@alexamici alexamici mentioned this pull request May 5, 2021
13 tasks
@alexamici
Copy link
Collaborator

@andersy005 are you OK to try and merge this "as is" sometime today and then help us making a couple of tests on test.pypi.prg before doing the real release tomorrow?

(worst case: we release the old way)

@keewis
Copy link
Collaborator

keewis commented May 5, 2021

from #5232:

@kmuehlbauer

Why not always upload pushes to master to test.pypi.org?

@andersy005, what do you think? This should be pretty easy if we have separate jobs for building / testing / uploads (where the upload to pypi is skipped unless a release was created)

@kmuehlbauer
Copy link
Contributor

This should be pretty easy if we have separate jobs for building / testing / uploads (where the upload to pypi is skipped unless a release was created)

I've doing this for a while now in one of my projects. Not sure if it will be used much, but users could easily install versions from test.pypi.org and check it. I'm not saying that pip-installing from github is too complex. Another aspect is, that you could even install the package again after upload to check that everything works. But that may be overkill.

@andersy005
Copy link
Member Author

andersy005 commented May 6, 2021

Here's the workflow visualization graph. Let me know if the current job dependency is okay...

Screen Shot 2021-05-06 at 6 23 35 AM

Also, someone with admin permissions on PyPI should make sure to get the necessary tokens from PyPI and TestPyPI and set them on this repo.

Comment on lines 80 to 86
- name: Publish package to TestPyPI
uses: pypa/gh-action-pypi-publish@v1.4.2
with:
user: __token__
password: ${{ secrets.TESTPYPI_TOKEN }}
repository_url: https://test.pypi.org/legacy/
verbose: true
Copy link
Member Author

@andersy005 andersy005 May 6, 2021

Choose a reason for hiding this comment

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

This will be triggered on every push event on the master branch and release tag... Should we constrain this to be triggered on push events on the master branch only?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should include tags but not releases (which are generated via the web page). That way, we can check the wheels on TestPyPI before creating the release and uploading to PyPI.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be resolved by the latest commit...

@keewis
Copy link
Collaborator

keewis commented May 6, 2021

should make sure to get the necessary tokens from PyPI and TestPyPI

I grabbed the xarray name on TestPyPI so TESTPYPI_TOKEN is already set

@alexamici
Copy link
Collaborator

alexamici commented May 6, 2021

Any consensus on whether this is ready to merge? We should start the release process shortly (once the US wakes up).

@keewis
Copy link
Collaborator

keewis commented May 6, 2021

I'd say let's resolve #5244 (comment) and #5244 (comment) first

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

great, looks good. I'd say let's merge and check whether the packages are uploaded correctly to TestPyPI (PYPI_TOKEN will have to be set by either @max-sixty, @dcherian, @jhamman, or @shoyer).

Edit: @andersy005, feel free to merge once you think it is ready

on:
release:
types:
- published
Copy link
Collaborator

@keewis keewis May 6, 2021

Choose a reason for hiding this comment

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

I usually use created here, but this probably doesn't make much of a difference since we don't use "draft releases" (and I can't seem to find a page which explains the difference between the "activity types")

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if you've seen this page already. There's this note:

Note: The prereleased type will not trigger for pre-releases published from draft releases, but the published type will trigger. If you want a workflow to run when stable and pre-releases publish, subscribe to published instead of released and prereleased.

@andersy005 andersy005 merged commit f5e4fd5 into pydata:master May 6, 2021
@andersy005 andersy005 deleted the pypi-release-via-github branch May 6, 2021 14:46
@keewis
Copy link
Collaborator

keewis commented May 6, 2021

it seems PyPI does not allow packages with versions like 0.17.1.dev108+gf5e4fd5f. I'm not sure if it's possible to have setuptools-scm create a different version. Should we run this only when a tag is created?

@keewis keewis mentioned this pull request May 6, 2021
1 task
@TomNicholas TomNicholas mentioned this pull request May 6, 2021
2 tasks
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.

6 participants