From 82e2f545653483ed4e43710c67ca021bc4ffa700 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Fri, 8 Nov 2019 13:04:30 +0100 Subject: [PATCH] BLD: Do not pre-cythonize when calling sdist (#15567) --- .circleci/config.yml | 5 ++ MANIFEST.in | 1 - build_tools/azure/install.sh | 6 +- doc/developers/advanced_installation.rst | 6 ++ setup.py | 8 ++ sklearn/_build_utils/__init__.py | 96 ++++++++++-------------- sklearn/setup.py | 9 ++- sklearn/tests/test_common.py | 4 + 8 files changed, 74 insertions(+), 61 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 04c8979fa0d6a..3933d5404202f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -13,6 +13,11 @@ jobs: - NUMPY_VERSION: 1.11.0 - SCIPY_VERSION: 0.17.0 - MATPLOTLIB_VERSION: 1.5.1 + # on conda, this is the latest for python 3.5 + # The following places need to be in sync with regard to Cython version: + # - .circleci config file + # - sklearn/_build_utils/__init__.py + # - advanced installation guide - CYTHON_VERSION: 0.28.5 - SCIKIT_IMAGE_VERSION: 0.12.3 steps: diff --git a/MANIFEST.in b/MANIFEST.in index e36adcae38b0e..04d62596bbf3d 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -5,4 +5,3 @@ recursive-include sklearn *.c *.h *.pyx *.pxd *.pxi *.tp recursive-include sklearn/datasets *.csv *.csv.gz *.rst *.jpg *.txt *.arff.gz *.json.gz include COPYING include README.rst - diff --git a/build_tools/azure/install.sh b/build_tools/azure/install.sh index 0330b40229c1e..1b4955acedbc8 100755 --- a/build_tools/azure/install.sh +++ b/build_tools/azure/install.sh @@ -90,7 +90,8 @@ elif [[ "$DISTRIB" == "conda-pip-latest" ]]; then # we use pypi to test against the latest releases of the dependencies. # conda is still used as a convenient way to install Python and pip. make_conda "python=$PYTHON_VERSION" - python -m pip install numpy scipy joblib cython + python -m pip install -U pip + python -m pip install numpy scipy cython joblib python -m pip install pytest==$PYTEST_VERSION pytest-cov pytest-xdist python -m pip install pandas matplotlib pyamg fi @@ -118,5 +119,8 @@ except ImportError: print('pandas not installed') " python -m pip list + +# Use setup.py instead of `pip install -e .` to be able to pass the -j flag +# to speed-up the building multicore CI machines. python setup.py build_ext --inplace -j 3 python setup.py develop diff --git a/doc/developers/advanced_installation.rst b/doc/developers/advanced_installation.rst index fc2639a83c0a5..6f73f982fda63 100644 --- a/doc/developers/advanced_installation.rst +++ b/doc/developers/advanced_installation.rst @@ -101,6 +101,12 @@ Build dependencies Building Scikit-learn also requires: +.. + # The following places need to be in sync with regard to Cython version: + # - .circleci config file + # - sklearn/_build_utils/__init__.py + # - advanced installation guide + - Cython >= 0.28.5 - A C/C++ compiler and a matching OpenMP_ runtime library. See the :ref:`platform system specific instructions diff --git a/setup.py b/setup.py index d3b2494432f81..dc0791f609ca6 100755 --- a/setup.py +++ b/setup.py @@ -161,6 +161,7 @@ def configuration(parent_package='', top_path=None): os.remove('MANIFEST') from numpy.distutils.misc_util import Configuration + from sklearn._build_utils import _check_cython_version config = Configuration(None, parent_package, top_path) @@ -171,6 +172,12 @@ def configuration(parent_package='', top_path=None): delegate_options_to_subpackages=True, quiet=True) + # Cython is required by config.add_subpackage for templated extensions + # that need the tempita sub-submodule. So check that we have the correct + # version of Cython so as to be able to raise a more informative error + # message from the start if it's not the case. + _check_cython_version() + config.add_subpackage('sklearn') return config @@ -240,6 +247,7 @@ def setup_package(): len(sys.argv) >= 2 and ('--help' in sys.argv[1:] or sys.argv[1] in ('--help-commands', 'egg_info', + 'dist_info', '--version', 'clean'))): # For these actions, NumPy is not required diff --git a/sklearn/_build_utils/__init__.py b/sklearn/_build_utils/__init__.py index 1d6c1eaf607a6..aea76d8fd42a6 100644 --- a/sklearn/_build_utils/__init__.py +++ b/sklearn/_build_utils/__init__.py @@ -6,7 +6,6 @@ import os - from distutils.version import LooseVersion import contextlib @@ -14,67 +13,50 @@ DEFAULT_ROOT = 'sklearn' -# on conda, this is the latest for python 3.5 + +# The following places need to be in sync with regard to Cython version: +# - .circleci config file +# - sklearn/_build_utils/__init__.py +# - advanced installation guide CYTHON_MIN_VERSION = '0.28.5' -def build_from_c_and_cpp_files(extensions): - """Modify the extensions to build from the .c and .cpp files. - - This is useful for releases, this way cython is not required to - run python setup.py install. - """ - for extension in extensions: - sources = [] - for sfile in extension.sources: - path, ext = os.path.splitext(sfile) - if ext in ('.pyx', '.py'): - if extension.language == 'c++': - ext = '.cpp' - else: - ext = '.c' - sfile = path + ext - sources.append(sfile) - extension.sources = sources - - -def maybe_cythonize_extensions(top_path, config): - """Tweaks for building extensions between release and development mode.""" - with_openmp = check_openmp_support() +def _check_cython_version(): + message = ('Please install Cython with a version >= {0} in order ' + 'to build a scikit-learn from source.').format( + CYTHON_MIN_VERSION) + try: + import Cython + except ModuleNotFoundError: + # Re-raise with more informative error message instead: + raise ModuleNotFoundError(message) - is_release = os.path.exists(os.path.join(top_path, 'PKG-INFO')) + if LooseVersion(Cython.__version__) < CYTHON_MIN_VERSION: + message += (' The current version of Cython is {} installed in {}.' + .format(Cython.__version__, Cython.__path__)) + raise ValueError(message) - if is_release: - build_from_c_and_cpp_files(config.ext_modules) - else: - message = ('Please install cython with a version >= {0} in order ' - 'to build a scikit-learn development version.').format( - CYTHON_MIN_VERSION) - try: - import Cython - if LooseVersion(Cython.__version__) < CYTHON_MIN_VERSION: - message += ' Your version of Cython was {0}.'.format( - Cython.__version__) - raise ValueError(message) - from Cython.Build import cythonize - except ImportError as exc: - exc.args += (message,) - raise - - n_jobs = 1 - with contextlib.suppress(ImportError): - import joblib - if LooseVersion(joblib.__version__) > LooseVersion("0.13.0"): - # earlier joblib versions don't account for CPU affinity - # constraints, and may over-estimate the number of available - # CPU particularly in CI (cf loky#114) - n_jobs = joblib.effective_n_jobs() - - config.ext_modules = cythonize( - config.ext_modules, - nthreads=n_jobs, - compile_time_env={'SKLEARN_OPENMP_SUPPORTED': with_openmp}, - compiler_directives={'language_level': 3}) + +def cythonize_extensions(top_path, config): + """Check that a recent Cython is available and cythonize extensions""" + _check_cython_version() + from Cython.Build import cythonize + + with_openmp = check_openmp_support() + n_jobs = 1 + with contextlib.suppress(ImportError): + import joblib + if LooseVersion(joblib.__version__) > LooseVersion("0.13.0"): + # earlier joblib versions don't account for CPU affinity + # constraints, and may over-estimate the number of available + # CPU particularly in CI (cf loky#114) + n_jobs = joblib.cpu_count() + + config.ext_modules = cythonize( + config.ext_modules, + nthreads=n_jobs, + compile_time_env={'SKLEARN_OPENMP_SUPPORTED': with_openmp}, + compiler_directives={'language_level': 3}) def gen_from_templates(templates, top_path): diff --git a/sklearn/setup.py b/sklearn/setup.py index 0c7f19f23d39c..cc257c30e6f43 100644 --- a/sklearn/setup.py +++ b/sklearn/setup.py @@ -1,6 +1,7 @@ +import sys import os -from sklearn._build_utils import maybe_cythonize_extensions +from sklearn._build_utils import cythonize_extensions from sklearn._build_utils.deprecated_modules import ( _create_deprecated_modules_files ) @@ -78,7 +79,11 @@ def configuration(parent_package='', top_path=None): # add the test directory config.add_subpackage('tests') - maybe_cythonize_extensions(top_path, config) + # Skip cythonization as we do not want to include the generated + # C/C++ files in the release tarballs as they are not necessarily + # forward compatible with future versions of Python for instance. + if 'sdist' not in sys.argv: + cythonize_extensions(top_path, config) return config diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index c2938b9f6fc08..33eee4ae554a5 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -125,6 +125,10 @@ def test_check_estimator_generate_only(): def test_configure(): # Smoke test the 'configure' step of setup, this tests all the # 'configure' functions in the setup.pys in scikit-learn + # This test requires Cython which is not necessarily there when running + # the tests of an installed version of scikit-learn or when scikit-learn + # is installed in editable mode by pip build isolation enabled. + pytest.importorskip("Cython") cwd = os.getcwd() setup_path = os.path.abspath(os.path.join(sklearn.__path__[0], '..')) setup_filename = os.path.join(setup_path, 'setup.py')