build: improve opt deps

[nstarman]

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]

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]

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

[astrofrog]

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

[pllim]

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

[nstarman]

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]

have all be a subset of test_all

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

[astrofrog]

Yes probably makes more sense

[eerovaher]

test_all has already been discussed, but I don't think all dependencies should contain docs dependencies either.

[nstarman]

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]

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!

[mhvk]

Actually, given our discussion over in #16351, perhaps we shouldn't have "hidden" dependencies - code has a dependence on whether ipython is installed in utils.console.

But possibly that is best done separately - here, what we list is the dependencies needed for all astropy functionality.

Anyway, am happy either with this as-is, or with ipython put back.

[neutrinoceros]

#16354 👀

[pllim]

Let's leave it then instead of deleting.

[eerovaher]

Any dependency we have should be listed exactly once, but it would indeed be better to keep ipython here and to delete the duplicate from the test_all set instead.

[eerovaher]

The first comment is useful (because it mentiones "run-time"), the second comment is not. The useless comment isn't added by this pull request, but several analogous and equally useless comments are being introduced that should all be cleaned up.

Suggested change

	"astropy[all]", # installs all optional run-time dependencies
	"astropy[test]", # installs the [test] dependencies
	"astropy[all]", # installs all optional run-time dependencies
	"astropy[test]"

[eerovaher]

These new installation options should be (briefly) introduced somewhere in the developer documentation.