[MRG+2] Rewrite setup.py files to handle cython dependencies by lesteve · Pull Request #7719 · scikit-learn/scikit-learn

lesteve · 2016-10-21T11:23:58Z

Fixes #7094 and maybe related issues if there are other. Closes #7708.

What does this implement/fix? Explain your changes.

Basically build_tools/cythonize.py doesn't take into account cython files dependencies. For example if you change _criterion.pxd the dependents .pyx don't get recompiled, potentially yielding to confusing segmentation faults.

The thing is that cython takes into acount correctly if you write your setup.py carefull (mostly by using Cython.Build.cythonize and switching sources between .c and .pyx as appropriate). Compared to #7708 which is a work-around this PR is trying to tackle the root problem.

Upsides:

make in in is almost instant if you do not change cython files. If you do change cython files, only affected cython files are generated. As as developer this cuts down the time of compiling scikit-learn by a whole lot.
dependency taken correctly into account. Try touching sklearn/tree/_criterion.pxd, and you'll see that all the sklearn/tree cython files gets recompiled.

Downsides:

the recompilation is based on file timestamps, so only on Travis we lose the caching based on content hash but as I argued in Tests failing on master due to tree recythonization issues #7094 the caching of .c generated files only saves 8% on individual build.

This is WIP because I haven't disabled caching on Travis yet. Just wanted to show the changes in setup.py files and get feed-back for now.

ping @raghavrv, @jnothman, @amueller.

pandas changes its sdist to cythonize all the files, maybe we need to do something like this depending how we release. I need to double-check this. Edit: sdist seems to work fine, pandas approach is a bit different where they have custom classes they pass to cmdclass in setup.
try with older versions of cython. pandas has a > 0.19.1 requirement, not sure why. Edit: Cython >= 0.23 is mentioned in the doc/developers/advanced_installation.rst and I checked it does compile fine with 0.23 and the tests run fine on my machine.

raghavrv · 2016-10-21T11:30:14Z

+10000 Thanks heaps for working on this...

lesteve

A few inline comments to help the review

lesteve · 2016-10-21T11:37:12Z

sklearn/_build_utils/__init__.py

+    is_cpp_filename = filename.endswith('.cpp')
+
+    # files in src are .c and .cpp files that are not cython-generated
+    if 'src/' in filename and (is_c_filename or is_cpp_filename):


This is the most brittle part of the PR I think. I don't know whether there is a better way of doing it, suggestions welcome.

I guess one possible improvement would be to add an additional argument in add_cython_extension like cython_generated_sources so that we can discriminate more easily betwen the .c files coming from cython and the ones in the repo.

lesteve · 2016-10-21T11:39:04Z

sklearn/_build_utils/__init__.py

+        sources = [get_cython_source(filename) for filename in sources]
+    config.add_extension(name, sources, **kwargs)
+
+    config.ext_modules = cythonize(config.ext_modules)


Hmmm I need to double-check this, maybe it should be inside an if is_dev_version

lesteve · 2016-10-21T11:41:11Z

sklearn/_build_utils/__init__.py

+            raise ValueError('Please install cython in order '
+                             'to build a scikit-learn development version')
+
+        sources = [get_cython_source(filename) for filename in sources]


This is the part that switches between using .c{,pp} files when building a release and the .pyx when building a dev version.

By using Cython.Build.cythonize and switching between .c and .pyx files as appropriate cython dependencies are correctly taken into account.

raghavrv · 2016-10-21T14:00:36Z

Just to note that sometimes there are inter module dependencies that needs to be recythonized too... For instance any change in the tree's cython sources needs a recythonizing of the gradient boosting... Does this PR handle that?

lesteve · 2016-10-21T14:09:49Z

Just to note that sometimes there are inter module dependencies that needs to be recythonized too... For instance any change in the tree's cython sources needs a recythonizing of the gradient boosting... Does this PR handle that?

It seems like it does:

make in
touch sklearn/tree/_criterion.pxd
make in

Excerpt of the output:

Compiling sklearn/ensemble/_gradient_boosting.pyx because it depends on ./sklearn/tree/_criterion.pxd.
Compiling sklearn/tree/_tree.pyx because it depends on ./sklearn/tree/_criterion.pxd.
Compiling sklearn/tree/_splitter.pyx because it depends on ./sklearn/tree/_criterion.pxd.
Compiling sklearn/tree/_criterion.pyx because it depends on sklearn/tree/_criterion.pxd.
Compiling sklearn/tree/_utils.pyx because it depends on ./sklearn/tree/_criterion.pxd.
[1/5] Cythonizing sklearn/tree/_criterion.pyx
[2/5] Cythonizing sklearn/tree/_splitter.pyx
[3/5] Cythonizing sklearn/ensemble/_gradient_boosting.pyx
[4/5] Cythonizing sklearn/tree/_tree.pyx
[5/5] Cythonizing sklearn/tree/_utils.pyx

raghavrv · 2016-10-21T14:12:26Z

Amazing *_*

TomDLT · 2016-10-21T15:31:28Z

This is nice !

lesteve

Added a few of my comments that got folded in outdated diffs.

lesteve · 2016-10-21T15:48:55Z

sklearn/_build_utils/__init__.py

+    is_cpp_filename = filename.endswith('.cpp')
+
+    # files in src are .c and .cpp files that are not cython-generated
+    if ('src' + os.sep) in filename and (is_c_filename or is_cpp_filename):


This is the most brittle part of the PR I think. I don't know whether there is a better way of doing it, suggestions welcome.

I guess one possible improvement would be to add an additional argument in add_cython_extension like cython_generated_sources so that we can discriminate more easily betwen the .c files coming from cython and the ones in the repo.

This is fine I think. Optionally you could just hardcode a list with all the non cython source paths...

How about checking if there exists a pyx file with the same name in the same folder?

That's a simple way of doing it indeed.

Or we could add .pyx extension when it's a cython file, and add a function like:

import os.path def no_cythonize(extensions, **_ignore): 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 return extensions

(copy-paste from cython doc)

I think it would make more sense this way around indeed. The .pyx would be mentioned in the setup.py and you would do the switching to .c or .cpp only for releases. I have to say I didn't fully understand the no_cythonize function when I read the doc but it makes more sense now (in particular the naming is confusing because it's not really related to Cython.Build.cythonize).

lesteve · 2016-10-21T15:48:59Z

sklearn/_build_utils/__init__.py

+    is_dev_version = not os.path.exists(os.path.join(top_path, 'PKG-INFO'))
+
+    if is_dev_version:
+        sources = [get_cython_source(filename) for filename in sources]


This is the part that switches between using .c{,pp} files when building a release and the .pyx when building a dev version.

amueller

This looks pretty good if it works as expected. Also, I feel like this shouldn't live in scikit-learn alone. I feel like at least numpy, pandas and scikit-image need the same. Shouldn't we share a solution?

amueller · 2016-10-24T15:35:55Z

sklearn/_build_utils/__init__.py

+    is_cpp_filename = filename.endswith('.cpp')
+
+    # files in src are .c and .cpp files that are not cython-generated
+    if ('src' + os.sep) in filename and (is_c_filename or is_cpp_filename):


How about checking if there exists a pyx file with the same name in the same folder?

amueller · 2016-10-24T15:41:24Z

sklearn/_build_utils/__init__.py

+            raise ValueError('Please install cython in order '
+                             'to build a scikit-learn development version')
+
+        config.ext_modules = cythonize(config.ext_modules)


So that takes care of the caching?

From https://cython.readthedocs.io/en/latest/src/reference/compilation.html#compiling-with-distutils:

The cythonize command also allows for multi-threaded compilation and dependency resolution. Recompilation will be skipped if the target file is up to date with its main source file and dependencies.

lesteve · 2016-10-24T16:37:55Z

This looks pretty good if it works as expected.

It does work as expected in the few tests I run like touching sklearn/tree/_criterion.pxd and making sure that all the dependents are recompiled (see #7719 (comment)). If you have more complicated scenarios that could go wrong I'd be happy to try them out.

Also, I feel like this shouldn't live in scikit-learn alone. I feel like at least numpy, pandas and scikit-image need the same. Shouldn't we share a solution?

Maybe we could have this in scikit-learn for some time so that it gets stress-tested a bit before consolidating across multiple projects?

amueller · 2016-10-24T17:50:42Z

Maybe we could have this in scikit-learn for some time so that it gets stress-tested a bit before consolidating across multiple projects?

Yes, but we could also try reaching out to others and see what their approaches and requirements are?

jnothman · 2016-10-25T12:58:37Z

Why WIP?

raghavrv · 2016-10-25T13:36:40Z

Remove build caching from Travis

+:100:

lesteve · 2016-10-25T13:37:10Z

As I said in the issue description:

This is WIP because I haven't disabled caching on Travis yet. Just wanted to show the changes in setup.py files and get feed-back for now.

No one complained about the Travis cache being useless as a side effect of this PR so I just pushed a commit that removes the build products from theTravis cache and changed the status to MRG.

Cython dependencies are taken care of by Cython.Build.cythonize and based on file timestamps, so .C and .so files will always be rebuild from scratch on each build in Travis.

More natural this way. Tweak the extensions to generate from .c and .cpp files for a release.

GaelVaroquaux · 2016-10-25T18:18:00Z

Also, I feel like this shouldn't live in scikit-learn alone. I feel like at
least numpy, pandas and scikit-image need the same. Shouldn't we share a
solution?
Maybe we could have this in scikit-learn for some time so that it gets
stress-tested a bit before consolidating across multiple projects?

+1. And then contribute it to Cython, IMHO.

ogrisel

LGTM.

jnothman

I've not tested that this works. Otherwise, it looks good to me...

jnothman · 2016-11-01T05:32:11Z

sklearn/utils/sparsetools/setup.py

 import numpy


+


Please remove added newline

Interestingly flake8_diff.sh is not able to spot these. Quickly looking at it this is because we do the diff without context (--unified=0) in order to avoid noise coming from lines that the PR hasn't touched.

jnothman · 2016-11-01T05:33:52Z

sklearn/_build_utils/__init__.py

+        try:
+            from Cython.Build import cythonize
+        except ImportError:
+            raise ValueError('Please install cython in order '


Given that we keep having issues related to cython versioning, I think we should be checking for or specifying version.

jnothman · 2016-11-01T05:35:20Z

sklearn/_build_utils/__init__.py

+        extension.sources = sources
+
+
+def tweak_extensions(top_path, config):


Can we call this something less opaque: conditional_cythonize or cythonize_if_development?

jnothman · 2016-11-01T05:38:26Z

sklearn/_build_utils/__init__.py

    return cblas_libs, blas_info
+
+
+def tweak_extensions_to_build_from_c_and_cpp_files(extensions):


maybe just rename_cython_extensions?

lesteve · 2016-11-02T10:19:52Z

@jnothman I have tackled your comments.

jnothman · 2016-11-02T10:40:18Z

sklearn/_build_utils/__init__.py

-        tweak_extensions_to_build_from_c_and_cpp_files(config.ext_modules)
+        build_from_c_and_cpp_files(config.ext_modules)
    else:
+        message = ('Please install cython with a version >= 0.23'


We should really pull this min version out as a constant.

Good idea, done.

jnothman · 2016-11-02T10:40:34Z

Otherwise LGTM.

now that cython >= 0.23 requirement is enforced in setup.py

No easy way to put comments inside multi-line command

jnothman · 2016-11-02T11:04:42Z

You can merge after CI approves. thanks!

raghavrv · 2016-11-02T12:00:30Z

Thank a lot for this @lesteve!

jnothman · 2016-11-02T12:12:56Z

Is there a benefit to having this in 0.18.1?

On 2 November 2016 at 23:00, Raghav RV notifications@github.com wrote:

Thank a lot for this @lesteve https://github.com/lesteve!

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#7719 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz69hzUbKLEXZENHwRSUVnqSZEiufFks5q6HtfgaJpZM4KdIXJ
.

raghavrv · 2016-11-02T12:16:03Z

Having this included in the backports will help avoid travis cache based error in the backport PR #7672 :D

lesteve · 2016-11-02T12:23:53Z

Thank a lot for this @lesteve!

@raghavrv your work-around PR motivated me to find a better solution, so you kind of initiated this ;-) !

…cikit-learn#7719) * Rewriting of cythonization in setup.py By using Cython.Build.cythonize and switching between .c and .pyx files as appropriate cython dependencies are correctly taken into account. * Use cythonize once on the root config rather than in each subpackage * Fix for Windows * Remove caching from Travis Cython dependencies are taken care of by Cython.Build.cythonize and based on file timestamps, so .C and .so files will always be rebuild from scratch on each build in Travis. * Specify .pyx in setup.files for cython generated extensions More natural this way. Tweak the extensions to generate from .c and .cpp files for a release. * COSMIT Remove commented out code * Check cython version is greater than 0.23 * COSMIT better names for functions * flake8 fix (imported module not at top of file) * Install cython 0.23 for Python 2.6 now that cython >= 0.23 requirement is enforced in setup.py * Use module constant for minimum required cython version * Fix Travis install.sh No easy way to put comments inside multi-line command

TomDLT · 2016-11-24T11:12:54Z

Really enjoying this change, compiling is very much faster !

…cikit-learn#7719) * Rewriting of cythonization in setup.py By using Cython.Build.cythonize and switching between .c and .pyx files as appropriate cython dependencies are correctly taken into account. * Use cythonize once on the root config rather than in each subpackage * Fix for Windows * Remove caching from Travis Cython dependencies are taken care of by Cython.Build.cythonize and based on file timestamps, so .C and .so files will always be rebuild from scratch on each build in Travis. * Specify .pyx in setup.files for cython generated extensions More natural this way. Tweak the extensions to generate from .c and .cpp files for a release. * COSMIT Remove commented out code * Check cython version is greater than 0.23 * COSMIT better names for functions * flake8 fix (imported module not at top of file) * Install cython 0.23 for Python 2.6 now that cython >= 0.23 requirement is enforced in setup.py * Use module constant for minimum required cython version * Fix Travis install.sh No easy way to put comments inside multi-line command

lesteve force-pushed the rewrite-setup-cythonize branch from 2f13e88 to ed6b671 Compare October 21, 2016 11:35

lesteve commented Oct 21, 2016

View reviewed changes

Rewriting of cythonization in setup.py

1eade7d

By using Cython.Build.cythonize and switching between .c and .pyx files as appropriate cython dependencies are correctly taken into account.

lesteve force-pushed the rewrite-setup-cythonize branch from ed6b671 to 1eade7d Compare October 21, 2016 11:49

Use cythonize once on the root config rather than in each subpackage

602d035

Fix for Windows

768399b

raghavrv mentioned this pull request Oct 21, 2016

[MRG] ENH Add a commit flag [ci recythonize] to recythonize all the sources. #7708

Closed

lesteve commented Oct 21, 2016

View reviewed changes

amueller reviewed Oct 24, 2016

View reviewed changes

Remove caching from Travis

7c731a6

Cython dependencies are taken care of by Cython.Build.cythonize and based on file timestamps, so .C and .so files will always be rebuild from scratch on each build in Travis.

lesteve force-pushed the rewrite-setup-cythonize branch from 23ee373 to 7c731a6 Compare October 25, 2016 14:29

Specify .pyx in setup.files for cython generated extensions

69c5ae1

More natural this way. Tweak the extensions to generate from .c and .cpp files for a release.

lesteve force-pushed the rewrite-setup-cythonize branch from 043f6d6 to 69c5ae1 Compare October 25, 2016 16:09

lesteve changed the title ~~[WIP] Rewrite setup.py files to handle cython dependencies~~ [MRG] Rewrite setup.py files to handle cython dependencies Oct 25, 2016

ogrisel approved these changes Oct 27, 2016

View reviewed changes

COSMIT Remove commented out code

52eb346

jnothman changed the title ~~[MRG] Rewrite setup.py files to handle cython dependencies~~ [MRG+1] Rewrite setup.py files to handle cython dependencies Nov 1, 2016

jnothman requested changes Nov 1, 2016

View reviewed changes

Check cython version is greater than 0.23

208688e

COSMIT better names for functions

51f7cff

jnothman reviewed Nov 2, 2016

View reviewed changes

jnothman changed the title ~~[MRG+1] Rewrite setup.py files to handle cython dependencies~~ [MRG+2] Rewrite setup.py files to handle cython dependencies Nov 2, 2016

lesteve added 4 commits November 2, 2016 11:46

flake8 fix (imported module not at top of file)

09ff110

Install cython 0.23 for Python 2.6

e377225

now that cython >= 0.23 requirement is enforced in setup.py

Use module constant for minimum required cython version

47fad2d

Fix Travis install.sh

56de34d

No easy way to put comments inside multi-line command

jnothman approved these changes Nov 2, 2016

View reviewed changes

jnothman merged commit 6a2d8d5 into scikit-learn:master Nov 2, 2016

lesteve deleted the rewrite-setup-cythonize branch November 2, 2016 12:08

raghavrv added the Build / CI label Nov 2, 2016

jnothman added this to the 0.18.1 milestone Nov 2, 2016

ericpre mentioned this pull request Oct 15, 2017

fix-enh: post-checkout hooks hyperspy/hyperspy#1754

Closed

		extension.sources = sources


		def tweak_extensions(top_path, config):

		return cblas_libs, blas_info


		def tweak_extensions_to_build_from_c_and_cpp_files(extensions):

Uh oh!

Conversation

lesteve commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Uh oh!

raghavrv commented Oct 21, 2016

Uh oh!

lesteve left a comment

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!

raghavrv commented Oct 21, 2016

Uh oh!

lesteve commented Oct 21, 2016

Uh oh!

raghavrv commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomDLT commented Oct 21, 2016

Uh oh!

lesteve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

amueller left a comment

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!

lesteve commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

jnothman commented Oct 25, 2016

Uh oh!

raghavrv commented Oct 25, 2016

Uh oh!

lesteve commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaelVaroquaux commented Oct 25, 2016

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lesteve commented Oct 21, 2016 •

edited

Loading

raghavrv commented Oct 21, 2016 •

edited

Loading

lesteve left a comment •

edited

Loading

lesteve Oct 21, 2016 •

edited

Loading

lesteve commented Oct 24, 2016 •

edited

Loading

lesteve commented Oct 25, 2016 •

edited

Loading