Skip to content

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

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

thomasjpfan
Copy link
Member

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

Copy link
Member

@ogrisel ogrisel left a 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
Copy link
Member

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:

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

@ogrisel @thomasjpfan

  • 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.

Copy link
Member

@ogrisel ogrisel left a 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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@marenwestermann
Copy link
Member

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.

Copy link
Member

@marenwestermann marenwestermann left a comment

Choose a reason for hiding this comment

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

LGTM

@ogrisel
Copy link
Member

ogrisel commented Nov 5, 2021

I think it's clear now that people need to create a virtual environment for contributing code which previously wasn't so clear

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 --user flag and using the system package manager to install the compilers and dev headers for Python.

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.

@ogrisel ogrisel merged commit 7734398 into scikit-learn:main Nov 5, 2021
@ogrisel
Copy link
Member

ogrisel commented Nov 5, 2021

Merged. @glemaitre feel free to open a follow-up PR if you want to change the way we present the pre-commit utility.

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
* DOC Link to build from source in contributing guide

* DOC Clean up
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
* DOC Link to build from source in contributing guide

* DOC Clean up
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
* DOC Link to build from source in contributing guide

* DOC Clean up
glemaitre pushed a commit that referenced this pull request Dec 25, 2021
* DOC Link to build from source in contributing guide

* DOC Clean up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants