Skip to content

CI Upload to pypi with GitHub actions #18800

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 2, 2020
Merged

CI Upload to pypi with GitHub actions #18800

merged 4 commits into from
Dec 2, 2020

Conversation

thomasjpfan
Copy link
Member

Moved here from MacPython/scikit-learn-wheels#65

This PR adds a github action so we can upload with pypi through the interface.

Screen Shot 2020-09-15 at 3 09 20 PM

  1. One needs to add the PYPI_TOKEN secret to enable uploading to pypi. (I already added a TEST_PYPI_TOKEN for testpypi).
  2. One has to type in pypi in the "Run workflow" UI to upload to pypi, otherwise it will upload to testpypi by default.
  3. There is a check_artifacts.py script that makes sure that wheelhouse_uploader fetches the correctly number of artifacts based on the azure-pipeline.yml configuration.

@alfaro96 alfaro96 mentioned this pull request Nov 20, 2020
Merged
rth
rth approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but maybe @alfaro96 could have a look as well.

alfaro96
alfaro96 approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@alfaro96 alfaro96 left a comment

Choose a reason for hiding this comment

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

Thank you @thomasjpfan for your PR!

LGTM too.

Do you mind updating the doc/developers/maintainer.rst file to refer to the new PyPI uploading system?

ogrisel
ogrisel reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Let me do the following change:

@ogrisel
Copy link
Member

ogrisel commented Dec 1, 2020

@thomasjpfan out of curiosity how do you test github actions? I have a dedicated github fork of the scikit-learn repo where you forcepush your branch to the remote master?

@thomasjpfan
Copy link
Member Author

@thomasjpfan out of curiosity how do you test github actions? I have a dedicated github fork of the scikit-learn repo where you forcepush your branch to the remote master?

For most GitHub Actions, they are triggered when I push a branch to my fork.

The complicated one is the pull_request_target trigger, where I want the workflow to have access to secrets, but triggered by a pull request. For testing this, I would force push to master and open a pull request to the fork.

ogrisel
ogrisel approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge.

I tested it (without the secrets) on my fork. I will add the token for the pypi.org site once merge so that we can give it a try for the upcoming 0.24 release candidate.

@ogrisel ogrisel merged commit 41f3fd5 into scikit-learn:master Dec 2, 2020
@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

Do you mind updating the doc/developers/maintainer.rst file to refer to the new PyPI uploading system?

Forgot about this step will do a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@amueller amueller amueller approved these changes

@rth rth rth approved these changes

@alfaro96 alfaro96 alfaro96 approved these changes

Assignees
No one assigned
Labels
Build / CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasjpfan @ogrisel @amueller @rth @alfaro96