MAINT Use cython language_level=3 directive by rth · Pull Request #12873 · scikit-learn/scikit-learn

rth · 2018-12-27T14:58:48Z

Aims to switch to language_level=3 for the generated Cython files.

Also switched to a more concise e.g.

# cython: boundscheck=False

instead of,

@cython.boundscheck(False)  # for each function

in places where it could be applied globally.

Specified this on the file level because sklearn/datasets/_svmlight_format.pyx still assumes C strings (and so requires language_level="3str") which would have required more logic if this was set globally in sklearn/_build_utils/__init__.py:maybe_cythonize_extensions).

Marking this as WIP, as there is one remaining issue to fix where the build modules fail to import with e.g.,

sklearn/utils/tests/test_validation.py:29: in <module>
    from sklearn.utils.estimator_checks import NotAnArray
sklearn/utils/estimator_checks.py:49: in <module>
    from sklearn.feature_selection import SelectKBest
sklearn/feature_selection/__init__.py:25: in <module>
    from .mutual_info_ import mutual_info_regression, mutual_info_classif
sklearn/feature_selection/mutual_info_.py:11: in <module>
    from ..neighbors import NearestNeighbors
sklearn/neighbors/__init__.py:6: in <module>
    from .ball_tree import BallTree
sklearn/neighbors/binary_tree.pxi:568: in init sklearn.neighbors.ball_tree
    cdef class NeighborsHeap:
E   AttributeError: type object 'sklearn.neighbors.ball_tree.NeighborsHeap' has no attribute '__reduce_cython__'

possibly related to cython/cython#1953 . Though that issue should have been fixed with cython 0.28 while I used cython 0.29 in my tests.

amueller · 2019-01-17T22:09:45Z

Should we have an issue with a checklist for all the python2 cruft we still have?

rth · 2019-01-17T22:14:47Z

We could just to have a point of reference, but I don't think there is much left to do.

amueller · 2019-01-17T22:15:35Z

ok, you'd know better than me ;)

amueller · 2019-01-17T22:16:28Z

sklearn/datasets/_svmlight_format.pyx

 #          Olivier Grisel <olivier.grisel@ensta.org>
 # License: BSD 3 clause
+#
+# cython: language_level=3str, boundscheck=False, wraparound=False


if you mean "3str", no that's intentional,

The 3str option enables Python 3 semantics but does not change the str type and unprefixed string literals to unicode when the compiled code runs in Python 2.x

because that uses C strchr and this was failing otherwise. But yeah, ideally I should investigate how to work around that and make it work with language_level=3.

amueller · 2019-02-27T12:16:56Z

what's the status? Wanna fix it up?

rth · 2019-02-27T14:01:19Z

Yes, will try to finish this.

rth · 2019-02-27T21:44:38Z

Yay, after spending one hour, I finally found the absolute import that needed to be replaced by a relative one that was producing the obscure error message in #12873 (comment)

Should be ready to review now.

ogrisel

Very nice!

massich · 2019-02-28T08:35:14Z

LGTM

jeremiedbb · 2019-02-28T11:29:03Z

This is nice. Just a quick remark, when you have a .pxd and a .pyx you only need to put the cython directives in the .pxd file

rth · 2019-02-28T12:26:21Z

Just a quick remark, when you have a .pxd and a .pyx you only need to put the cython directives in the .pxd file

I agree both are likely redundant, but do you have a reference for that (that it's in that order and not say the opposite)? The cython documentation is not too explicit about it...

jeremiedbb · 2019-02-28T12:36:57Z

.pxd files are kind of equivalent to .h in C.
see https://cython.readthedocs.io/en/latest/src/tutorial/pxd_files.html

amueller

lgtm

…)" This reverts commit b2ee0c0.

rth added 2 commits December 27, 2018 16:51

Use cython: language_level=3 directive

455279e

Cython directives before module docstrings

fdee34a

rth mentioned this pull request Jan 2, 2019

MRG Drop legacy python / remove six dependencies #12639

Merged

amueller reviewed Jan 17, 2019

View reviewed changes

qinhanmin2014 mentioned this pull request Feb 3, 2019

MNT redundant from __future__ import #13079

Merged

rth added 3 commits February 27, 2019 17:10

Merge branch 'master' into cython-language-level

82d0c85

Fix merge issues

fe8e614

Fix relative import

b2c32f8

rth changed the title ~~WIP: MAINT Use cython language_level=3 directive~~ MAINT Use cython language_level=3 directive Feb 27, 2019

rth added 2 commits February 27, 2019 18:45

Fix integer division

a45ab1c

Fix sag_fast.pyx.tp

5db658e

ogrisel approved these changes Feb 28, 2019

View reviewed changes

amueller approved these changes Feb 28, 2019

View reviewed changes

Merge branch 'master' into cython-language-level

ba18813

jnothman merged commit 4957048 into scikit-learn:master Feb 28, 2019

rth deleted the cython-language-level branch March 4, 2019 09:23

rth mentioned this pull request Apr 13, 2019

[MRG] MNT Use a common language_level cython directive #13630

Merged

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

MAINT Use cython language_level=3 directive (scikit-learn#12873)

b2ee0c0

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "MAINT Use cython language_level=3 directive (scikit-learn#12873…

b50c4b7

…)" This reverts commit b2ee0c0.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "MAINT Use cython language_level=3 directive (scikit-learn#12873…

903ad9a

…)" This reverts commit b2ee0c0.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

MAINT Use cython language_level=3 directive (scikit-learn#12873)

7d226f2

Uh oh!

Conversation

rth commented Dec 27, 2018

Uh oh!

amueller commented Jan 17, 2019

Uh oh!

rth commented Jan 17, 2019

Uh oh!

amueller commented Jan 17, 2019

Uh oh!

amueller Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

rth Jan 17, 2019 • 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!

amueller commented Feb 27, 2019

Uh oh!

rth commented Feb 27, 2019

Uh oh!

rth commented Feb 27, 2019

Uh oh!

ogrisel 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!

massich commented Feb 28, 2019

Uh oh!

jeremiedbb commented Feb 28, 2019

Uh oh!

rth commented Feb 28, 2019

Uh oh!

jeremiedbb commented Feb 28, 2019

Uh oh!

amueller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rth Jan 17, 2019 •

edited

Loading

ogrisel left a comment •

edited

Loading