[MRG] BLD Specify build time dependencies via pyproject.toml #16244

jeremiedbb · 2020-01-27T17:17:22Z

Fixes #15298
I basically took all what's done by adrin in #15533 but the cythonizing part.

That was discussed in several previous PRs (#15335, #15533, #15567) but we decided to not add it right before a release. We now have more time before the next release. Let's not delay that to the last minute.

It can also let us use external build tools like extension-helpers (#15917) or oldest-supported-numpy (https://pypi.org/project/oldest-supported-numpy/).

rth · 2020-02-03T10:03:27Z

I agree it would be good to merge this some time before the release. Is anything holding this?

jeremiedbb · 2020-02-04T13:57:32Z

Is anything holding this?

We need to decide if we do want this. The main concern is editable mode vs build isolation. With a "pyproject.toml", pip install -e . makes an isolated build. As a consequence, pip entirely re-builds sklearn each time. To get an incremental build, we'd need to run pip install --no-build-isolation -e .. In that case, we need to install build dependencies manually (as we do today).

So in developer mode, the new way to do is:

install build dependencies (same as today)
pip install --no-build-isolation -e . (1 additional flag)

Where I see the value of adding "pyproject.toml" is for people who install sklearn from source distribution. Today, one needs to install cython first otherwise pip will fail. But usually you'll never have to build sklearn again in that case, so here an isolated build makes totally sense.

jeremiedbb · 2020-02-04T14:12:18Z

To keep track of previous discussions, the behavior reported in #15533 (comment) can't be reproduced. We tried it again with olivier and the build is still isolated.

NicolasHug · 2020-02-04T15:58:09Z

pip entirely re-builds scklearn each time

Are you sure -- do you have a ref?

From my understanding of https://pip.pypa.io/en/stable/reference/pip/#pep-517-and-518-support, scikit-learn woudn't be entirely re-built. The worst thing that I guess could happen is that the build dependencies are still installed again in the temp dir, even if they are already satisfied. Though I hope that pip is smart enough to avoid that?

I.e. I assume that merging this PR wouldn't change our workflow since we would still manually install cython numpy scipy and then pip install -e . or make in would work as before?

jeremiedbb · 2020-02-04T16:00:37Z

Are you sure -- do you have a ref?

tested it with different versions of pip from 10.X to 20.X. In verbose mode you can see the compilation happening twice when running pip install -e . twice.

I.e. I assume that merging this PR wouldn't change our workflow since we would still manually install cython numpy scipy and then pip install -e . or make in would work as before?

No, as I said, pip install -e . would re-compile everything. You'll need to run pip install --no-build-isolation -e ..

Make in would work as before

jvesely · 2020-02-04T17:30:35Z

There's a benefit beyond pulling in cython automatically; pip creates and caches wheels so it doesn't have to be rebuilt every time scikit-learn is installed.
This improves CI times on platforms that lack official scikit-learn wheels (arm64, pypy3).

NicolasHug · 2020-02-04T19:38:49Z

In verbose mode you can see the compilation happening twice when running pip install -e . twice

Indeed. This is because numpy is installed in the temp dir and Cython believes that it needs to recompile all its files that have a cimport numpy, because the numpy headers (in the temp dir) have a newer timestamp.

I think that if pip didn't installed numpy in the temp dir, the compilation issue would disappear.

I've opened an issue on https://discuss.python.org (currently going through the spam filter, will link to it when available).

EDIT: link to post

jeremiedbb · 2020-02-06T11:07:01Z

I think it's due to cython being installed in the temp dir.

Compiling sklearn/cluster/_k_means_fast.pyx because it depends on /tmp/pip-build-env-uo_4y7gb/overlay/lib/python3.6/site-packages/Cython/Includes/numpy/__init__.pxd.

The detected change is in cython's header for numpy c api.

It also occurs for non numpy stuff:

Compiling sklearn/decomposition/_cdnmf_fast.pyx because it depends on /tmp/pip-build-env-uo_4y7gb/overlay/lib/python3.6/site-packages/Cython/Includes/libc/math.pxd.
Compiling sklearn/utils/_openmp_helpers.pyx because it depends on /tmp/pip-build-env-uo_4y7gb/overlay/lib/python3.6/site-packages/Cython/Includes/openmp.pxd.

The problem is that we can't expect to have cython in the original env.

ogrisel · 2020-02-06T12:16:06Z

Maybe Cython could be made smarter by using a sha256 hash of the the dependent source files instead of file timestamps?

ogrisel · 2020-02-06T12:23:50Z

Looking at the source code of Cython, I think only mtime-based change detection is implemented: https://github.com/cython/cython/blob/master/Cython/Build/Dependencies.py

ogrisel · 2020-02-06T12:27:21Z

I have the feeling that we will need to use the --no-build-isolation flag to benefit from incremental building our cython extensions...

NicolasHug · 2020-02-06T13:39:45Z

yeah I guess that's fine. It will only impact contributors that modify Cython files and that can't use make in.

We'd need to update the note in https://scikit-learn.org/stable/developers/contributing.html#how-to-contribute to introduce the --no-build-isolation there (and also suggest make in instead). Also the note in https://scikit-learn.org/stable/developers/advanced_installation.html#building-from-source

thomasjpfan · 2020-02-07T03:33:23Z

If we want to service those who does not have make, we can advise contributors to: python setup.py build_ext -i (and with -j if they want to do it faster).

NicolasHug

LGTM. Maybe worth pinging other core devs for completeness

NicolasHug · 2020-02-07T12:49:29Z

build_tools/azure/install.sh

+    # to speed-up the building multicore CI machines.
+    python setup.py build_ext --inplace -j 3
+    python setup.py develop
+fi


@NicolasHug you did not finish your comment, or am I missing something ?

there was a missing line return but it's been fixed (it showed on the main diff, maybe not in the PR comments page)

oh I see. I must have fixed that in a recent commit indeed :)

rth

I think using --no-build-isolation by contributors is a modest price to pay for the advantages this could bring.

If we want to service those who does not have make, we can advise contributors to: python setup.py build_ext -i (and with -j if they want to do it faster).

I am sad there is no equivalent of -j for pip install (or at least I haven't found one), but I was under the impression that python setup.py ... use is now discouraged, and I'm also not so keen on make in as that's not universal with respect to other projects.

pip install --no-build-isolation -e . is still the most standard way IMO.

ogrisel

LGTM as well.

+1 for keeping the message simple and only mentioning the pip install --no-build-isolation -e . command as it's cross-platform.

ogrisel · 2020-02-07T21:49:11Z

pyproject.toml

+    "wheel",
+    "Cython>=0.28.5",
+    "numpy>=1.11.0",
+    "scipy>=0.17.0",


#15106 will be merged soon. Let's bump-up numpy to 1.13.3 and scipy to 0.19.1.

ogrisel · 2020-02-07T22:01:31Z

BTW the discussion is going on at https://discuss.python.org/t/pep-518-and-editable-mode-dont-install-already-satisfied-dependencies/3124 if you are interested in contributing to pip to improve incremental building by reusing an existing build env instead of creating a new one each time.

adrinjalali

thanks @jeremiedbb

doc/developers/advanced_installation.rst

doc/developers/contributing.rst

pyproject.toml

adrinjalali · 2020-02-10T11:15:23Z

MANIFEST.in

@@ -5,3 +5,4 @@ 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
+include pyproject.toml


This is needed because we need this file in the sdist? Otherwise in the binary releases we don't need to have it. Is my understanding correct?

adrinjalali

Thanks @jeremiedbb

…16244) * add pyproject.toml * do it in CI * cln * no build isolation in the doc * bump dependencies * no-build-isolation everywhere needed

add pyproject.toml

f4c6f3c

jeremiedbb added Build / CI Needs work labels Jan 27, 2020

jeremiedbb added this to the 0.23 milestone Jan 27, 2020

do it in CI

019105e

jeremiedbb changed the title ~~[WIP] Specify build time dependencies via pyproject.toml~~ [MRG] Specify build time dependencies via pyproject.toml Feb 4, 2020

jeremiedbb removed the Needs work label Feb 4, 2020

jeremiedbb changed the title ~~[MRG] Specify build time dependencies via pyproject.toml~~ [MRG] BLD Specify build time dependencies via pyproject.toml Feb 4, 2020

cln

99559f5

no build isolation in the doc

1a0fc4b

rth mentioned this pull request Feb 6, 2020

[MR+3] CI dropping python-3.5 #15106

Merged

NicolasHug approved these changes Feb 7, 2020

View reviewed changes

rth approved these changes Feb 7, 2020

View reviewed changes

ogrisel approved these changes Feb 7, 2020

View reviewed changes

jeremiedbb added 2 commits February 10, 2020 11:30

bump dependencies

470d7f4

no-build-isolation everywhere needed

85f87ba

adrinjalali reviewed Feb 10, 2020

View reviewed changes

adrinjalali approved these changes Feb 10, 2020

View reviewed changes

thomasjpfan approved these changes Feb 10, 2020

View reviewed changes

adrinjalali merged commit 97d49f2 into scikit-learn:master Feb 11, 2020

sebnyberg mentioned this pull request Nov 5, 2020

Bump scikit-learn version to 0.23.x snorkel-team/snorkel#1620

Closed

NicolasHug mentioned this pull request Jun 7, 2021

Build dependencies in developer guide seem incorrect #20212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] BLD Specify build time dependencies via pyproject.toml #16244

[MRG] BLD Specify build time dependencies via pyproject.toml #16244

jeremiedbb commented Jan 27, 2020 •

edited

Loading

rth commented Feb 3, 2020

jeremiedbb commented Feb 4, 2020 •

edited

Loading

jeremiedbb commented Feb 4, 2020

NicolasHug commented Feb 4, 2020

jeremiedbb commented Feb 4, 2020 •

edited

Loading

jvesely commented Feb 4, 2020

NicolasHug commented Feb 4, 2020 •

edited

Loading

jeremiedbb commented Feb 6, 2020

ogrisel commented Feb 6, 2020

ogrisel commented Feb 6, 2020

ogrisel commented Feb 6, 2020

NicolasHug commented Feb 6, 2020

thomasjpfan commented Feb 7, 2020

NicolasHug left a comment

NicolasHug Feb 7, 2020

jeremiedbb Feb 10, 2020

NicolasHug Feb 10, 2020

jeremiedbb Feb 10, 2020

rth left a comment

ogrisel left a comment •

edited

Loading

ogrisel Feb 7, 2020

adrinjalali Feb 10, 2020

ogrisel commented Feb 7, 2020

adrinjalali left a comment

adrinjalali Feb 10, 2020

adrinjalali left a comment

[MRG] BLD Specify build time dependencies via pyproject.toml #16244

[MRG] BLD Specify build time dependencies via pyproject.toml #16244

Conversation

jeremiedbb commented Jan 27, 2020 • edited Loading

rth commented Feb 3, 2020

jeremiedbb commented Feb 4, 2020 • edited Loading

jeremiedbb commented Feb 4, 2020

NicolasHug commented Feb 4, 2020

jeremiedbb commented Feb 4, 2020 • edited Loading

jvesely commented Feb 4, 2020

NicolasHug commented Feb 4, 2020 • edited Loading

jeremiedbb commented Feb 6, 2020

ogrisel commented Feb 6, 2020

ogrisel commented Feb 6, 2020

ogrisel commented Feb 6, 2020

NicolasHug commented Feb 6, 2020

thomasjpfan commented Feb 7, 2020

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug Feb 7, 2020

Choose a reason for hiding this comment

jeremiedbb Feb 10, 2020

Choose a reason for hiding this comment

NicolasHug Feb 10, 2020

Choose a reason for hiding this comment

jeremiedbb Feb 10, 2020

Choose a reason for hiding this comment

rth left a comment

Choose a reason for hiding this comment

ogrisel left a comment • edited Loading

Choose a reason for hiding this comment

ogrisel Feb 7, 2020

Choose a reason for hiding this comment

adrinjalali Feb 10, 2020

Choose a reason for hiding this comment

ogrisel commented Feb 7, 2020

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali Feb 10, 2020

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

jeremiedbb commented Jan 27, 2020 •

edited

Loading

jeremiedbb commented Feb 4, 2020 •

edited

Loading

jeremiedbb commented Feb 4, 2020 •

edited

Loading

NicolasHug commented Feb 4, 2020 •

edited

Loading

ogrisel left a comment •

edited

Loading