8000 [MRG + 2] Ensure dependencies installed by saketkc · Pull Request #4371 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG + 2] Ensure dependencies installed #4371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2015
Merged

[MRG + 2] Ensure dependencies installed #4371

merged 1 commit into from
Apr 28, 2015

Conversation

saketkc
Copy link
Contributor
@saketkc saketkc commented Mar 9, 2015

@amueller
Copy link
Member

I think we should point to the installation instructions on the sklearn website, or maybe directly recommend binary packages (anaconda / canopy)

@saketkc
Copy link
Contributor Author
saketkc commented Mar 10, 2015

I updated it. But I am somehow still not happy with it. I think we should keep it more explicit?

@saketkc
Copy link
Contributor Author
saketkc commented Mar 11, 2015

@amueller Comments?

amueller
amueller reviewed Mar 11, 2015
View reviewed changes
setup.py
if is_scipy_installed() is False:
sys.stderr.write("Error: Scientific Python(SciPy) is not installed.\n"
"scikit-learn requires SciPy.\n"
"Instruction installations are available on sklearn website:\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say " on the scikit-learn website"

@amueller
Copy link
Member

Apart from my comment it seems fine with me.

@saketkc saketkc force-pushed the fix_setup branch 3 times, most recently from 081bd4d to 6d14d52 Compare March 11, 2015 23:03
@saketkc
Copy link
Contributor Author
saketkc commented Mar 16, 2015

Good to merge @amueller?

@amueller amueller changed the title FIX: Ensure dependencies installed [MRG + 1] Ensure dependencies installed Mar 16, 2015
@amueller
Copy link
Member

Yeah, it looks good to me.

@saketkc
Copy link
Contributor Author
saketkc commented Mar 25, 2015

Bumping this up.

@amueller
Copy link
Member

closes #1495.

jnothman
jnothman reviewed Mar 30, 2015
View reviewed changes
setup.py
@@ -162,6 +175,18 @@ def setup_package():

metadata['version'] = VERSION
else:
if is_numpy_installed() is False:
sys.stderr.write("Error: Numerical Python(NumPy) is not installed.\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much rather a space between Python and (NumPy)

@amueller amueller changed the title [MRG + 1] Ensure dependencies installed [MRG] Ensure dependencies installed Apr 1, 2015
@amueller
Copy link
Member
amueller commented Apr 1, 2015

Gave my +1 to fast, sys.exit is not good

@saketkc
Copy link
Contributor Author
saketkc commented Apr 2, 2015

Done.

@amueller
Copy link
Member
amueller commented Apr 2, 2015

OK, LGTM now :)

@amueller amueller changed the title [MRG] Ensure dependencies installed [MRG + 1] Ensure dependencies installed Apr 2, 2015
@kastnerkyle
Copy link
Member

Looks good!

@kastnerkyle kastnerkyle changed the title [MRG + 1] Ensure dependencies installed [MRG + 2] Ensure dependencies installed Apr 16, 2015
@saketkc
Copy link
Contributor Author
saketkc commented Apr 20, 2015

bump.

@gravyboat gravyboat mentioned this pull request Apr 22, 2015
Closed
@ogrisel
Copy link
Member
ogrisel commented Apr 28, 2015

Hi, in its recent versions (along with recent enough versions of pip), the setup.py has an setup_requires for numpy that works meaning that pip install scipy works by first building and installing numpy from source if not available and then re-running the scipy setup.py.

https://github.com/scipy/scipy/blob/master/setup.py#L201

@ogrisel
Copy link
Member
ogrisel commented Apr 28, 2015

Ok I changed my mind: for windows users it's true that it won't help them trying to build make pip try numpy and scipy source and they will get obscure build error messages.

For MacOSX users there are wheels on PyPI for numpy / scipy & scikit-learn so the setup.py is never executed (unless the user explicitly 8000 ask to build from the source one way or another). Mac OSX users are therefore neither impacted by the merge of this PR nor by the alternative which would be to configure setup_requires like scipy does wrt. numpy.

For Linux users using the pip version currently shipped in most "stable" distros, namely pip 1.5.4 for Ubuntu 14.04 LTS, the setup_requires solution does not work properly for scipy. Based on some tests, only pip 6 and later can properly get numpy installed before trying to install scipy from source.

Let's merge this PR for now and we might want to explore again the possibility to use setup_requires once we have numpy and scipy publish wheels for windows on PyPI and major linux distros ship pip 6+ by default.

ogrisel added a commit that referenced this pull request Apr 28, 2015
@ogrisel
Merge pull request #4371 from saketkc/fix_setup
95f9be5 
[MRG + 2] Ensure dependencies installed
@ogrisel ogrisel merged commit 95f9be5 into scikit-learn:master Apr 28, 2015
@GaelVaroquaux
Copy link
Member

Thanks @ogrisel : I agree with this decision.

In addition, I fear very much the results of a "pip install -U" on an already-working system with the setup_requires.

@ogrisel
Copy link
Member
ogrisel commented Apr 28, 2015

In addition, I fear very much the results of a "pip install -U" on an already-working system with the setup_requires.

Without a sudo it's not going to break anything (if the user has a --user flag or is working in a venv). With a sudo I agree it could be a problem but then the hack used by scipy's setup.py to only populate setup_requires and install_requires if numpy is missing would give a reasonable protection.

@saketkc saketkc deleted the fix_setup branch April 28, 2015 14:09
@amueller
Copy link
Member

Why wouldn't it break anything with a --user flag? The imported version afterwards would be the slow one, right?

@HolgerPeters HolgerPeters mentioned this pull request Nov 14, 2016
Closed
@lesteve lesteve mentioned this pull request Jan 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@saketkc @amueller @kastnerkyle @ogrisel @GaelVaroquaux @jnothman
0