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

Feature/push to pypi workflow #147

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

desaibhargav
Copy link
Collaborator

This PR address #125 , and adds a GitHub Workflow for automatically pushing a tagged release to PyPI.

@Zach-Attach Zach-Attach self-requested a review October 10, 2024 16:23
Copy link
Collaborator

@Zach-Attach Zach-Attach left a comment

Choose a reason for hiding this comment

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

I really like this. Should be a huge time saver, but I see a few issues that need to be addressed.

.github/workflows/publish-to-test-pypi.yml Show resolved Hide resolved
python -m pip install setuptools==65.5.0 pip==21
- name: Install pypa/build
run: |
python3 -m pip install build --user
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this change from python to python3? Seems inconsistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, let's make it python3

publish-to-pypi:
name: >-
Publish Python 🐍 distribution 📦 to PyPI
if: startsWith(github.ref, 'refs/tags/') # only publish to PyPI on tag pushes
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is triggered by pushing a new tag? Does this mean we still need to push a new tag?
Should this not be triggered by a push to main instead? And then we can grab the version from src/nett/_version.py.

This is how I would imagine it:

  1. Check if this is a PR to main AND that the version found in src/nett/_version.py is not an existing release (denoted as v{version num}): If both are true, run the build to confirm it is buildable.
  2. If this is being pushed to main (from merging the PR): Start the publication process, resulting in it being tagged and published to pypi.

The particular if statement solution for seeing if it is being pushed to main or is just a PR for main can be found in the docs workflow, see https://github.com/buildingamind/NewbornEmbodiedTuringTest/blob/bf29953ab909ae216eb67a06fb9dbda281b9def1/.github/workflows/docs.yml#L3C1-L11C12

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest I don't have an opinion, I'm fine if we do this either way, but if what you have is already how docs work, it would make all the sense to have consistency between the two workflows.

uses: actions/setup-python@v5
with:
python-version: "3.10.12"
- name: Install specific setuptools and pip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor suggestion (feel free to ignore): Caching would greatly help with build times here. See: https://github.com/buildingamind/NewbornEmbodiedTuringTest/blob/bf29953ab909ae216eb67a06fb9dbda281b9def1/.github/workflows/docs.yml#L27C6-L42C47

Copy link
Collaborator

Choose a reason for hiding this comment

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

For bonus points, building this once and then using it for both docs.yml and publish-to-test.yml would be nice and efficient.

runs-on: ubuntu-latest
environment:
name: pypi
url: https://pypi.org/p/nett-benchmarkse
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo:

Suggested change
url: https://pypi.org/p/nett-benchmarkse
url: https://pypi.org/p/nett-benchmarks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks

.github/workflows/publish-to-test-pypi.yml Show resolved Hide resolved
.github/workflows/publish-to-test-pypi.yml Show resolved Hide resolved
@Zach-Attach
Copy link
Collaborator

Just looking at this. Do you think there are too many checks? Also, these names are kinda confusing lol

@desaibhargav
Copy link
Collaborator Author

I've made some changes, and I'm testing out the workflow file with testpypi, I'll let keep this updated

@desaibhargav
Copy link
Collaborator Author

Just looking at this. Do you think there are too many checks? Also, these names are kinda confusing lol

Yeah I think so too, but they are doing useful stuff so I'll let it be for now. I want to have a version of this working and then we can move things around

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