build: improve opt deps #16340

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

pyproject.toml

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!

pyproject.toml

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

pyproject.toml

+    "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

pyproject.toml

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


Suggested change

"typing_extensions>=4.0.0"

"typing_extensions>=4.0.0",

docs/install.rst

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.

docs/development/codeguide.rst

docs/development/testguide.rst

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

docs/development/testguide.rst

+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.

docs/install.rst

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

pyproject.toml

+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 <nstarman@users.noreply.github.com>

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?

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 pllim, eerovaher and WilliamJamieson 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

pyproject.toml Outdated Show resolved Hide resolved

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

mhvk approved these changes Apr 28, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

eerovaher reviewed Apr 29, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide 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 mhvk, eerovaher 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 <nstarman@users.noreply.github.com>

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
56 of 57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: improve opt deps #16340

build: improve opt deps #16340

nstarman commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited by nstarman

pllim commented Apr 25, 2024

astrofrog commented Apr 25, 2024 •

edited

pllim commented Apr 25, 2024

nstarman commented Apr 25, 2024

pllim commented Apr 25, 2024

astrofrog commented Apr 25, 2024

nstarman commented Apr 26, 2024 •

edited

mhvk commented Apr 27, 2024

mhvk left a comment

nstarman commented May 19, 2024 •

edited

neutrinoceros left a comment

neutrinoceros May 20, 2024

nstarman May 22, 2024

neutrinoceros May 20, 2024

nstarman May 22, 2024

eerovaher left a comment

eerovaher May 20, 2024

eerovaher May 20, 2024

neutrinoceros May 20, 2024

pllim May 21, 2024

nstarman May 22, 2024

nstarman May 22, 2024 •

edited

eerovaher May 23, 2024

neutrinoceros May 23, 2024 •

edited

nstarman May 23, 2024

eerovaher May 24, 2024

mhvk left a comment

pllim commented May 22, 2024

eerovaher left a comment

mhvk commented May 22, 2024

mhvk commented May 28, 2024

pllim commented May 28, 2024

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

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

build: improve opt deps #16340

build: improve opt deps #16340

Conversation

nstarman commented Apr 25, 2024 • edited

github-actions bot commented Apr 25, 2024 • edited by nstarman

pllim commented Apr 25, 2024

astrofrog commented Apr 25, 2024 • edited

pllim commented Apr 25, 2024

nstarman commented Apr 25, 2024

pllim commented Apr 25, 2024

astrofrog commented Apr 25, 2024

nstarman commented Apr 26, 2024 • edited

mhvk commented Apr 27, 2024

mhvk left a comment

Choose a reason for hiding this comment

nstarman commented May 19, 2024 • edited

neutrinoceros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerovaher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstarman May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

pllim commented May 22, 2024

eerovaher left a comment

Choose a reason for hiding this comment

mhvk commented May 22, 2024

mhvk commented May 28, 2024

pllim commented May 28, 2024

nstarman commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited by nstarman

astrofrog commented Apr 25, 2024 •

edited

nstarman commented Apr 26, 2024 •

edited

nstarman commented May 19, 2024 •

edited

nstarman May 22, 2024 •

edited

neutrinoceros May 23, 2024 •

edited