DOC Contributing guide update #13961
Conversation
| @@ -678,54 +672,6 @@ To test code coverage, you need to install the `coverage | |||
|
|
|||
| 3. Loop. | |||
|
|
|||
| Developers web site | |||
| ------------------- | |||
There was a problem hiding this comment.
The wiki seems outdated, and with lots of TODO, so I don't think it's a particularly useful resource. This is a typical rabit-hole risk for new users.
There was a problem hiding this comment.
Let's put a note on the wiki to that effect.
| Contributing pull requests | ||
| -------------------------- | ||
| Pull request checklist | ||
| ---------------------- |
There was a problem hiding this comment.
This list is essentially the same but I tried to prioritize tests (with some tips that were IMO missing), documentation and pep8.
|
|
||
| $ git checkout -b my-feature | ||
|
|
||
| and start making changes. Always use a ``feature`` branch. It's good practice to | ||
| never work on the ``master`` branch! | ||
|
|
||
| .. note:: |
There was a problem hiding this comment.
I thought this note was redundant with the instructions below about solving conflicts, but feel free to disagree (esp @adrinjalali since I think you wrote it IIRC).
|
Looks good to me. |
doc/documentation.rst
Outdated
| </div> | ||
| <div class="row-fluid"> | ||
| <div class="span4 box"> | ||
| <h2><a href="governance.html">Governance</a></h2> |
There was a problem hiding this comment.
Hmm... I had thought it sufficient to link to this from the About Us page, but now I can't see where we link to About Us, except on index.html
There was a problem hiding this comment.
I don't think anywhere except for "about us". The whole navigation is a bit of a mess tbh, I don't have strong feelings either way
There was a problem hiding this comment.
I think we somehow came to the conclusion to not put the link here. But I personally prefer to have it here and more visible.
There was a problem hiding this comment.
We should probably link to About Us here instead of governance. And we should possibly move the Governance section of about.rst up to before the author list???
…ntributing_guide_rewrite
|
|
||
| pip install --editable . | ||
|
|
||
| every time the source code of a compiled extension is | ||
| changed (for instance when switching branches or pulling changes from upstream). | ||
| every time the source code of a compiled extension is changed (for |
There was a problem hiding this comment.
I'd replace that, or maybe just mention, by python setup.py build_ext -i which really is what is needed.
There was a problem hiding this comment.
I'd rather not add another command, even if it's more strictly correct. I think introducing a single command is less scary for beginners.
|
@scikit-learn/core-devs people, any thought against switching from [MRG]/[WIP] to using Github Draft PRs? |
|
I did not find a way to change from PR to Draft, and we would rely completely on users to mark it as Draft. |
|
Me too, I'm not sure that using Draft instead of WIP matters in practice. |
|
I am +1 on keeping [MRG]/[WIP] since there is not a way to change from PR to Draft. |
|
Ok, leaving it as is then. Thanks! |
| The latest contributing guide is available in the repository at | ||
| `doc/developers/contributing.rst`, or online at: | ||
|
|
||
| https://scikit-learn.org/dev/developers/contributing.html |
There was a problem hiding this comment.
I've been thinking about (and maybe we had some discussion about it?) having the link from the stable website to the dev for the contributing guide. But I'm more than happy with this note here.
There was a problem hiding this comment.
I think this note is good enough.
| In the vast majority of cases, building scikit-learn for development purposes | ||
| can be done with:: | ||
|
|
||
| pip install numpy scipy cython pytest flake8 |
There was a problem hiding this comment.
I find it also kinda necessary to have sphinx and sphinx-gallery for development.
There was a problem hiding this comment.
Good point for joblib.
For the doc dependencies, I had them before, but reverted (https://github.com/scikit-learn/scikit-learn/pull/13961/files#r288462944)
I have no strong opinion
There was a problem hiding this comment.
Joblib will be installed automatically when installing scikit-learn.
There was a problem hiding this comment.
with the same argument we should also remove numpy and scipy. We have numpy, scipy, and joblib as requirements in setup.py.
There was a problem hiding this comment.
I removed them. I'm not sure about removing numpy though.
doc/developers/contributing.rst
Outdated
| 4. Install library in editable mode:: | ||
| 4. Install the development dependencies:: | ||
|
|
||
| $ pip install numpy scipy cython pytest flake8 |
There was a problem hiding this comment.
joblib? Also, I find it suboptimal and hard to maintain that we basically have this section twice in our documentations.
There was a problem hiding this comment.
I think a little bit of redundancy in our docs is worth it if it avoids users going through the rabbit holes of clicking into our multiple "see link for details".
| specific to the file | ||
| - `pytest sklearn/linear_model` to test the whole `linear_model` module | ||
| - `pytest sklearn/doc/linear_model.rst` to make sure the user guide | ||
| examples are correct. |
There was a problem hiding this comment.
adding the test_common and test_docstrings may also be useful?
There was a problem hiding this comment.
Adding test_common, but test_docstrings very rarely fails so probably not worth adding it
|
Before this is merged we need to know what are the actual dependencies needed to run
Just |
|
AFAIK, it works because pip fetches dependencies, and we have included numpy, scipy, and joblib in setup.py. Cython needs to be installed manually cause it's a build dependency and not a runtime/install dependency, and I couldn't find a way to tell pip to install something only if its making the package and not just installing a prepackaged one. |
|
(just a remark: cython only is not enough if you build with the |
|
Could we have this in before Friday? (it'd be really nice to have it for the sprint on Saturday) |
Still waiting for a clear answer to #13961 (comment). maybe @ogrisel @amueller ? |
that's true, but at the point somebody does |
There was a problem hiding this comment.
I can see that the section on "Working Notes" under "Rolling your own estimator" could be removed or changed to say that we use GitHub's pull requests and issues to track work on a project more than its Wiki.
Otherwise LGTM. I'm wondering still whether there are good ways to split this into two pages, one focused on coding (?) with scikit-learn practice and the other on getting started with contributing.
doc/documentation.rst
Outdated
| </div> | ||
| <div class="row-fluid"> | ||
| <div class="span4 box"> | ||
| <h2><a href="governance.html">Governance</a></h2> |
There was a problem hiding this comment.
We should probably link to About Us here instead of governance. And we should possibly move the Governance section of about.rst up to before the author list???
doc/developers/contributing.rst
Outdated
| the user guide, with small code snipets. If relevant, please also add | ||
| references in the literature, with PDF links when possible. | ||
|
|
||
| 11. The documentation should also include expected time and space complexity |
There was a problem hiding this comment.
which documentation? say user guide...
| @@ -678,54 +672,6 @@ To test code coverage, you need to install the `coverage | |||
|
|
|||
| 3. Loop. | |||
|
|
|||
| Developers web site | |||
| ------------------- | |||
There was a problem hiding this comment.
Let's put a note on the wiki to that effect.
…ntributing_guide_rewrite
| @@ -69,11 +76,6 @@ The following people have been active contributors in the past, but are no longe | |||
| - Virgile Fritsch | |||
| - Wei Li | |||
There was a problem hiding this comment.
In my opinion, we should really kill this author list, because it is never up to date, and point to the list generated by github.
However, this could go in another PR.
|
|
||
| To build the documentation, you need to be in the ``doc`` folder:: | ||
|
|
||
| cd doc | ||
|
|
||
| It also requires having the version of scikit-learn installed that corresponds | ||
| to the documentation, e.g.:: |
There was a problem hiding this comment.
Why remove this? The command should be executed before cd ing into the doc folder though.
There was a problem hiding this comment.
we start the section with "First, make sure you have :ref:properly installed <install_bleeding_edge>
the development version."
doc/developers/contributing.rst
Outdated
| In the past, the policy to resolve conflicts was to rebase your branch on | ||
| ``master``. GitHub interface deals with merging ``master`` better than in | ||
| the past. | ||
| The `Git documentation <https://git-scm.com/documentation>`_ is an excellent |
There was a problem hiding this comment.
For readability, I would put this in a "topic" directive.
Also, I would add a link to http://try.github.io/, which I find really good to learn git + github workflow.
|
Before this is merged, what are the actual dependencies needed to run
|
|
@NicolasHug I think it's easy to confirm (in a reproducible manner) that |
It's just that it's not in |
|
But this works: [adrin@beauty scikit-learn]$ virtualenv -p python3 /tmp/.venv
Running virtualenv with interpreter /usr/bin/python3
Using base prefix '/usr'
New python executable in /tmp/.venv/bin/python3
Also creating executable in /tmp/.venv/bin/python
Installing setuptools, pip, wheel...
done.
[adrin@beauty scikit-learn]$ source /tmp/.venv/bin/activate
(.venv) [adrin@beauty scikit-learn]$ pip install Cython
Collecting Cython
Using cached https://files.pythonhosted.org/packages/b8/c0/500afce93187b9d94d5c6e80a3032f90bf559bd4c8d12b45a10bc9a67b92/Cython-0.29.10-cp37-cp37m-manylinux1_x86_64.whl
Installing collected packages: Cython
Successfully installed Cython-0.29.10
(.venv) [adrin@beauty scikit-learn]$ pip install -e .
Obtaining file:///home/adrin/Projects/github.com/sklearn/scikit-learn
Collecting numpy>=1.11.0 (from scikit-learn==0.22.dev0)
Using cached https://files.pythonhosted.org/packages/fc/d1/45be1144b03b6b1e24f9a924f23f66b4ad030d834ad31fb9e5581bd328af/numpy-1.16.4-cp37-cp37m-manylinux1_x86_64.whl
Collecting scipy>=0.17.0 (from scikit-learn==0.22.dev0)
Using cached https://files.pythonhosted.org/packages/5d/bd/c0feba81fb60e231cf40fc8a322ed5873c90ef7711795508692b1481a4ae/scipy-1.3.0-cp37-cp37m-manylinux1_x86_64.whl
Collecting joblib>=0.11 (from scikit-learn==0.22.dev0)
Using cached https://files.pythonhosted.org/packages/cd/c1/50a758e8247561e58cb87305b1e90b171b8c767b15b12a1734001f41d356/joblib-0.13.2-py2.py3-none-any.whl
Installing collected packages: numpy, scipy, joblib, scikit-learn
Running setup.py develop for scikit-learn
Successfully installed joblib-0.13.2 numpy-1.16.4 scikit-learn scipy-1.3.0 |
|
Yes because we state numpy, scipy and joblib in the
So for the command |
|
Ok, so I guess this can be merged now :) |
There is a lot of +1. Feel free to merged once this is green ;) |
|
Thanks everyone for the reviews! @adrinjalali have fun at the sprint! You'll get to crash test it ^^ |
This PR updates the contributing guide.
I tried to prioritize the instructions that, IMHO, are relevant for a potentially inexperienced new contributor (based on my experience from the sprints)
Some of the changes are personal taste, so feel free to disagree.
I also created a separate entry for the governance document, since I don't think it should be in the contributing guide.