Repository navigation
MNT: replace fragile auto-update script with uv remove/uv add routine - #19380
Conversation
neutrinoceros
added
Enhancement
no-changelog-entry-needed
dev-automation
dependencies
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
66163b0 to
b0335a4
Compare
|
I should note explicitly here that I'm blocking this on #19022 |
|
#19022 is merged. Please rebase. Thanks. |
647e770 to
b98da42
Compare
b98da42 to
b11c872
Compare
|
@pllim is there anyone in particular we might want to ask for review ? merging this before the next run of this workflow would minimize friction |
|
Tom R hopefully |
| - run: | | ||
| uv remove astropy-iers-data --no-sync | ||
| uv add astropy-iers-data --no-sync | ||
| uv run scripts/check-lowest-resolved-tree.py --overwrite |
There was a problem hiding this comment.
I might be missing something, but how does this update the minimum pin in pyproject.toml?
There was a problem hiding this comment.
yes. But to be clear, it's not this exact line that does it. I'm taking advantage of the fact that uv adding a (new) dependency without an explicit lower bound automatically sets one at the current latest version. uv remove is called right before that to simulate astropy-iers-data being a "new" dependency, as far as uv add is concerned.
There was a problem hiding this comment.
Ok if this is the case, please be sure to add a comment about this. Also to be clear, are we sure this is not actually more fragile than before? Does uv add guarantee that it will always set the lower bound automatically? (or is there a flag we can use to explicitly opt in in case they ever change the defaults?)
There was a problem hiding this comment.
I'll add a comment.
Does uv add guarantee that it will always set the lower bound automatically?
yes:
- https://docs.astral.sh/uv/concepts/resolution/#lower-bounds
- https://docs.astral.sh/uv/reference/settings/#add-bounds
in short: dependencies without lower bounds tend to trigger warnings in uv's installer when env resolution is set to lowest or lowest-direct, so there's a strong incentive for its devs to preserve this behavior for the sake of consistency, if nothing else.
Additionally, changing this behavior would be much more dirsuptive than most (if not all) of the breaking changes they've allowed thusfar, so I'm not worried.
is there a flag we can use to explicitly opt in in case they ever change the defaults?
The default behavior corresponds to --bounds lower, but as explained, I'd be less worried about astral changing the default behavior, and more about giving the impression, to anyone who'll read these lines in the future, that passing this argument is necessary.
I can do it and ensure my comment mentions how it's just hard-coding a default behavior, if you like.
There was a problem hiding this comment.
Yes I think just making it explicit and commenting that it's not really needed but just a safeguard against future changes would be ideal
b11c872 to
9cbad42
Compare
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
MInor nit, but looks good. Once you apply the suggestion feel free to merge
Co-authored-by: Thomas Robitaille <[email protected]>
Description
I wanted to improve this script's stability, using a proper dependency manager instead of manual str manipulation, and found that
uv remove+uv addwas a simple way to achieve the same effect, so the script can be removed altogether.At the time of opening there's a small catch however: the
remove/addroutine doesn't seem to preserve comments from nearby lines (specifically, aroundpyerfa's requirement). It shouldn't be much of an issue, since this one comment is temporary anyway and probably should have it's how ticket. It is likely a bug in the rust toml library used by uv.