-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC: Update documentation of Installing the development version of scikit-learn #31173
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: Update documentation of Installing the development version of scikit-learn #31173
Conversation
…ibution in Building from Source
…s instructions using Build Tools
…n_dev_version_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.
Thanks for the PR @vitorpohlenz. Here are some comments and nitpicks
.. note:: | ||
|
||
At the time of writing, the newest Python 3.13 version from the conda distribution uses freethreading, | ||
which may cause problems when compiling on some OS like Windows. | ||
In these cases, it is recommended to use Python 3.12 in the `conda environment`. With the following command: | ||
|
||
.. prompt:: bash $ | ||
|
||
conda create -n sklearn-env -c conda-forge python=3.12 numpy scipy cython meson-python ninja | ||
|
||
If the situation above is not the case, you can use the latest versions with the following 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.
I would not include that in the doc. This is a temporary issue with conda-forge and the workaround is pretty straightforward. (by the way you can still install python 3.13 by requesting the python-gil package additionaly). This is the kind of stuff that stick in the doc for too long and we forget to clean it up and it clutters the doc and creates confusion.
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 really understand what you are saying, sometimes "temporary" recommendations stick as permanents. But I just wanted to give some warning about this version that uses free-threeding (not sure if in the following versions this error will persist).
But it may be the case to wait for that to see, right?
I can remove it, no problem, but if you see any way to give this "temporary warning," just let me know and I will adjust in this doc.
#. Build the project with pip, | ||
whether you are in `conda environment` or in dedicated `virtualenv`: |
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 would not change that. This is the next step after creating and activating the env. I don't see how it could be ambiguous here. I'm probably biased but reading this makes me wonder "why is there a need for this precision ? Is there something I missed ?".
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 would not change that. This is the next step after creating and activating the env. I don't see how it could be ambiguous here. I'm probably biased but reading this makes me wonder "why is there a need for this precision ? Is there something I missed ?".
Probably, I also have some bias, which was the reason for me to update that line. As my first time building from source using conda env
, I was forgetting this step in the conda env
, and just executing this step when in the virtualenv
.
But it was probably more related to my lack of attention in this part than a problem with the tutorial.
Ajusting note comment sugested by jeremiedbb when using Windows Build Tools Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
… file and the specific python 3.13 version in conda env
…n_dev_version_from_source
Thanks for the comments @jeremiedbb, since it was my first PR in sklearn, I found it nice to think about what you suggested, even the "nitpicks". Also, I would like to ask for advice for this and future PRs: Should I click on the button I do not want to be annoying like "I want you to review it again now," but this also could be used as a flag like "Everything alright, I have finished working in the comments, so you can review it again when you have time." |
I can't answer for the other maintainers. I don't mind the request review button. You can also directly ping me, that's what we usually use. In any case, we might not answer right away because the review bandwidth is limited on the project. |
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. Thanks
Thanks for the tip @jeremiedbb ! I will do this in the next PRs! |
@@ -186,7 +186,11 @@ commands in ``cmd`` or an Anaconda Prompt (if you use Anaconda): | |||
.. prompt:: bash C:\> | |||
|
|||
SET DISTUTILS_USE_SDK=1 | |||
"C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Auxiliary\Build\vcvarsall.bat" x64 | |||
"C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Auxiliary\Build\vcvarsall.bat" x64 |
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 wondering if this is still needed now that we use meson ... the DISTUTILS_USE_SDK
is not needed anymore with 99% probability, the vcvarsall.bat
is probably not needed.
Personally, on Windows I compile inside a miniforge prompt and I never do this ...
It would be great if you could confirm @vitorpohlenz!
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 also tried to install CPython from python.org and use a venv in a cmd console and same thing it works fine without setting DISTUTILS_USE_SDK
and vcvarsall.bat
. I opened #31202 to simplify the Windows instructions.
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.
@lesteve, it seems that you are right!
I have tried creating the environment again without activating or calling the VS Environment for the compiler, and the Meson Build seems to handle it quite well.
Here is the Full log of the build. You can see that in line 15 it activates the VS Env, when running the command:
pip install --editable . --verbose --no-build-isolation --config-settings editable-verbose=true
Without the :
SET DISTUTILS_USE_SDK=1
"C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Auxiliary\Build\vcvarsall.bat" x64
The same message appears whether using a conda-env
or virtualenv
.
Summarizing, it seems that if you installed the Microsoft Visual Studio Build Tools correctly, the Meson Build will handle these things, and these commands are not necessary.
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 @vitorpohlenz for confirming this!
…kit-learn#31173) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Reference Issues/PRs
Fixes #31149
What does this implement/fix? Explain your changes.
Changed a few lines in the conda environments (suggestion for not using the newest Python 3.13 version in Windows), and updated the Windows Build Tools path for compilers.
Any other comments?
It is my first PR in scikit-learn, so I tried to keep it really small