Split up requirements, update CONTRIBUTING.md #2208
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twiecki
reviewed
May 22, 2017
requirements.txt
Outdated
| six | ||
| h5py | ||
| six>=1.10.0 | ||
| h5py>=2.7.0 |
There was a problem hiding this comment.
I think h5py and numdifftools are already optional, so we should move them to extra.
|
Your point on |
|
I think this is a blocker for 3.1. |
|
erp, sorry, forgot about it -- can tweak in a few hours. reflecting on it, I'm actually more in favor of removing numpy/scipy as requirements, since theano already does that (so we're just increasing the chance of a version mismatch). That can wait for a while though! |
|
We can remove them, it's a good idea. We'd push another release candidate to make sure it doesn't cause any problems. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #2199 . Might take a few tries on travis.
A few changes:
Using a neat trick to install by version in requirements.txt: http://stackoverflow.com/questions/19559247/requirements-txt-depending-on-python-version/33451105#33451105, though this will apparently break on
pip < 6.0.setup.pystill reads requirements fromrequirements.txt(and plays nicely with the above trick), and useful packages for development can be installed withrequirements-dev.txt.Also slimmed down
create_testenv.sh(for example,pytest-covinstallscoverage), though it'd be nice to figure out a way for that to read from requirements{-dev}.txt, too. I like using conda to create that environment, but don't know enough about how to get conda to play nicely withsetup.py...Also, there are at least two libraries that are still included but could be considered optional or removed (really, if we wanted to be super careful, probably just theano suffices, since it requires numpy and scipy):
numdifftoolsis used for one test, and one function that is used nowhere else (tuning.scaling.approx_hessian), and was last touched 4 years ago when @twiecki madenumdifftoolsoptional.h5pyis only used for the backendFinally! We might want to benchmark speedups from
mkl-serviceand advertise them -- thecreate_testenv.shscript installs it, but if we find real performance benefits from it, it might make sense to add to the requirements, or mention somewhere that this exists.