-
Notifications
You must be signed in to change notification settings - Fork 299
TST/DEP: migrate CI installer from pip to uv, setup lockfile, dependency caching, and make pyproject.toml the one source of truth for test dependencies #5311
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
base: main
Are you sure you want to change the base?
Conversation
caed110 to
68c756a
Compare
|
bleeding-edge failures are unrelated, see yt-project/unyt#599 |
b073b25 to
eff6b4a
Compare
|
I'm struggling to adapt the sdist test. |
60567d4 to
7fe4846
Compare
| shell: bash | ||
| run: | | ||
| python -m pip install --no-build-isolation . | ||
| enable-cache: false |
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.
is there a particular reason to not enable the cache?
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 actually want to enable it in a dedicated PR, but that requires a bit of fiddling to do it right, so it's simpler to just preserve the existing no-cache strategy for now.
It also works best in the presence of a lock file, which is why I'm not bothering with it right now
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.
update: I added a lock file + caching everywhere except in a couple strategic places
- bleeding-edge CI: cache would rot too quickly to be worth anything
- wheels: using a cache for publishing creates a risk of a supply-chain attack via cache poisoning
18b3a02 to
d1164ab
Compare
362edd3 to
0b31d2b
Compare
|
|
||
| - name: Install and patch nosetest | ||
| if: matrix.test-runner == 'nose' | ||
| # note: this could be handled with [tool.uv.sources] |
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.
to add to this comment: the idea is we could host a fork of nose within our org, apply this patch, and use the fork as the source instead of PyPI. This would only work with uv but would arguably less of a hack. Or we could also try to use nose2 ...
a84b3fe to
09348a2
Compare
6f62b7a to
13da1c9
Compare
|
pre-commit.ci run |
1 similar comment
|
pre-commit.ci run |
1e29d20 to
640283c
Compare
cf24d3c to
e625503
Compare
e625503 to
3ea307f
Compare
3ea307f to
e3ff24d
Compare
chrishavlin
left a comment
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 did a quick read through to keep up with changes on this PR -- hoping to have some time to do a more thorough read next week. but left one minor comment :)
pyproject.toml
Outdated
| "sphinx-bootstrap-theme>=0.8.1", | ||
| "sphinx-rtd-theme>=1.3.0", | ||
| ] | ||
| geophysics = [ |
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'd replace geophysics with something more general and mapping-related since cartopy is used purely for 2d plotting and the term "geophysics" has broader meaning. Maybe just mapping ?
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.
sure. Done.
25c6b1b to
5153e77
Compare
b31e48e to
3df1e67
Compare
PR Summary
This is the first step towards #5310. I'm not adding a lock file yet, but I'm migrating existing CI to use uv instead of pip. From there, adding the lockfile will be a formality.update 2025-11-23: I expanded the scope of the PR, now I don't expect follow ups to be needed.
Close #5310
The one piece that's missing from the PR is automated updates for the lock file. Since Dependabot doesn't support uv.lock at the moment, I can't do this bit in PR, but I am experimenting with Mend.io's Renovate and plan to use it in the future here.update 2025/12/01: I was wrong. Dependabot already has support for uv.lock. I included the relevant piece of configuration.
PR Checklist