Skip to content

CI: add reference lowest env solution + script to check its evolutions - #19022

Merged
pllim merged 4 commits into
astropy:mainfrom
neutrinoceros:ci/check-tree
Mar 10, 2026
Merged

pllim merged 4 commits into
astropy:mainfrom
neutrinoceros:ci/check-tree

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented Dec 3, 2025 •

Copy link
Copy Markdown
Contributor

Description

based off #18955
close #18965

There are several options for "locking" the resolved environment:

  1. generate a lock file (e.g. pylock.oldest.toml)
  2. store the output of pip freeze or pip list
  3. store the output of uv tree

both 1) and 2) have portability issues because the exact result would be platform dependent, so I went with 3) instead, which can be made cross-platform with a --universal resolution strategy.
Option 3) also comes with what I think is a great benefit: it makes the relationship between all our dependencies apparent, immediately answering mundane but otherwise time consuming questions like "where does this one even come from ?". Also, storing this graph in a legible form helps demystifying dependency hell, which is a nice side effect.

I still need to add the CI job to actually run the script, but the script itself should already be stable.
I intentionally introduced a mistake in the reference file to make sure CI fails as expected when I do plug it in.
I plan to finish this tomorrow.
note to self: I want to run the script through pipx, not uv, just to be absolutely sure that it doesn't use a system level install of uv.

  • 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 Dec 3, 2025
@neutrinoceros neutrinoceros added no-changelog-entry-needed dev-automation dependencies Pull requests that update a dependency file labels Dec 3, 2025
@github-actions

github-actions Bot commented Dec 3, 2025

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

Copy link
Copy Markdown
Contributor Author

Hum, CI is hitting another edge cache around the astropy[all]<->asdf-astropy circular dependency:

creating virtual environment...
installing uv~=0.9.15...
Using CPython 3.14.0 interpreter at: /opt/hostedtoolcache/Python/3.14.0/x64/bin/python3
  × No solution found when resolving dependencies for split (markers:
  │ python_full_version < '3.12'):
  ╰─▶ Because asdf-astropy==0.9.0 depends on your project and only the
      following versions of asdf-astropy are available:
          asdf-astropy<=0.7.0
          asdf-astropy==0.7.1
          asdf-astropy==0.8.0
          asdf-astropy==0.9.0
      we can conclude that asdf-astropy>0.8.0 depends on your project.
      And because asdf-astropy>=0.7.0,<=0.8.0 depends on your project, we can
      conclude that asdf-astropy>=0.7.0 depends on your project.
      And because astropy[all] depends on asdf-astropy>=0.7.0 and your project
      requires astropy[all], we can conclude that your project's requirements
      are unsatisfiable.

      hint: The package `asdf-astropy` depends on the package `astropy` but
      the name is shadowed by your project. Consider changing the name of
      the project.

      hint: While the active Python version is 3.14, the resolution failed for
      other Python versions supported by your project. Consider limiting your
      project's supported Python versions using `requires-python`.

Traceback (most recent call last):
  File "/home/runner/work/astropy/astropy/scripts/check-lowest-resolved-tree.py", line 99, in <module>
    raise SystemExit(main())
                     ^^^^^^
  File "/home/runner/work/astropy/astropy/scripts/check-lowest-resolved-tree.py", line 63, in main
    new_tree = generate_tree(cli_args.debug)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/astropy/astropy/scripts/check-lowest-resolved-tree.py", line 28, in generate_tree
    raise AssertionError("call to uv command failed")
AssertionError: call to uv command failed

I don't know why I'm not seeing this locally. Will try to solve this after lunch.

@neutrinoceros
neutrinoceros force-pushed the ci/check-tree branch 3 times, most recently from 25a9a3c to 8e22f4e Compare December 4, 2025 13:33
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

I'm not convinced the error message even makes sense. I don't see why having a dependency that itself depends on the project should make requirement unsatisfiable (it this was true, dependency cycles wouldn't be allowed at all ?). I will take this upstream for now.

@neutrinoceros

neutrinoceros commented Dec 4, 2025 •

Copy link
Copy Markdown
Contributor Author

reported at astral-sh/uv#16978

update: my mistake. Resolved locally.

@neutrinoceros
neutrinoceros force-pushed the ci/check-tree branch 5 times, most recently from 1b7c5c6 to a7a4553 Compare December 4, 2025 17:55
@neutrinoceros neutrinoceros removed the Upstream Action Required Was: Upstream Fix Required label Dec 4, 2025
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Interesting, now I'm getting what looks like a genuine failure that I seemingly cannot reproduce locally.

@neutrinoceros

neutrinoceros commented Dec 5, 2025 •

Copy link
Copy Markdown
Contributor Author

A, maybe I got it. appnope is special because, it isn't just a (twice) transitive dependency, it's also platform dependent: ipykernel only requires it for platform_system=="Darwin" (i.e., macOS). I do happen to work on two macOS systems, and CI is on ubuntu. It should not make a difference because I'm explicitly requiring a --universal solution, but maybe that's just not enough to guarantee reproducible and cross-platform resolutions. I'll try to switch the os in CI to confirm or refute this working hypothesis first.

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Turned out to be a red herring. Instead, the problem was I had a residual, botched uv.lock locally, which uv used instead of re-computing the graph from scratch. Removing this broken lockfile solved the problem: I was able to align my local system with CI, and update the reference solution accordingly.

@neutrinoceros neutrinoceros changed the title CI: add reference lowest env solution + script to check its evolutions CI: add reference lowest env solution + script to check its evolutions (blocked by #18955) Dec 5, 2025
@neutrinoceros neutrinoceros changed the title CI: add reference lowest env solution + script to check its evolutions (blocked by #18955) CI: add reference lowest env solution + script to check its evolutions Dec 5, 2025
@neutrinoceros
neutrinoceros marked this pull request as ready for review December 5, 2025 14:56
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

rebased to resolve a merge conflict

@Cadair, do you think you could have another look at this reasonably soon ?

@neutrinoceros
neutrinoceros force-pushed the ci/check-tree branch 5 times, most recently from 46b32b5 to cbc9984 Compare March 9, 2026 17:20
@neutrinoceros

Copy link
Copy Markdown
Contributor Author

I finally got RTD green.
@pllim I see your review is still marked as "changes requested", but I think I addressed everything so far. Is there anything I missed ?

pllim
pllim previously requested changes Mar 9, 2026

@pllim pllim left a comment

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.

I probably blocked merge initially to await more maintainers to chime in but since there are now 2 approvals, I guess we can merge and see what happens. But before that, I have some comments.

Also if you all want to think about ways this could compromise GHA security, now is the time to do it. Recent attacks: https://www.stepsecurity.io/blog/hackerbot-claw-github-actions-exploitation

Comment thread .github/workflows/ci_workflows.yml Resolved
Comment thread scripts/check-lowest-resolved-tree.py Resolved
Comment thread scripts/check-lowest-resolved-tree.py Resolved
Comment thread scripts/check-lowest-resolved-tree.py Resolved
@@ -0,0 +1,467 @@
astropy
├── astropy-iers-data v0.2026.2.23.0.48.33

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.

Is this going to have to be updated every week?

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.

yes. But I'm planning to automate it in #19380 :)

@neutrinoceros neutrinoceros Mar 10, 2026 •

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.

(it'll be as simple as running the script before changes are committed)

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.

@astrofrog do you have any concern here with the answers above?

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.

No all good

@neutrinoceros

Copy link
Copy Markdown
Contributor Author

Also if you all want to think about ways this could compromise GHA security, now is the time to do it.

I'm not a security expert so evidently this isn't worth much but I cannot forsee how this would create an attack vector. I do try to stay as paranoid as my mental capacity allows though 😄

@pllim

pllim commented Mar 10, 2026

Copy link
Copy Markdown
Member

Thanks for addressing my comments. I think we just waiting on #19022 (review) now from Tom R.

@astrofrog

astrofrog commented Mar 10, 2026 •

Copy link
Copy Markdown
Member

Don't have time to review in detail myself but if there are two approvals that's fine by me

@pllim
pllim dismissed their stale review March 10, 2026 17:06

Addressed

@pllim
pllim merged commit 9f82dd4 into astropy:main Mar 10, 2026
34 of 35 checks passed
@neutrinoceros
neutrinoceros deleted the ci/check-tree branch March 10, 2026 17:11
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/DEP: guarding against stealth upgrades in oldestdeps CI

5 participants