build: improve opt deps by nstarman · Pull Request #16340 · astropy/astropy

nstarman · 2024-04-25T15:19:31Z

Small improvements to the specification of optional dependencies. In particular, this improves astropy[all] to actually have everything and installs pre-commit for astropy[test] since our CI consists of 1. pre-commit checks, e.g. yaml verification, and 2. pytest-powered stuff.

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.

github-actions · 2024-04-25T15:20:00Z

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.

pllim · 2024-04-25T15:50:01Z

Looks reasonable but let me run the other jobs, just in case. Thanks!

astrofrog · 2024-04-25T15:57:09Z

Should all actually include test_all? I think of it as meaning all runtime dependencies? Not a strong opinion though.

pllim · 2024-04-25T16:15:45Z

Good point... There is a distinction between what users need and what testers need.

nstarman · 2024-04-25T17:50:03Z

As in I should reverse the relationship and have all be a subset of test_all, not test_all a subset of all ?

all = [...] # doesn't have test_all
test_all = ["astropy[all]", ...]

pllim · 2024-04-25T17:51:36Z

have all be a subset of test_all

I think this makes more sense. What do you think, @astrofrog ?

astrofrog · 2024-04-25T17:54:05Z

Yes probably makes more sense

nstarman · 2024-04-26T16:21:01Z

It would be nice to have an "absolutely everything" category, which makes getting setting up astropy dev environments super simple. I was making all into that, but it's AOK to keep that as all runtime dependencies. What do people think about a dev category that contains all dependencies -- runtime, static, docs, testing ?

@pllim @eerovaher @astrofrog

mhvk · 2024-04-27T13:41:55Z

Agreed that I always thought of all as getting meaning I can now run absolutely every astropy function, not that it includes testing or building documentation. Hence, all being part of test_all makes more sense to me, just like astropy[recomended] is part of docs.

I like the idea of a dev dependency that includes all needed to start development, but slightly orthogonal here. E.g., not quite clear that needs all of astropy's optional dependencies (I guess that would be dev_all...)

mhvk

See my one remaining comment, which mostly means another reason to leave this open for a little longer so others can comment.

But I think this little reorganization makes good sense now!

nstarman · 2024-05-19T22:10:13Z

Given the merge conflict I took this opportunity to add code comments and some discussion in the docs. I also added the [ipython] and [jupyter] optional dependency categories to group the newly added dependencies (the source of the merge conflict). Lastly, I put the run-time groups at the top.

neutrinoceros

Just a couple of extremely minor suggestions

neutrinoceros · 2024-05-20T07:35:39Z

+    "astropy[test]",
+    # Install all remaining dependencies
+    "objgraph",
+    "coverage[toml]",


shouldn't this line be in [test] ? I also note that it's actually already there in a way (pytest-astropy requires pytest-cov which itself requires coverage[toml])

neutrinoceros · 2024-05-20T07:36:20Z

+    "array-api-strict",
+]
+typing = [
+    "typing_extensions>=4.0.0"


Suggested change

"typing_extensions>=4.0.0"

"typing_extensions>=4.0.0",

eerovaher

I'm not convinced we need separate ipython and jupyter dependency sets.

It does look like it would be a good idea to rewrite the astropy development version installation instructions and the dependency descriptions, but doing that properly is beyond the scope of this pull request.

eerovaher · 2024-05-20T13:33:02Z

+To test the full set of optional dependencies, use the ``test_all`` option::
+
+    python -m pip install --editable ".[test_all]"


It would be much better if these descriptions could link to relevant subsections in the description of dependencies, but that would require those subsections to exist. It looks like all the installation instructions (including the installation instructions for building documentation) could benefit from being rewritten, but that's going beyond the scope of this pull request and is best done in a separate pull request instead.

eerovaher · 2024-05-20T13:35:08Z

+jupyter = [  # these are optional dependencies for `utils.console`
+    "astropy[ipython]",
+    "ipywidgets",
+    "ipykernel",
 ]


Does this install jupyter?

I don't think it does. But I'm also not sure it should: jupyter comes in different shapes (jupyterlab and notebook) and we don't need to impose a choice onto our users (I think ?)

Something seems to be pulling in jupyter stack but it is already that way in main. Hmm!

2024-05-19T22:19:37.9405799Z jupyter_client==8.6.1 2024-05-19T22:19:37.9406161Z jupyter_core==5.7.2 2024-05-19T22:19:37.9406568Z jupyterlab_widgets==3.0.10

This is just moving around the dependencies, not adding any. I titled it jupyter since it's for use when in a Jupyter environment.

This is in contrast to the ipython dep, which enables functionality in astropy.utils.console within any ipython environment not just a Jupyter one.

Do you expect anyone to ever use the ipython and jupyter dependency sets?

Yes. Even if not publicly advertised, these things have a way of finding their users, in my experience.

For another PR, but I do think we should have a page detailing these install options. Like...

If you're using an IPYthon environment, use astropy[ipython]. This is further improved by astropy[jupyter] when in a Jupyter notebook. astropy[typing] improves static typing compatibility. To get everything do astropy[all].

It might indeed be the case that documentation could explain what these dependency sets would be good for, but the more dependency sets we have, the more documentation we need to explain them. At some point the more granular dependency sets will not be worth the effort of documenting them.

mhvk

Nice! Good to go with the remaining suggested changes from others as far as I'm concerned.

Signed-off-by: nstarman <[email protected]>

pllim · 2024-05-22T17:45:23Z

with the remaining suggested changes from others

Does this mean we're still seeking a couple of more approvals?

eerovaher

I'm still not convinced we need separate ipython and jupyter dependency sets, but listing the dependencies is definitely done much better in this pull request than on main.

mhvk · 2024-05-22T20:31:18Z

with the remaining suggested changes from others

Does this mean we're still seeking a couple of more approvals?

I thought the comments given by others had not yet been all addressed, so was hoping to see those addressed or dismissed.

mhvk · 2024-05-28T13:21:18Z

Looking at this again, I think there are actually no outstanding issues - and if there are, these can be dealt with later. So, let's get this in. Thanks, @nstarman, this is substantially nicer!

pllim · 2024-05-28T13:55:11Z

tl;dr -- Did you double check that tox.ini and the workflows in .github/workflows not need updating?

bsipocz · 2024-09-24T19:00:27Z

-    "ipykernel",
-    "ipython>=7.32",
-    "ipywidgets",
-    "pytest>=7.0",


Why has been pytest removed as a dependency here? The test runner uses it, so strictly speaking it's a genuine optional dependency.

nstarman added the no-changelog-entry-needed label Apr 25, 2024

nstarman added this to the v7.0.0 milestone Apr 25, 2024

nstarman requested review from WilliamJamieson, eerovaher and pllim April 25, 2024 15:19

github-actions Bot added the installation label Apr 25, 2024

pllim added Extra CI Run cron CI as part of PR Build all wheels Run all the wheel builds rather than just a selection labels Apr 25, 2024

eerovaher reviewed Apr 25, 2024

View reviewed changes

Comment thread pyproject.toml Resolved

nstarman requested review from eerovaher and mhvk April 28, 2024 01:24

mhvk approved these changes Apr 28, 2024

View reviewed changes

Comment thread pyproject.toml Resolved

eerovaher reviewed Apr 29, 2024

View reviewed changes

Comment thread pyproject.toml Resolved

Comment thread pyproject.toml Resolved

nstarman force-pushed the build-pre-commit-in-test branch from d94cd6a to 6887bbb Compare May 19, 2024 22:07

nstarman requested review from eerovaher, mhvk and neutrinoceros May 19, 2024 22:07

nstarman closed this May 19, 2024

nstarman reopened this May 19, 2024

neutrinoceros approved these changes May 20, 2024

View reviewed changes

eerovaher reviewed May 20, 2024

View reviewed changes

mhvk reviewed May 21, 2024

View reviewed changes

build: improve opt deps

7b84565

Signed-off-by: nstarman <[email protected]>

nstarman force-pushed the build-pre-commit-in-test branch from 4e54edc to 7b84565 Compare May 22, 2024 02:52

nstarman added 2 commits May 21, 2024 22:56

fix: from review suggestions

d27e522

mild rearranging

9f000ef

nstarman requested a review from eerovaher May 22, 2024 03:12

eerovaher reviewed May 22, 2024

View reviewed changes

mhvk merged commit c88f256 into astropy:main May 28, 2024

bsipocz reviewed Sep 24, 2024

View reviewed changes

pllim mentioned this pull request Oct 28, 2024

Clearer error when show_in_notebook is missing optional dependencies #17268

Merged

1 task

pllim mentioned this pull request Jul 3, 2025

DEP: redefine dev[_all] extras as PEP 735 Dependency Groups #18342

Merged

1 of 2 tasks

		To test the full set of optional dependencies, use the ``test_all`` option::

		python -m pip install --editable ".[test_all]"

Conversation

nstarman commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 25, 2024 • edited by nstarman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Apr 25, 2024

Uh oh!

astrofrog commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Apr 25, 2024

Uh oh!

nstarman commented Apr 25, 2024

Uh oh!

pllim commented Apr 25, 2024

Uh oh!

astrofrog commented Apr 25, 2024

Uh oh!

Uh oh!

nstarman commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Apr 27, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nstarman commented May 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

nstarman commented Apr 25, 2024 •

edited

Loading

github-actions Bot commented Apr 25, 2024 •

edited by nstarman

Loading

astrofrog commented Apr 25, 2024 •

edited

Loading

nstarman commented Apr 26, 2024 •

edited

Loading

nstarman commented May 19, 2024 •

edited

Loading

nstarman May 22, 2024 •

edited

Loading

neutrinoceros May 23, 2024 •

edited

Loading