Skip to content

EXP: make oldestdeps CI reproducible#19000

Merged
mhvk merged 2 commits intoastropy:mainfrom
neutrinoceros:exp/fixup-lowest
Dec 5, 2025
Merged

EXP: make oldestdeps CI reproducible#19000
mhvk merged 2 commits intoastropy:mainfrom
neutrinoceros:exp/fixup-lowest

Conversation

@neutrinoceros
Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros commented Nov 26, 2025

Description

This includes #18955 and adds minimal changes to finalize #18782, including a workaround for #18992 (which I don't intend to get merged). This is meant as a demonstration of where the effort is at, and illustrates what's still missing to resolve #18782.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@neutrinoceros neutrinoceros added this to the v8.0.0 milestone Nov 26, 2025
@github-actions
Copy link
Copy Markdown
Contributor

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@neutrinoceros neutrinoceros changed the title EXP: make \oldestdeps\ CI reproducible EXP: make oldestdeps CI reproducible Nov 26, 2025
@neutrinoceros neutrinoceros force-pushed the exp/fixup-lowest branch 3 times, most recently from 76971f9 to 7d5c792 Compare November 30, 2025 13:41
@neutrinoceros neutrinoceros mentioned this pull request Dec 3, 2025
1 task
@neutrinoceros neutrinoceros marked this pull request as ready for review December 5, 2025 14:57
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

@mhvk just undrafted this. The diff is now extremely small, and the goal is simply closing #18782, but please do let me know if you need any other detail.

Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super! This looks good to me, except that I think for our future selves it would be helpful to ensure the reason for things are explained in comments (see in-line).

Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, let's get this in.

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Dec 5, 2025

p.s. The one advantage of tox is that it is (relatively) easy to get exactly what CI uses for tests... Anyway, whether we change (again) is a discussion for another time...

@mhvk mhvk enabled auto-merge December 5, 2025 16:04
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

The one advantage of tox is that it is (relatively) easy to get exactly what CI uses for tests...

I've said this already but I'll repeat as many times as needed: this is only true if the global state of the package index (PyPI) doesn't change between your CI run and your local run, and if you're running on the same (or comparable) platform.
The second condition could be emulated with a container (docker), but the first one can only really be addressed with lock files (which I'm still hoping the community can get behind, some day), and tox doesn't help with this.

@mhvk mhvk merged commit 2bd0902 into astropy:main Dec 5, 2025
34 checks passed
@neutrinoceros neutrinoceros deleted the exp/fixup-lowest branch December 5, 2025 16:55
@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Dec 5, 2025

In practice, it works reasonably well to reproduce test failures. Anyway, for another time. I'm sure I'm not the only one who basically copies the astropy/openastronomy setup, and then whenever things get too far out of whack, just makes updates that astropy did. Getting rid of tox in testing would now be a huge hassle (as it was when starting with it), so there have to be very strong reasons to do it (I also use spin for numpy, which has its own problems...).

@nstarman
Copy link
Copy Markdown
Member

nstarman commented Dec 5, 2025

There is also https://github.com/tox-dev/tox-uv to enable lock file support.

@neutrinoceros
Copy link
Copy Markdown
Contributor Author

Getting rid of tox in testing would now be a huge hassle

IIUC the one thing we get from using it is ease to migrate between different CI systems, and considering the direction GitHub has been taking I wouldn't want to bet that another migration is implausible in the foreseeable future, so I'm definitely not proposing we "detox" the repo now of all times. Any way, I'm just blowing off accumulated steam, don't mind me !

There is also tox-dev/tox-uv to enable lock file support.

Hum, we've been using tox-uv for a over a year now (and this PR is actually all about updating a parameter from tox-uv) so let's say I'm aware 😅

# on oldestdeps, we let warnings be warnings (-Wdefault) instead of treated-as-errors,
# so we don't need to deal with filtering anything triggered from dependencies
# Other options replicate the static configuration from pyproject.toml, since they
# would otherwise be shaddowed by the env variable.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops belated comment on a typo:

shaddowed -> shadowed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks ! I can smuggle a typofix into my next PR updating this file, which I plan to open this weekend :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind I dropped this plan (no actual need for a PR). I'll just fix this one typo but will skip CI

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum actually if I skip CI the PR will just be unmergeable...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just let it be fixed by the next person who looks at it -- it is not like the text is incomprehensible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TST: towards reproducible builds for oldestdeps

4 participants