Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[MRG] Issue 7867: Install numpy and scipy using "install_requires" #10402
Conversation
Jan 4, 2018
This was referenced
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lesteve
Jan 4, 2018
Member
To remove the errors from the log, I think you need to remove the numpy_status stuff like you removed the scipy one. Also maybe look at the whole file to spot opportunities for clean-up. Amongst other things there probably are some comments that don't make sense any more once this PR is merged.
To remove the errors from the log, I think you need to remove the numpy_status stuff like you removed the scipy one. Also maybe look at the whole file to spot opportunities for clean-up. Amongst other things there probably are some comments that don't make sense any more once this PR is merged. |
Small github tip: I edited your PR description to use "Fix #issueNumber", this way the associated issue gets closed automatically when the PR is merged. For more details, look at this. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
barrywhart
Jan 4, 2018
Contributor
@lesteve: About the errors, njsmith had suggested:
If import numpy doesn't work, then don't use setup_requires, just print an error and exit.
My question is where and how to do that in order to get the desired effect? I'm thinking as written currently, it is checking in some cases when it shouldn't. This seems like a tricky area requiring expertise in Python setuptools
. I notice that both scikit-learn
and scipy
have some tricky command-line parsing in setup.py
to try and determine whether certain steps are needed.
Specific suggestions would be appreciated.
@lesteve: About the errors, njsmith had suggested:
My question is where and how to do that in order to get the desired effect? I'm thinking as written currently, it is checking in some cases when it shouldn't. This seems like a tricky area requiring expertise in Python Specific suggestions would be appreciated. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
barrywhart
Jan 4, 2018
Contributor
I looked and don't see any obvious opportunities for cleanup. We can take another look at that once bigger issues are addressed (e.g. the spurious error building a wheel during source installation).
I looked and don't see any obvious opportunities for cleanup. We can take another look at that once bigger issues are addressed (e.g. the spurious error building a wheel during source installation). |
lesteve
referenced this pull request
in nistats/nistats
Jan 4, 2018
Open
scipy not listed as dependency in setup.py #154
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lesteve
Jan 12, 2018
Member
I looked at this a bit more and I think this is fine as it is despite the spurious error:
- this only happens when installing from the .tar.gz (and only when you have the
wheels
package installed). In practice, wheels are available on all platforms for recent releases. - the scikit-learn package ends up being installed correctly anyway
I feel the benefit of having scikit-learn installable via pip install scikit-learn
in most cases outweighs by far the downside of having a spurious error in the middle of the installation that happens only in a few cases.
I looked at this a bit more and I think this is fine as it is despite the spurious error:
I feel the benefit of having scikit-learn installable via |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Why should we not keep the scipy check removed here? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
barrywhart
Jan 14, 2018
Contributor
It shouldn't be needed anymore; install_requires
will take care of installing it, so we'd just be trusting the package manager (i.e. pip
) to work correctly.
It shouldn't be needed anymore; |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jnothman
Jan 14, 2018
Owner
Well, I've not done much packaging, but don't think this PR does harm, and will believe the experts. Let's merge.
Well, I've not done much packaging, but don't think this PR does harm, and will believe the experts. Let's merge. |
barrywhart commentedJan 4, 2018
•
Edited 1 time
-
lesteve
Jan 4, 2018
Alternate, simpler PR after feedback on #10398. We can iterate on this one but possibly fall back to the other one if this doesn't work out.
In my testing, installing from a
.tar.gz
appears to work (i.e. I can import thesklearn
module afterwards) but there are some errors displayed during installation. I don't know if those are a concern or not. Full output below.Installing from a wheel worked smoothly.
Installing from source tarball (Mac OS X Sierra, Python 2.7.13):
Fixes #10398, fix #7867.