Declare optional dependencies in extras_require by astrofrog · Pull Request #8198 · astropy/astropy

astrofrog · 2018-11-30T12:59:52Z

This does the following:

Separate testing and non-testing optional dependencies in the install docs
Declare all optional dependencies in an extras_require entry in setup.py - with this, users could get all dependencies by running pip install astropy[all]
Declare all testing dependencies in the test entry in extras_require. This would allow all those dependencies to be installed when doing pip install astropy[test]. Note that this is different from tests_require which is the one actually used to temporarily install the bare minimum testing dependencies when running tests.
Declare the docs dependencies in the docs entry - and RTD can recognize this
Remove the pip requirement files to avoid duplication
Update the minimum required version of Matplotlib to 2.0 (this is effectively what we supported but the docs claimed 1.5)
Modify one of the Travis builds to pip install dependencies instead of using ci-helpers

At the moment, this has a small performance impact on Travis - the build I modified took 14 min 37 sec in the last successful run before this PR, and 15 min 58 sec now. I think this is because astropy is built twice, once in the pip install command, and the second time from the python setup.py test command, so I need to look into whether we can avoid this. Part of it could also be because we now run 12638 tests in this build instead of 12123, presumably because not all optional dependencies were included in CONDA_ALL_DEPENDENCIES.

As a side goal, I want to see how fast installing dependencies with pip is, as if it is quite fast then we could consider actually using pip to install dependencies in most builds instead of conda, and we could also avoid declaring all dependencies in CI config files (instead relying on the minimal/full configurations available in setup.py - that is, some builds could use the all extras_require`` and some not).

This is an experimental PR that I hope to find time to discuss next week, so do not merge!

drdavella · 2018-11-30T13:41:01Z

For consistency should tests_require contain the same dependencies as extras_require['test']? Presumably, tests_require is going to be less useful as the community moves away from using setup.py as a tool entry point.

astrofrog · 2018-11-30T13:43:02Z

@drdavella - I'm not sure yet, because some of the test dependencies are optional, so I don't want to always force people to temporarily install those. Maybe we should have a testall extras_require which has all the optional test dependencies, and then test can match tests_require?

drdavella · 2018-11-30T13:57:34Z

Ah, makes sense. Maybe at the very least we can just say that extras_require['test'] = tests_require + the_kitchen_sink.

saimn · 2018-11-30T21:14:27Z

I was thinking about this when playing with tox, but I'm not sure about the 'all' dependencies bundle, which represents a lot of things. It could make sense to have a finer split, maybe by sub-package, 'io', 'table', 'stats', 'viz' ? I think these can be combined in the pip command. But I don't know where to put scipy...

bsipocz · 2018-11-30T21:37:00Z

But I don't know where to put scipy...

in that framework I suppose on package can be specified in several points. at least scipy should go into multiple ones

saimn · 2018-11-30T21:49:58Z

in that framework I suppose on package can be specified in several points. at least scipy should go into multiple ones

That may be an option, but I think if if you specify multiple extra to pip then it will complain about the duplicate dependencies. Something to check.

bsipocz · 2018-11-30T21:51:27Z

Then it's a feature request for them to do a set before complaining for things like this ;)

astrofrog · 2018-11-30T23:12:24Z

It doesn't actually take long to install all though, so I'm not sure if it's really important to split them up. Especially if duplicate dependencies are going to cause a problem. I think regardless of whether we add more fine-grained extras_requires, we do need an 'all' one that just installs everything.

astrofrog · 2018-12-02T21:42:42Z

So this now also adds a [docs] extras require and uses that for RTD. I've also removed pip requirement files because if we do go down this road we should avoid duplicating information.

codecov · 2018-12-02T23:07:31Z

Codecov Report

Merging #8198 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8198      +/-   ##
==========================================
- Coverage   86.91%   86.91%   -0.01%     
==========================================
  Files         383      383              
  Lines       57889    57889              
  Branches     1056     1056              
==========================================
- Hits        50313    50312       -1     
- Misses       6962     6963       +1     
  Partials      614      614

Impacted Files	Coverage Δ
astropy/utils/data.py	`80.7% <0%> (-0.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a170a74...007f0b8. Read the comment docs.

eteq

I like this change, but a few suggestions (which I'm not necessarily attached to but they might be good ideas):

(related to an inline comment) keep the requirements files and just pull them in in the setup script so people can pip install -r <whatever> but still get all of the goodness here.
Add something about this to the install instructions?
Make sure this works on RTD. I think @astrofrog did this already (in-person discussion), but just making sure.

Also, later on (doesn't have to be in this PR), we should add something that somehow checks if all the optional dependencies are actually in this list. @astrofrog says there's something already in glue that makes that fairly straightofrward to manage (i.e., a thing we can replace our HAS_SCIPY and similar pytest marks with). But we can add an issue after this is merged about this.

eteq · 2018-12-06T19:06:08Z

setup.py

+extras_require = {}
+
+# All optional dependencies for the tests
+extras_require['test'] = ['pytest-astropy', 'pytest-xdist', 'pytest-mpl',


We could keep the existing requirements files and instead do something like:

Suggested change

extras_require['test'] = ['pytest-astropy', 'pytest-xdist', 'pytest-mpl',

with open('pip-requrirements-test') as f:

extras_require['test'] = [line for line in f.read().split('\n') if line.strip()!='']

?

eteq · 2018-12-06T19:24:45Z

Some in-person discussion re 1 led to: we should not use requirements files because they aren't for dependencies, rather for environments according to the PyPA (thanks @Cadair for pointing that out). But we can still see have a cleaner representation of the requirements by putting them in setup.cfg, so we should do that.

@pllim pointed out some users are using requirements files though, so we should do something like leaving it there with a comment that says "switch to the the new pip install astropy[all] syntax". I would argue it shouldn't be a comment, rather it should error so that a user has to go and learn the new trick. But opinions in the room varied on that...

pllim · 2018-12-06T20:12:42Z

some users are using requirements

Maybe send out a notice to astropy users mailing list to see if there are any objections?

…on RTD

astrofrog · 2018-12-07T17:06:41Z

I've added mentions of this to the installation docs, and tested that it works on RTD. I've also added a dummy pip-requirements file with instructions for using the new way.

CHANGES.rst

[ci skip]

astrofrog · 2018-12-07T22:47:56Z

Since the CI passed before my minor changelog entry change, and this has been approved, I'll go ahead and merge.

bsipocz · 2018-12-31T08:41:59Z

docs/nitpick-exceptions

 py:obj NotImplementedError
 py:obj AttributeError
 py:obj NotImplementedError
+py:obj RendererBase


I need this change in the release. Should I try to backport the whole PR, or rather just cherry pick this single commit?

pllim · 2019-01-19T01:20:04Z

So, what is the minversion of setuptools that supports this?

astrofrog · 2019-01-19T22:40:22Z

I think it's setuptools 30.3.

This reverts commit 1d9729b. The way `astropy` declared its dependencies was changed in astropy#8198. The migration instructions added by the reverted commit are more than 4 years old by now and it is safe to assume they are not needed anymore.

The way `astropy` declared its dependencies was changed in astropy#8198. The migration instructions added by the deleted file are more than 4 years old by now and it is safe to assume they are not needed anymore.

astrofrog added testing Experimental labels Nov 30, 2018

astrofrog force-pushed the extras-require branch from 5436134 to a6780b5 Compare November 30, 2018 13:02

astrofrog mentioned this pull request Nov 30, 2018

install_requires is ignored astropy/ci-helpers#337

Closed

astrofrog force-pushed the extras-require branch from 405c169 to 6d6af00 Compare November 30, 2018 13:22

astrofrog force-pushed the extras-require branch from 6d6af00 to 5eb1105 Compare December 2, 2018 21:42

astrofrog force-pushed the extras-require branch from 5eb1105 to de1ca0e Compare December 2, 2018 21:43

astrofrog force-pushed the extras-require branch 3 times, most recently from 48e1f94 to 45f918e Compare December 3, 2018 04:28

astrofrog mentioned this pull request Dec 3, 2018

Added a new mode of using ci-helpers based on pip install -e .[...] astropy/ci-helpers#357

Closed

astrofrog force-pushed the extras-require branch 3 times, most recently from 8840061 to 8c88cb9 Compare December 5, 2018 15:32

astrofrog removed the Experimental label Dec 6, 2018

eteq approved these changes Dec 6, 2018

View reviewed changes

astrofrog force-pushed the extras-require branch from a493618 to 00f2f6a Compare December 7, 2018 06:54

astrofrog added this to the v3.2 milestone Dec 7, 2018

astrofrog added 10 commits December 7, 2018 10:04

Added [docs] to extras_require and remove pip requirement files

65979a9

Remove mentions of pip requirement files, use the extras requirement …

66d9f71

…on RTD

Include pytest in docs dependencies

f9e608a

Fix sphinx warnings

3e8dab7

Tidy up extras_require

1b6bbc8

Fix syntax in RTD config

70d29cf

Added changelog entry

0b35ec3

Define extras_require in setup.cfg

24746a8

Added empty pip requirements file with migration instructions

1d9729b

Update installation instructions

2d215f5

astrofrog force-pushed the extras-require branch from 3c2e6d6 to 2d215f5 Compare December 7, 2018 17:05

Clean up changelog

aed5682

astrofrog added the Ready-for-final-review label Dec 7, 2018

astrofrog requested a review from bsipocz December 7, 2018 20:16

bsipocz approved these changes Dec 7, 2018

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

Minor change to changelog

007f0b8

[ci skip]

astrofrog merged commit b225a5a into astropy:master Dec 7, 2018

astrofrog mentioned this pull request Dec 7, 2018

[WIP] Using Tox #8248

Closed

4 tasks

bsipocz reviewed Dec 31, 2018

View reviewed changes

astrofrog mentioned this pull request Jan 13, 2019

Add pip requirements and consolidate with doc and setup ejeschke/ginga#709

Closed

6 tasks

pllim mentioned this pull request Jan 24, 2019

Documentation of trying development has python2-only stuff #7287

Closed

pllim mentioned this pull request Jun 11, 2019

Make RTD requirements configurable #8820

Closed

eerovaher mentioned this pull request Feb 9, 2023

Remove pip-requirements #14375

Merged

	extras_require['test'] = ['pytest-astropy', 'pytest-xdist', 'pytest-mpl',
	with open('pip-requrirements-test') as f:
	extras_require['test'] = [line for line in f.read().split('\n') if line.strip()!='']

Uh oh!

Conversation

astrofrog commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drdavella commented Nov 30, 2018

Uh oh!

astrofrog commented Nov 30, 2018

Uh oh!

drdavella commented Nov 30, 2018

Uh oh!

saimn commented Nov 30, 2018

Uh oh!

bsipocz commented Nov 30, 2018

Uh oh!

saimn commented Nov 30, 2018

Uh oh!

bsipocz commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Dec 2, 2018

Uh oh!

codecov bot commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eteq left a comment • 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!

eteq Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

eteq commented Dec 6, 2018

Uh oh!

pllim commented Dec 6, 2018

Uh oh!

astrofrog commented Dec 7, 2018

Uh oh!

Uh oh!

astrofrog commented Dec 7, 2018

Uh oh!

bsipocz Dec 31, 2018

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 19, 2019

Uh oh!

astrofrog commented Jan 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astrofrog commented Nov 30, 2018 •

edited

Loading

bsipocz commented Nov 30, 2018 •

edited

Loading

astrofrog commented Nov 30, 2018 •

edited

Loading

codecov bot commented Dec 2, 2018 •

edited

Loading

eteq left a comment •

edited

Loading