Skip to content

TST: avoid building indirect dependencies from the all extra via tool.uv.no-build-package#18949

Merged
pllim merged 1 commit intoastropy:mainfrom
neutrinoceros:tst/expand-build-blacklist
Nov 24, 2025
Merged

TST: avoid building indirect dependencies from the all extra via tool.uv.no-build-package#18949
pllim merged 1 commit intoastropy:mainfrom
neutrinoceros:tst/expand-build-blacklist

Conversation

@neutrinoceros
Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros commented Nov 16, 2025 •
edited
Loading

Description

A first line of defense against unnecessary builds when trying to install with --resolution=lowest.
A bunch of these can be upstreamed later (and I've started the process already), but merging this will immediately help towards #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 16, 2025
@neutrinoceros neutrinoceros added testing no-changelog-entry-needed dependencies Pull requests that update a dependency file labels Nov 16, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 16, 2025

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 16, 2025

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@neutrinoceros neutrinoceros mentioned this pull request Nov 16, 2025
Closed
@neutrinoceros neutrinoceros marked this pull request as ready for review November 16, 2025 08:17
@neutrinoceros neutrinoceros force-pushed the tst/expand-build-blacklist branch from 558087b to 67a5d62 Compare November 16, 2025 20:15
@neutrinoceros neutrinoceros mentioned this pull request Nov 17, 2025
Merged
1 task
eerovaher
eerovaher reviewed Nov 18, 2025
View reviewed changes
pyproject.toml
Comment on lines +753 to +754
"backcall", # can be removed once ipython<8.17 is dropped
"pickleshare", # can be removed once ipython<8.17 is dropped
Copy link
Copy Markdown
Member

@eerovaher eerovaher Nov 18, 2025

Choose a reason for hiding this comment

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

Why can't we drop support for ipython<8.17 right now?

Copy link
Copy Markdown
Contributor Author

@neutrinoceros neutrinoceros Nov 18, 2025

Choose a reason for hiding this comment

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

Well, we can do that separately, I just don't think this is in scope for this PR (and I'm trying to bump as little dependencies as possible in this process)

Copy link
Copy Markdown
Member

@eerovaher eerovaher Nov 18, 2025

Choose a reason for hiding this comment

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

These constraints are workarounds, but bumping the dependencies would solve the problem.

Copy link
Copy Markdown
Contributor Author

@neutrinoceros neutrinoceros Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

these constraints are temporary code that barely cost anything to maintain, and have no repercussions on users. Bumping optional dependencies is for ever and may incur a costs to users. I don't understand why temporary workarounds with clear expiration conditions should be avoided at all costs, or why we seem to be drawing diverging conclusions in face of what seems to me like a very imbalanced trade off.

pyproject.toml
Comment on lines +767 to +769
"psutil",
"pyzmq",
"tornado",
Copy link
Copy Markdown
Member

@eerovaher eerovaher Nov 18, 2025

Choose a reason for hiding this comment

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

Why do some packages have comments documenting when they can be removed from this list and others do not?

Copy link
Copy Markdown
Contributor Author

@neutrinoceros neutrinoceros Nov 18, 2025

Choose a reason for hiding this comment

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

I include comments for the ones I was able to figure out an exit strategy for. The IPython/Jupyter ecosystem is a very intricate and deep dependency graph on its own, and while I did give it a try, I wasn't able to find the exact conditions when this constraints can be lifted.

@pllim pllim added the Extra CI Run cron CI as part of PR label Nov 24, 2025
@pllim pllim force-pushed the tst/expand-build-blacklist branch from 67a5d62 to c98d887 Compare November 24, 2025 19:13
@pllim
Copy link
Copy Markdown
Member

pllim commented Nov 24, 2025 •
edited
Loading

Overall, I don't see how disallowing non-binary for more stuff would hurt, as long as this does not just pull them in blindly (doesn't, right?). I can approve after CI passes. I want to see the Extra CI also, just in case.

pyinstaller failure is unrelated (#18982).

pllim
pllim approved these changes Nov 24, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Specific concerns like bumping minversion of IPython should be a follow-up issue/PR, should you decide to pursue it.

Thanks, all!

@pllim pllim merged commit b687806 into astropy:main Nov 24, 2025
41 of 42 checks passed
@neutrinoceros neutrinoceros deleted the tst/expand-build-blacklist branch November 24, 2025 23:10
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Nov 24, 2025

as long as this does not just pull them in blindly (doesn't, right?).

Closing the loop: it doesn't!

@pllim
Copy link
Copy Markdown
Member

pllim commented Nov 25, 2025

it doesn't!

Yup, the minimal deps job looked clean. :)

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

Reviewers

@eerovaher eerovaher eerovaher left review comments

@pllim pllim pllim approved these changes

Assignees

No one assigned

Labels

dependencies Pull requests that update a dependency file Extra CI Run cron CI as part of PR installation no-changelog-entry-needed testing

Projects

None yet

Milestone

v8.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@neutrinoceros @pllim @eerovaher