-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC Link to build from source in contributing guide #21369
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
DOC Link to build from source in contributing guide #21369
Conversation
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.
Thanks. Here are some suggestions for further improvement. Let me know what you think.
Other than that, LGTM.
|
||
.. prompt:: bash $ | ||
|
||
pip install cython pytest pytest-cov flake8 mypy black==21.6b0 |
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.
Those optional dev dependencies (besides cython) are not part of the build from source doc. We should either add insert an item on the latter and cross-link back to this doc in the last item.
.. _upstream: | ||
|
||
6. Add the ``upstream`` remote. This saves a reference to the main | ||
4. Add the ``upstream`` remote. This saves a reference to the main | ||
scikit-learn repository, which you can use to keep your repository | ||
synchronized with the latest changes: | ||
|
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 think it's better to keep upstream remote configuration as a specific items because first time contributors are often confused by the remote configuration so I believe it's important to have a linkable bullet item for this. Then at the end if this item, I would add something like:
"""
You can check that the upstream
and origin
remote aliases are probably configured with the git remote -v
command. It should display something like the following.
origin https://github.com/mygithubname/scikit-learn.git (fetch)
origin https://github.com/mygithubname/scikit-learn.git (push)
upstream https://github.com/scikit-learn/scikit-learn.git (fetch)
upstream https://github.com/scikit-learn/scikit-learn.git (push)
with those remote repository aliases in place, doing:
.. prompt:: bash $
git pull upstream main
will be equivalent to the explicit:
.. prompt:: bash $
git pull https://github.com/scikit-learn/scikit-learn.git main
"""
Note that I used the HTTPS URL not to introduce the complexity of setting up ssh. Maybe we should update the advanced installation doc to also use the https://
URL for consistency.
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 updated everything to use SSH since most of our git clone
are targeting ssh. I also adding the check for git remote -v
for checking the remotes.
As for git pull upstream main
, in step 7, we already describe a similar workflow:
git checkout main
git fetch upstream
git merge upstream/main
I like the fetch+merge
a little better, since it is the same steps for syncing a local branch with upstream/main
.
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.
+1 for fetch and merge because it does not depend on the git config for the rebase/merge behavior of the git pull
command.
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.
- 1
It is good to see the fork information in here.
The page is called "Contributing" and for the text "If you plan on submitting a pull-request, you should clone from your fork instead," it's helpful for the user to have the remotes prominently part of the workflow.
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.
LGTM!
|
||
If you receive errors in building scikit-learn, see the | ||
:ref:`install_bleeding_edge` section. | ||
pip install pytest pytest-cov flake8 mypy black==21.6b0 |
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 am not mistaken, you only need to install pre-commit
that at the first commit will install flake8 mypy and black==21.6b0
. It is indeed the step 9. below that is stated optional.
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 think that we should remove the **optional**
because it would be disregarded easily. I think that most of the time it is a good practice to install pre-commit
because it will run black which is the main source of linting issues.
So in step 9. below, I would:
- remove the optional part
- emphasize that they are not required but will ensure that the commit changes will not fail the linting
- mention that you can skip pre-commit using the flag
--no-verify
when commiting.
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 am not mistaken, you only need to install pre-commit that at the first commit will install flake8 mypy and black==21.6b0.
My understanding is that pre-commit
installs black
in it's own virtual environment. If we want contributors to have black
in their development environment, then they would still need install black
.
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 think we can merge this and follow up with a PR if we want required pre-commit
.
Thanks a lot for this improvement as well as the improvements in #21353. I think it's clear now that people need to create a virtual environment for contributing code which previously wasn't so clear when following the steps here: https://scikit-learn.org/dev/developers/contributing.html# . I've been wanting to update the documentation after the last sprint but I'm happy to see that this has already been done. It looks good to me. |
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.
LGTM
It's actually not strictly needed but it's much more uniform and simpler to manage than alternatives such as installing everything with pip with the It's also possible to install everything in the base environment of a conda install but it's not considered a good practice so better learn to use isolated envs right from the beginning. |
Merged. @glemaitre feel free to open a follow-up PR if you want to change the way we present the pre-commit utility. |
* DOC Link to build from source in contributing guide * DOC Clean up
* DOC Link to build from source in contributing guide * DOC Clean up
* DOC Link to build from source in contributing guide * DOC Clean up
* DOC Link to build from source in contributing guide * DOC Clean up
Reference Issues/PRs
Follow up to #21353 and the comment from @reshamas #21353 (comment)
What does this implement/fix? Explain your changes.
In this PR, I link directly to "Building from Source" from the contributing steps. This way when looking at scikit-learn.org/dev/developers/contributing.html a contributor can redirect to the "Building from source page". The upside is that there is only one "building from source" location. The downside is that contributors need to navigate back and forth.
Any other comments?
CC @ogrisel