Conversation
6b699e7 to
be39717
Compare
| - name: Install dependencies | ||
| run: make install-dev | ||
| run: | | ||
| echo ${{ matrix.python-version }} > .python-version |
There was a problem hiding this comment.
this controls which python environment is installed in the following step.
| rm -rf test/output/ | ||
|
|
||
| build: | ||
| uv build |
There was a problem hiding this comment.
this generates both wheel and sdist already
in other words, there is no need to be explicit with --wheel and --sdist as is done in the workflow (pyproject.toml defines these defaults already)
|
@gauravrparikh ready for review. (used what I learned there to improve the workflow here with even more tightly scoped permissions by putting publish into its own step) |
| "wheel" | ||
| [project] | ||
| name = "pacmap" | ||
| version = "0.8.1" |
There was a problem hiding this comment.
| version = "0.8.1" | |
| version = "0.8.2rc0" |
I suggest trying to create a release against this branch (or perhaps master after merge) with this same tag number, after setting up trusted publishing.
|
|
||
| on: | ||
| pull_request: | ||
| branches: [ master, dev ] |
There was a problem hiding this comment.
| branches: [ master, dev ] | |
| branches: [ master ] |
There was a problem hiding this comment.
can't hurt, can it?
There was a problem hiding this comment.
can't hurt, can it?
you mean, "it can't hurt [to keep it there]"? i guess not 👍
I'll assume you use it in a personal workflow or something, or aspire to propose a dev branch. regardless, if you'd prefer not to remove, it's the sort of thing I try not to care about splitting hairs over :)
Makefile
Outdated
| requirements-test.txt: | ||
| uv pip compile --group dev pyproject.toml -o requirements-test.txt | ||
|
|
||
| requirements.txt: | ||
| uv pip compile pyproject.toml -o requirements.txt | ||
|
|
||
| install: requirements-test.txt | ||
| uv sync |
There was a problem hiding this comment.
I think I'm just unfamiliar with that's happening here. Can you explain why we're using requirements-test.txt and requirements.txt that are auto-generated from uv? Is it just to maintain backward compat for ppl who don't want to use uv? I previously assume uv.lock kinda replaced these methods.
Maybe you could explain to me via PR comment, and I can use your explanation to write inline comments to the makefile that would have helped me understand :)
There was a problem hiding this comment.
good question. yes, uv.lock replaces them. few reasons.
- primarily bwd compatibility (there was a set of these files before, this improves their utility by making them true frozen requirements files)
- if someone doesn't want to use
uvbut for some reason does want to run the test suite (best reason) currently,dependabotcan't scanuv.lockorpyproject.toml- so having these files present allows for security scans to occur which effectively check on what's inuv.lock- as a form of documentation in and of itself: "to create this file, use this syntax"
technically it should be the following, though (so if one changes, so does the other):
| requirements-test.txt: | |
| uv pip compile --group dev pyproject.toml -o requirements-test.txt | |
| requirements.txt: | |
| uv pip compile pyproject.toml -o requirements.txt | |
| install: requirements-test.txt | |
| uv sync | |
| requirements-test.txt: pyproject.toml | |
| uv pip compile --group dev pyproject.toml -o requirements-test.txt | |
| requirements.txt: pyproject.toml | |
| uv pip compile pyproject.toml -o requirements.txt | |
| install: | |
| uv sync |
but yeah, technically speaking, these can be removed.
There was a problem hiding this comment.
I'm all for keeping them for backward compat, especially if core maintainers aren't quite sold on uv yet :)
- (best reason) currently,
dependabotcan't scanuv.lockorpyproject.toml- so having these files present allows for security scans to occur which effectively check on what's inuv.lock
but the "dependabot" rationale feels less strong. it is a bit neglected by github imho and highly locked to github infra, whereas renovate is open source and multi-platform (and is officially what uv recommends)
https://docs.astral.sh/uv/guides/integration/dependency-bots/
https://docs.renovatebot.com/bot-comparison/
There was a problem hiding this comment.
In the end, your call -- no objection, and all this can be changed later quite easily!
There was a problem hiding this comment.
that's a good point.
how this works:
when you create a tag (via release on github, for example) - this workflow will trigger publishing the version listed under
pyproject.toml. you can test that it would work (including inspecting the built artifacts) via the github action by creating PRs tomasterordev. this will build and upload the artifacts to github actions (but not pypi), allowing you to catch errors early.for example, see here: https://github.com/YingfanWang/PaCMAP/actions/runs/15859852088?pr=105 - you can download the artifacts and verify them yourself, if you so choose.
setting up trusted publishing:
pypiunder thepacmapmanagement (settings) and click on "Publishing"YingfanWang) under Ownerpacmapunder repository namepublish.ymlunder workflow namerelease-devunder environment namesetting up github environment:
for best isolation, go to this repo's settings, and under
Environmentscreate a new environment namedrelease-dev(matching the one in the workflow).this will allow you to have granular control over the publishing environment, without interfering with the "test that the build step works" part.
other changes:
uvto manage environment (automatically generate requirements files with a know set of resolved versions). this is not forced on the users, but provided as convenience / used in the workflows (big speed up in installation + extra guarantees of stability viauv.lock)