DOC Improvements to developer documentation by Eschivo · Pull Request #23551 · scikit-learn/scikit-learn

Eschivo · 2022-06-06T21:24:13Z

Improvements to developer documentation:

In advanced_installation.rst:
- added windows case when creating a virtual env without conda
- added recommendation to check the folder before running the build command
In contributing.rst:
- added reference to GitHub guide to generate SSH key (that's necessary to run some commands) .

Added Windows case in virtualenv activation

Added a check recommendation in the build project command

Added reference to github ssh key association. That is necessary to run git@github:YourLogin commands.

Eschivo · 2022-06-07T07:44:37Z

Since I forgot to run the make command inside the doc directory, I'm trying to run it but I get the following message:

(sklearn-env) C:\Users\....\...\...\scikit-learn\doc>make
Please use `make <target>` where <target> is one of
  html      to make standalone HTML files
  dirhtml   to make HTML files named index.html in directories
  pickle    to make pickle files
  json      to make JSON files
  htmlhelp  to make HTML files and a HTML help project
  qthelp    to make HTML files and a qthelp project
  latex     to make LaTeX files, you can set PAPER=a4 or PAPER=letter
  changes   to make an overview over all changed/added/deprecated items
  linkcheck to check all external links for integrity
  doctest   to run all doctests embedded in the documentation if enabled
  html-noplot   to make HTML files using Windows

Should I run the make html command? Thank you.

lesteve · 2022-06-23T11:58:49Z

Should I run the make html command? Thank you.

make should work on Windows since #23561 has been merged. It does the same as make html-noplot i.e. build the documentation without running the examples.

Your changes mostly make sense but they may need some tweaking. To suggest more precise changes I may need to have a closer look at the documentation to remind myself how it is organised:

clearly the SSH setup info was missing, not sure where to add it for better visibility. Not that at one point I was thinking of changing the instructions to use git clone with https. This is easier to clone but then when you want to push you need to create a Gihtub personal access token (you can not use user + password anymore) so this is not great either for easy setup. Keeping the instructions as they are and linking to SSH keys setup is reasonable.
the venv action on Windows is different, we should mention this, not sure how to do this so that it is easy to spot.
not sure where we should say that most of the commands are supposed to be run from the git repo root folder

thomasjpfan

Thank you for the PR!

thomasjpfan · 2022-06-07T15:39:58Z

doc/developers/advanced_installation.rst

+   Be sure to be in the folder of the scikit-learn repository before run the above command,
+   otherwise you will face errors.


A few steps above this one, we have:

scikit-learn/doc/developers/advanced_installation.rst

Lines 50 to 51 in a4a53ea

git clone git://github.com/scikit-learn/scikit-learn.git # add --depth 1 if your connection is slow

cd scikit-learn

which says to go into the directory. Do you think users will run into this issue here?

It may be. Actually, after I first cloned the repository I went directly to the next step and didn't notice that there was also to do the cd after the clone (that it's also obvious after all). I think that a friendly reminder here it's well accepted :)

In that case, I prefer have another step after cloning that says "navigate to the scikit-learn directory". Would that help you?

Yes, I think that will be clearer and less prone to missing the passage

thomasjpfan · 2022-06-07T15:51:06Z

doc/developers/contributing.rst

+   In order to run the above clone command you should have an SSH key to connect to your GitHub account.
+   Please refer to `this guide <https
10000
://docs.github.com/en/authentication/connecting-to-github-with-ssh/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent>`_ if you hadn't set it yet.


I do not think cloning public repos require any authentication. But to push to a fork, it does require authentication. I think this is point big enough to be it's own item.

Not that at one point I was thinking of changing the instructions to use git clone with https.

@lesteve I think following: https://docs.github.com/en/get-started/getting-started-with-git/caching-your-github-credentials-in-git with https is easier to follow compare to setting up ssh.

With the command we currently have in our doc, you do need SSH keys setup for cloning a public repo. For example this fails for me since I don't have SSH keys setup:

git clone git@github.com:scikit-learn/scikit-learn.git

Which one of the two sections would you advise, the gh CLI or the Git Credential Manager one?

I noticed now that there's also a mismatch in the two documents here discussed. In contributing.rst the clone is performed using ssh, as pointed out by @lesteve:

scikit-learn/doc/developers/contributing.rst

Lines 247 to 248 in 559533e

git clone git@github.com:YourLogin/scikit-learn.git # add --depth 1 if your connection is slow

cd scikit-learn

while in advanced_installation.rst, is made using git protocol:

scikit-learn/doc/developers/advanced_installation.rst

Lines 50 to 51 in a4a53ea

git clone git://github.com/scikit-learn/scikit-learn.git # add --depth 1 if your connection is slow

cd scikit-learn

That also could run into issues if you are in a network restricted by a firewall. I think that it's better to rethink the documentation using the https connection, which is easier to manage

To move this PR forward, lets revert this line about ssh keys so we can move forward with the other improvements.

We can work on authentication in a follow up.

Since when git clone was first introduced in the advanced_installation.rst through #10698 , it was included as it is now via the git protocol. So I will add a commit soon changing it to ssh in order to have consistency throughout these docs.

So I will add a commit soon changing it to ssh in order to have consistency throughout these docs.

I prefer not block this PR with the discussion around git+ssh. The Windows specific information and the changing directories is already an improvement to the instructions.

To clarify, I am suggesting to work on git+ssh instructions in a separate PR after this one gets merged.

vitorpohlenz · 2025-04-17T12:35:29Z

Just a guess: I think those problems were fixed in other PRs, because at least in the two recent PRs about documentation for developers to contribute ( #31202 and #31173) I did not find this problem.

@Eschivo, @thomasjpfan , @lesteve could you give a look if this Issue is still open or these modifications are still needed?

lesteve · 2025-04-17T15:54:05Z

Thanks @vitorpohlenz let's close this one since unfortunately it hasn't been inactive for a while and it has conflicts.

Quickly looking at it using sklearn-env\Scripts\activate.bat on Windows is indeed relevant ... if you find a way to mention the Windows tweak without breaking the flow too much, feel free to open a new PR!

I would not recommend working on the git clone https:// vs git clone git@. Personally I slightly lean towards the https option but people may have other opinions on this and it's likely too much work compared to what it can bring ...

vitorpohlenz · 2025-04-18T14:26:18Z

Thanks @vitorpohlenz let's close this one since unfortunately it hasn't been inactive for a while and it has conflicts.

Quickly looking at it using sklearn-env\Scripts\activate.bat on Windows is indeed relevant ... if you find a way to mention the Windows tweak without breaking the flow too much, feel free to open a new PR!

I would not recommend working on the git clone https:// vs git clone git@. Personally I slightly lean towards the https option but people may have other opinions on this and it's likely too much work compared to what it can bring ...

@lesteve, about sklearn-env\Scripts\activate.bat ,maybe just referencing how to work with vevns in with the "Official Tutorial", would be enough?

In this tutorial they have examples for Windows, Unix, and MacOs

Eschivo added 3 commits June 6, 2022 22:57

Update advanced_installation.rst

eab2d7d

Added Windows case in virtualenv activation

Update advanced_installation.rst

9d812f1

Added a check recommendation in the build project command

Update contibuting.rst

559533e

Added reference to github ssh key association. That is necessary to run git@github:YourLogin commands.

Eschivo mentioned this pull request Jun 23, 2022

DOC fix link "Comparison of model selection for regression" #23736

Merged

thomasjpfan reviewed Jun 23, 2022

View reviewed change 8000 s

thomasjpfan changed the title ~~[MRG] Documentation improvements~~ DOC Improvements to developer documentation Feb 16, 2023

github-actions bot added the Documentation label Feb 16, 2023

lorentzenchr added the Stalled label Feb 19, 2023

lesteve closed this Apr 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

DOC Improvements to developer documentation#23551

DOC Improvements to developer documentation#23551
Eschivo wants to merge 3 commits intoscikit-learn:mainfrom
Eschivo:documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		Be sure to be in the folder of the scikit-learn repository before run the above command,
		otherwise you will face errors.

	git clone git://github.com/scikit-learn/scikit-learn.git # add --depth 1 if your connection is slow
	cd scikit-learn

		In order to run the above clone command you should have an SSH key to connect to your GitHub account.
		Please refer to `this guide <https 10000 ://docs.github.com/en/authentication/connecting-to-github-with-ssh/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent>`_ if you hadn't set it yet.

	git clone git@github.com:YourLogin/scikit-learn.git # add --depth 1 if your connection is slow
	cd scikit-learn

Uh oh!

Comments

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants