-
Notifications
You must be signed in to change notification settings - Fork 52
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 package build and upload to pypi for releases into ci/cd setup #71
base: main
Are you sure you want to change the base?
Conversation
This is just a suggestion btw. We don't have to include this in release v0.2.0, I just thought it might be nice since we have everything in place now to make it possible :) |
I think this is great, and I agree that we should put neural-lam on pypi. Perhaps it would make most sense to do this for the v0.3.0 release? The logic being that at the moment there are still things that are MEPS (region/data) specific, which does not really make sense in a general package that you install from pypi. In that way it is better to make sure all users end up in this repo and read about these limitations before installing. I see v0.3.0 as the step to a completely general setup, and then it makes sense to put up the package with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments and yes, it would be great to add the pypi installation instructions to the README as this would be the preferred way by a lot of users.
- uses: actions/checkout@v2 | ||
- uses: actions/setup-python@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the usual comments ;)
- uses: actions/checkout@v2 | |
- uses: actions/setup-python@v2 | |
- uses: actions/checkout@v4 | |
- uses: actions/setup-python@v5 |
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: actions/setup-python@v2 | ||
- uses: casperdcl/deploy-pypi@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not get away with the more common github action pypi-publish? pypi-publish has a lot more maintainers and a larger community making it more likely to provide updates. Alternative we could also use the publish action of some package manager like pdm publish
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, yes we can, I am just used to using this action and I am bit loathe change this since I won't know until we create the release whether I have made a typo. Unless you have already used pypi-publish
and have an example I can copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand. I recently have used this pdm publish
Describe your changes
Add ci/cd setup to build and upload releases created on github automatically to pypi.org
This would allow people to install releases of
neural_lam
directly withpip
frompypi.org
, i.e. to install releases ofneural_lam
with:I could also include a note in the README about being able to install releases of
neural-lam
from pypi, but I haven't yet.The change requires a pypi.org API key be added to the repository secrets (you can see its use here: https://github.com/mllam/neural-lam/pull/71/files#diff-8edc26411310cfa4356b22a2f31845a08e57cb9f418b3b802f0366a2630af992R22). I have create an API token under my pypi.org user, but I will transfer this to a group on pypi.org when the first package has been uploaded, and people can then join that group on pypi.org.
Issue Link
There is no issue to link to. We haven't discussed this before I don't think. But I thought it might make sense and be convenient since with #37 and #32 it is now possible to install
neural-lam
as a package.Type of change
Checklist before requesting a review
pull
with--rebase
option if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee