-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
clean up installation doc organization #10698
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.
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
Conversation
Maybe three main issues I'm trying to resolve with this:
|
This also closes #9881
Yes, that is strange... |
|
||
If you run the development version, it is cumbersome to reinstall the | ||
package each time you update the sources. It is thus preferred that | ||
you add the scikit-learn directory to your ``PYTHONPATH`` and build the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about suggesting to modify PYTHONPATH
as the preferred way to install from source.
I think pip install --editable .
+ suggesting to use either the --user
flag or run it in a virtualenv/conda virtualenv (for contributors) might be more standard.
doc/other_distributions.rst
Outdated
It can be installed by typing the following | ||
command:: | ||
|
||
sudo port install py26-scikit-learn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sudo port install py36-scikit-learn
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
|
||
pip install -U numpy scipy scikit-learn | ||
- pytest | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests require pandas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not to self: sync with current travis setup. Also maybe point to the travis setup file here?
doc/install.rst
Outdated
@@ -93,5 +95,4 @@ The `WinPython <https://winpython.github.io/>`_ project distributes | |||
scikit-learn as an additional plugin. | |||
|
|||
|
|||
For installation instructions for particular operating systems or for compiling | |||
the bleeding edge version, see the :ref:`advanced-installation`. | |||
For installation instructions for more distributions see :ref:`install_by_distribution`. For compiling the development version from source, or building the package if no distribution is available for your architecture, see the :ref:`advanced-installation`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awkward because it's under a heading about third party distributions and says "see Third party distributions of scikit-learn". Rather, put this paragraph before Anaconda and WinPython, and alias the link to say "see other distributions".
doc/other_distributions.rst
Outdated
|
||
Some third-party distributions are now providing versions of | ||
scikit-learn integrated with their package-management systems. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should note that the most popular are listed on the install page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not sure if we want to go into environments. If you have a good resource we can link it, but if we say something half-baked I don't think it's gonna be useful.
Agreed that we don't want to go there. I am not aware of any reference resource that would make sense of virtualenv, pipenv and conda env, but this one does try to. I'm not completely convinced it should be linked.
A few minor comments are below, otherwise LGTM.
@@ -7,58 +7,95 @@ Advanced installation instructions | |||
|
|||
There are different ways to get scikit-learn installed: | |||
|
|||
* :ref:`Install an official release <install_official_release>`. This | |||
is the best approach for most users. It will provide a stable version and pre-build packages are available for most platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a line break
newest version. | ||
or if you have write privileges:: | ||
|
||
git clone git@github.com:scikit-learn/scikit-learn.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's a copy paste but I would remove these two lines. It's unrelated to having to write privileges but rather is to do with registering an SSH key on GitHub (one can clone any repository via ssh). And even for people with write privileges cloning a fork is safer IMO to avoid pushing local branches to the scikit-learn org by accident..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, this is confusing / not true.
|
||
If you run the development version, it is cumbersome to reinstall the | ||
package each time you update the sources. Therefore it's recommended that you install in editable, | ||
which allows you to edit the code in-place. This basically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove "basically"
python setup.py build_ext --inplace | ||
|
||
every time the source code of a compiled extension is | ||
changed (for instance when switching branches or pulling changes from upstream). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should leave the instructions on using build_ext -i after changes. You could also do pip install -e . again, right?
Yes, I just run pip install -e .
to update - it does only recompile the changed Cython files as far as I can tell. Not sure what's the official position about this upstream, but if we use pip instead of setuptools to install IMO it would also make sense to use pip to re-compile extensions.
doc/developers/contributing.rst
Outdated
|
||
In case you experience issues using this package, do not hesitate to submit a | ||
ticket to the | ||
`Bug Tracker <https://github.com/scikit-learn/scikit-learn/issues>`_. You are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Github Issue tracker" (just to make it explicit that issues are tracked on Github without having to inspect the URL)
<install_bleeding_edge>`. This is best for users who want the | ||
latest-and-greatest features and aren't afraid of running | ||
brand-new code. | ||
brand-new code. This document describes how to build from source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dot missing.
It would be helpful to include these improved installation instructions for the next release... |
|
||
Scikit-learn requires: | ||
|
||
- Python (>= 2.7 or >= 3.4), | ||
- NumPy (>= 1.8.2), | ||
- SciPy (>= 0.13.3). | ||
|
||
Building Scikit-learn also requires |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is the building section this header might be superfluous?
this could probably do with some more polish but I think it's an improvement. Merge and go from there? |
thanks @jnothman ! |
This is an attempt to clean up the installation docs which I kinda left in shambles at my last attempt to simplify things.
The
advanced_installation.rst
is currently a mess, and also quite wrong in a couple of places.I tried to separate out three use-cases:
install.rst
)developers/advanced_installation.rst
)other_distributions.rst
).I moved the "getting the bleeding edge code" from "contributing" to "advanced installation" because I'm convinced the mosts common reason for people to build from source is that they want the development version.
This is motivated by my students asking how to install the dev version and me not being able to figure out where to point them.