Update pyproject.toml & actions to poetry 2.x / PEP 621 #387
Conversation
dalito
force-pushed
the
poetry2-upgrade-contd
branch
3 times, most recently
from
de8fc05 to
9165dc9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
=======================================
Coverage 63.79% 63.79%
=======================================
Files 63 63
Lines 8951 8951
Branches 2587 2587
=======================================
Hits 5710 5710
Misses 2634 2634
Partials 607 607 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dalito
force-pushed
the
poetry2-upgrade-contd
branch
from
d1e81aa to
ed642f1
Compare
.github/dependabot.yml
Outdated
|
|
||
| version: 2 | ||
| updates: | ||
| - package-ecosystem: github-actions |
There was a problem hiding this comment.
Is there a plan to have dependabot manage python deps, too?
There was a problem hiding this comment.
Not that I am aware of. But I remember that Patrick mentioned another tool for updating the dependencies somewhere else (renovate?). I would suggest to do it step by step (so not in this PR).
There was a problem hiding this comment.
Dependabot currently supports poetry-mediated python dependency management via the pip configuration.
uv support has recently been announced.
I suggest adding in a config section for watching python deps since you're adding in GHA support in this PR.
dalito
force-pushed
the
poetry2-upgrade-contd
branch
from
b66f3c7 to
cd28a04
Compare
.github/workflows/pypi-publish.yaml
Outdated
|
|
||
| on: | ||
| release: | ||
| # Trigger the workflow only for real releases but no for draft releases |
There was a problem hiding this comment.
but not for draft releases (typo)
.github/workflows/pypi-publish.yaml
Outdated
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/[email protected] | ||
| # By default the ref (branch, tag or SHA) that triggered the workflow, |
There was a problem hiding this comment.
extremely pedantic 😁 : no need for the comma after "workflow"
.github/workflows/pypi-publish.yaml
Outdated
| # Check if correct tag is available | ||
| git describe --tags --abbrev=0 |
There was a problem hiding this comment.
what happens if the correct tag isn't available?
There was a problem hiding this comment.
Hmm. I just found out that it is surprisingly possible to create a release without selecting a tag on the release-creation page. So it may be safer to not rely on it and use the full checkout with tags to derive the version safely. I'll change this.
.github/workflows/pypi-publish.yaml
Outdated
| # Used actions: (updates managed by dependabot) | ||
| # - https://github.com/actions/checkout | ||
| # - https://github.com/actions/setup-python | ||
| # - https://github.com/actions/upload-artifact | ||
| # - https://github.com/actions/download-artifact | ||
| # - https://github.com/pypa/gh-action-pypi-publish/ |
There was a problem hiding this comment.
-
Some files have this list at the top, some at the bottom. Can we standardise on a location?
-
Is this list useful if deps are managed by dependabot anyway?
There was a problem hiding this comment.
It is only useful for other developers. Dependabot does not need it. Maybe we should remove this info everywhere?
--> I removed it.
.github/workflows/pypi-publish.yaml
Outdated
| if: github.event_name == 'release' | ||
| uses: pypa/[email protected] | ||
| with: | ||
| user: __token__ |
There was a problem hiding this comment.
does this get substituted in from somewhere?
There was a problem hiding this comment.
I just copied the code from the former version. __token__ is supplied by the pypi-publish action https://github.com/pypa/gh-action-pypi-publish/?tab=readme-ov-file#specifying-a-different-username
But based on the docs the line user: __token__ is not be needed. I remove it.
.github/workflows/test-upstream.yaml
Outdated
| with: | ||
| # don't specify repository like this or else we won't get pull request branches correctly | ||
| # repository: linkml/linkml-runtime |
There was a problem hiding this comment.
this looks like cruft - can it be removed?
.github/workflows/test-upstream.yaml
Outdated
|
|
||
| - name: Load cached venv | ||
| id: cached-poetry-dependencies | ||
| uses: actions/cache@v3 | ||
| uses: actions/cache@1bd1e32a3bdc45362d1e726936510720a7c30a57 |
There was a problem hiding this comment.
Why do some actions use specific commits but others use versions? Can we standardise?
| packages = [ | ||
| { include = "linkml_runtime" } | ||
| include = ["linkml_runtime"] | ||
| requires-python = ">=3.9" |
There was a problem hiding this comment.
do we need to add an upper bound?
There was a problem hiding this comment.
Not for requires-python, see python-poetry/poetry#10136
| run: pipx install poetry==1.3.2 | ||
| run: | | ||
| pipx install poetry | ||
| pipx inject poetry poetry-dynamic-versioning |
There was a problem hiding this comment.
don't need the dynamic versioning plugin unless you're releasing a package to pypi
There was a problem hiding this comment.
Your comment made me look at the action in detail: I believe it does not do what it should do. My understanding is that it should run the tests with updated dependencies but it just installs from the current (possibly outdated) lock file. Then it removes the lockfile but does not create an updated one before running the tests. So the tests are exactly the same as when run from main.yaml.
Would you agree that the idea here is to update the lock file first and then test? In this case dynamic-versioning would be needed for the dependency update.
I'll update the logic.
There was a problem hiding this comment.
@sierra-moxon created the action, but accidentally removed the re-installation with updated dependencies in 0579243
There was a problem hiding this comment.
I also noticed that this action seems to duplicate a lot of the main.yaml actions so it may be time for a general GHA overhaul -- these actions could be written more efficiently and it's likely that some can be generalised and served organisation-wide.
@sierra-moxon would have to comment on the purpose of the action. IMO dependency updates should be done by developers, with dependabot, renovate, or something similar to alert devs to when updates are needed. I would not use a GHA to test whether the code still functions with all deps updated to the latest version because it doesn't actually do anything with the information -- if the tests fail, there's no prompt to limit versions and if newer deps are available, there's nothing to prompt devs to update the deps.
|
LGTM! Unfortunately I don't have write access to be able to approve this for merging, though. |
This is a clean start based on #364
pyproject.tomlto PEP621 style for poetry 2.xpypi-publishaction to follow the suggested 2-step process of building in one job and publishing in anotherpypi-publishaction for trusted publishing; it is commented out now since you need to change the configuration in PyPI as well to enable it. Note that the env name is notpypias in the screenshots of the PyPI-docs butpypi-release.Part of the work on linkml/linkml#2615
Closes linkml/linkml#2520