BLD Migrate away from distutils and only use setuptools #24563

thomasjpfan · 2022-10-02T22:04:26Z

This PR migrates away from distutils and numpy.distutils to only using setuptools. Summary of changes:

Since setuptools does not support nested setup.py, all the setup goes into the root setup.py.
As noted in Should we continue to support compiler=intelem? #24525, compiler=intelem is dropped. In the future, I think a possible way to support it is to use environment flags, such as CC, etc.
No need to monkeypatch CCompiler_spawn anymore.

thomasjpfan · 2022-10-02T22:05:38Z

MANIFEST.in

@@ -1,7 +1,7 @@
 include *.rst
 recursive-include doc *
 recursive-include examples *
-recursive-include sklearn *.c *.h *.pyx *.pxd *.pxi *.tp
+recursive-include sklearn *.c *.cpp *.h *.pyx *.pxd *.pxi *.tp


This is required for the sdist to contain *.cpp files that is not a source file. Specifically, svm/src/libsvm/svm.cpp.

setup.py

…_distutils_pr

amueller · 2022-10-14T17:39:29Z

Looks good but wheels are failing?

…_distutils_pr

azure-pipelines.yml

thomasjpfan · 2022-10-26T15:35:21Z

sklearn/_build_utils/openmp_helpers.py

@@ -8,9 +8,6 @@
 import sys
 import textwrap
 import warnings
-import subprocess
-
-from distutils.errors import CompileError, LinkError


setuptools introduced CompileError and LinkError, but that requires 61.1. Even the latest version of Ubuntu (22.04LTS) only ships with 59.6.

In our case, I think we can get away with catching all Exceptions and display the error message.

jjerphan

Thank you for doing this migration, @thomasjpfan! 💯

There are still a few references to distutils and numpy.distutils, namely:

-W options in this snippet in this CI script which I think should be removed or be turned -Werror:

scikit-learn/build_tools/azure/test_script.sh

Lines 60 to 66 in bc55cd7

    
           # Python 3.10 deprecates distutils, which is imported by numpy internally 
        
           TEST_CMD="$TEST_CMD -Wignore:The\ distutils:DeprecationWarning" 
        
           # Ignore distutils deprecation warning, used by joblib internally 
        
           TEST_CMD="$TEST_CMD -Wignore:distutils\ Version\ classes\ are\ deprecated:DeprecationWarning"

in this snippet of the documentation which I think can instead state to use sklearn.show_versions:

scikit-learn/doc/computing/computational_performance.rst

Lines 279 to 286 in bc55cd7

    
           You can display the BLAS / LAPACK implementation used by your NumPy / SciPy / 
        
           scikit-learn install with the following commands:: 
        
               from numpy.distutils.system_info import get_info 
        
               print(get_info('blas_opt')) 
        
               print(get_info('lapack_opt'))

Here are some questions and suggestions, mainly about the inclusion of subpackages which was specified in setup.py files when recursively buidling.

setup.py

jjerphan · 2022-11-03T08:20:15Z

azure-pipelines.yml

-# Check compilation with intel C++ compiler (ICC)
- template: build_tools/azure/posix.yml
-  parameters:
-    name: Linux_Nightly_ICC
-    vmImage: ubuntu-20.04
-    dependsOn: [git_commit, linting]
-    condition: |
-      and(
-        succeeded(),
-        not(contains(dependencies['git_commit']['outputs']['commit.message'], '[ci skip]')),
-        or(eq(variables['Build.Reason'], 'Schedule'),
-           contains(dependencies['git_commit']['outputs']['commit.message'], '[icc-build]')
-        )
-      )
-    matrix:
-      pylatest_conda_forge_mkl:
-        DISTRIB: 'conda'
-        LOCK_FILE: 'build_tools/azure/pylatest_conda_forge_mkl_no_coverage_linux-64_conda.lock'
-        COVERAGE: 'false'
-        BUILD_WITH_ICC: 'true'
-


There are a few occurrences of ICC in the code-base, namely:

scikit-learn/doc/developers/contributing.rst

Line 552 in f905c21

[icc-build] Build & test with the Intel C compiler (ICC)

scikit-learn/sklearn/_build_utils/openmp_helpers.py

Lines 21 to 25 in bc55cd7

if sys.platform == "win32" and ("icc" in compiler or "icl" in compiler):

return ["/Qopenmp"]

elif sys.platform == "win32":

return ["/openmp"]

elif sys.platform in ("darwin", "linux") and "icc" in compiler:

Should we let users be able to use icc? or should we remove those if we do not test the support of this compiler on the CI?

I think we should keep this, assuming setting the CC env variable works for setuptools. But it's true it would be better to test it at some point. But I would not make this a blocker for this PR.

I did not get the Intel compilers to work by setting the CC environment variable (and friends). From memory, I got Intel to compile by setting environment variables, but there was an issue when running pytest.

(The Azure logs does not exist anymore because it has been too long)

Ok let's keep that in mind for a follow-up PR.

jjerphan · 2022-11-03T08:24:31Z

setup.py

+    "neighbors": [
+        {"sources": ["_ball_tree.pyx"], "include_np": True},
+        {"sources": ["_kd_tree.pyx"], "include_np": True},
+        {"sources": ["_partition_nodes.pyx"], "language": "c++", "include_np": True},


We can remove the distutils directive for the support of C++ in the header of this file:

scikit-learn/sklearn/neighbors/_partition_nodes.pyx

Lines 1 to 2 in bc55cd7

# distutils : language = c++

jjerphan · 2022-11-03T08:38:13Z

setup.py

+    is_pypy = platform.python_implementation() == "PyPy"
+    np_include = numpy.get_include()
+    default_libraries = ["m"] if os.name == "posix" else []
+    default_extra_compile_args = ["-O3"]


Does using the third level of optimisation with "-O3" produces bigger binaries?

Maybe but our binaries are not too big compared to other projects in the ecosystem, so I think it's fine for now.

But it would be interesting to understand why the linux binaries are much bigger than the other OSes. Maybe -03 is very different for GCC used by default for Linux than it is for clang used for macos?

We should investigate in a later issue / PR but let's keep this PR focused on the handling of the distutils deprecation.

sklearn/datasets/setup.py

jjerphan · 2022-11-03T08:39:29Z

sklearn/datasets/setup.py

-            sources=["_svmlight_format_fast.pyx"],
-            include_dirs=[numpy.get_include()],
-        )
-    config.add_subpackage("tests")


Are tests folders now added as sub-packages recursively and automatically?

I think this is thanks to:

https://github.com/scikit-learn/scikit-learn/pull/24563/files#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52

sklearn/inspection/setup.py

sklearn/ensemble/setup.py

sklearn/linear_model/setup.py

jjerphan · 2022-11-03T08:49:36Z

setup.py

@@ -294,6 +243,328 @@ def check_package_status(package, min_version):
            )


+extension_config = {


With this configuration, I guess the following comment can be updated to not mention the use of numpy.distutils and of a recursively build:

scikit-learn/setup.py

Lines 25 to 31 in bc55cd7

# This is a bit (!) hackish: we are setting a global variable so that the

# main sklearn __init__ can detect if it is being loaded by the setup

# routine, to avoid attempting to load components that aren't built yet:

# the numpy distutils extensions that are used by scikit-learn to

# recursively build the compiled extensions in sub-packages is based on the

# Python import machinery.

builtins.__SKLEARN_SETUP__ = True

I pushed: aebf345 but we can keep this possible simplification for a later PR (preferably after 1.2 is released).

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

ogrisel

Pending the points raised by @jjerphan about remaining references to distutils and the following, LGTM.

sklearn/_build_utils/openmp_helpers.py

ogrisel · 2022-11-03T15:16:45Z

azure-pipelines.yml

-# Check compilation with intel C++ compiler (ICC)
- template: build_tools/azure/posix.yml
-  parameters:
-    name: Linux_Nightly_ICC
-    vmImage: ubuntu-20.04
-    dependsOn: [git_commit, linting]
-    condition: |
-      and(
-        succeeded(),
-        not(contains(dependencies['git_commit']['outputs']['commit.message'], '[ci skip]')),
-        or(eq(variables['Build.Reason'], 'Schedule'),
-           contains(dependencies['git_commit']['outputs']['commit.message'], '[icc-build]')
-        )
-      )
-    matrix:
-      pylatest_conda_forge_mkl:
-        DISTRIB: 'conda'
-        LOCK_FILE: 'build_tools/azure/pylatest_conda_forge_mkl_no_coverage_linux-64_conda.lock'
-        COVERAGE: 'false'
-        BUILD_WITH_ICC: 'true'
-


I think we should keep this, assuming setting the CC env variable works for setuptools. But it's true it would be better to test it at some point. But I would not make this a blocker for this PR.

sklearn/inspection/setup.py

ogrisel · 2022-11-03T15:35:51Z

I prefer to use sklearn.show_versions() because it displays actual runtime information. The build time information from numpy might be wrong (and is less precise).

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

jjerphan

LGTM if CI and CD are 🟢!

…#24563) Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

thomasjpfan added 4 commits October 2, 2022 17:57

BLD Migrate away from distutils and only use setuptools

771573e

CI [cd build]

9ba6a47

CLN Reduce diff

68f84a1

CI [cd build]

c524327

thomasjpfan commented Oct 2, 2022

View reviewed changes

thomasjpfan added 3 commits October 5, 2022 15:10

FIX Removes remaining setup.py file [cd build]

0c510bf

Merge remote-tracking branch 'upstream/main' into wheels/remove_numpy…

739c11a

…_distutils_pr

CI [cd build gh]

d0f4a7a

thomasjpfan added 2 commits October 14, 2022 14:23

Merge remote-tracking branch 'upstream/main' into wheels/remove_numpy…

ba8a018

…_distutils_pr

CI [cd build gh]

979d764

amueller approved these changes Oct 14, 2022

View reviewed changes

azure-pipelines.yml Outdated Show resolved Hide resolved

thomasjpfan and others added 7 commits October 14, 2022 16:32

CI Remove BUILD_WITH_ICC everywhere [cd build gh]

7baf7d0

Merge branch 'main' into wheels/remove_numpy_distutils_pr

0efa8b1

CLN Remove more distutil references [cd build gh]

5dda5d4

FIX Fixes build_utils [cd build gh]

429eed6

FIX Remove disutils.version [cd build gh]

2f547bc

CI Set minimum setuptools [cd build gh]

bd22900

CI Allow for lower versions of setuptools [cd build gh]

af0b25c

thomasjpfan mentioned this pull request Oct 26, 2022

DOC Use show_config instead of numpy.distutils's get_info #24762

Merged

DOC Better formatting [cd build gh]

bc55cd7

thomasjpfan commented Oct 26, 2022

View reviewed changes

jjerphan self-requested a review October 31, 2022 16:11

jjerphan reviewed Nov 3, 2022

View reviewed changes

ogrisel and others added 2 commits November 3, 2022 15:38

Update comment for builtins.__SKLEARN_SETUP__

aebf345

Apply suggestions from code review

2d162e8

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

ogrisel approved these changes Nov 3, 2022

View reviewed changes

ogrisel added 2 commits November 3, 2022 16:29

Do not use distutils for BLAS/LAPACK inspection

a4db54e

Merge branch 'main' into wheels/remove_numpy_distutils_pr

8a12d9b

thomasjpfan and others added 9 commits November 3, 2022 11:38

Apply suggestions from code review

38b61b5

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

CLN Remove remaining references to intel compilers

dd60027

CLN Remove remaining references to intel compilers [cd build gh]

4c18f92

CLN Remove c++ distutil entry in partition_nodes

24ac0fb

CLN Improve grammar

a9feb59

TST Remove ignore warning in testing [cd build gh]

8661809

CLN Fixes warnings in openmp_helpers

aad2bff

CI [cd build gh]

ee3c884

CI [cd build gh]

98fe1e2

jjerphan approved these changes Nov 3, 2022

View reviewed changes

ogrisel mentioned this pull request Nov 3, 2022

MAINT bump up CI dependencies #24803

Merged

jjerphan merged commit 266d2a2 into scikit-learn:main Nov 3, 2022

jjerphan mentioned this pull request Nov 4, 2022

EHN Optimized CSR-CSR support for Euclidean specializations of PairwiseDistancesReductions #24556

Merged

eli-schwartz mentioned this pull request Nov 20, 2022

Sklearn pip installation broke because of issues with setuptools #24992

Closed

This was referenced Jan 4, 2024

CI Remove unused mkl_no_coverage lock file #28061

Merged

BLD Add Meson support #28040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD Migrate away from distutils and only use setuptools #24563

BLD Migrate away from distutils and only use setuptools #24563

thomasjpfan commented Oct 2, 2022 •

edited

Loading

thomasjpfan Oct 2, 2022

amueller commented Oct 14, 2022

thomasjpfan Oct 26, 2022 •

edited

Loading

jjerphan left a comment •

edited

Loading

jjerphan Nov 3, 2022

ogrisel Nov 3, 2022

thomasjpfan Nov 3, 2022

ogrisel Nov 3, 2022

jjerphan Nov 3, 2022

jjerphan Nov 3, 2022

ogrisel Nov 3, 2022

jjerphan Nov 3, 2022

ogrisel Nov 3, 2022

jjerphan Nov 3, 2022

ogrisel Nov 3, 2022

ogrisel left a comment

ogrisel Nov 3, 2022

ogrisel commented Nov 3, 2022

jjerphan left a comment


	# Python 3.10 deprecates distutils, which is imported by numpy internally
	TEST_CMD="$TEST_CMD -Wignore:The\ distutils:DeprecationWarning"

	# Ignore distutils deprecation warning, used by joblib internally
	TEST_CMD="$TEST_CMD -Wignore:distutils\ Version\ classes\ are\ deprecated:DeprecationWarning"


	You can display the BLAS / LAPACK implementation used by your NumPy / SciPy /
	scikit-learn install with the following commands::

	from numpy.distutils.system_info import get_info
	print(get_info('blas_opt'))
	print(get_info('lapack_opt'))

	if sys.platform == "win32" and ("icc" in compiler or "icl" in compiler):
	return ["/Qopenmp"]
	elif sys.platform == "win32":
	return ["/openmp"]
	elif sys.platform in ("darwin", "linux") and "icc" in compiler:

		@@ -294,6 +243,328 @@ def check_package_status(package, min_version):
		)


		extension_config = {

	# This is a bit (!) hackish: we are setting a global variable so that the
	# main sklearn __init__ can detect if it is being loaded by the setup
	# routine, to avoid attempting to load components that aren't built yet:
	# the numpy distutils extensions that are used by scikit-learn to
	# recursively build the compiled extensions in sub-packages is based on the
	# Python import machinery.
	builtins.__SKLEARN_SETUP__ = True

BLD Migrate away from distutils and only use setuptools #24563

BLD Migrate away from distutils and only use setuptools #24563

Conversation

thomasjpfan commented Oct 2, 2022 • edited Loading

Choose a reason for hiding this comment

amueller commented Oct 14, 2022

thomasjpfan Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

jjerphan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel commented Nov 3, 2022

jjerphan left a comment

Choose a reason for hiding this comment

thomasjpfan commented Oct 2, 2022 •

edited

Loading

thomasjpfan Oct 26, 2022 •

edited

Loading

jjerphan left a comment •

edited

Loading