Skip to content

MAINT Use cython language_level=3 directive #12873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Feb 28, 2019
Merged

MAINT Use cython language_level=3 directive #12873

merged 8 commits into from
Feb 28, 2019

Conversation

rth
Copy link
Member

@rth rth commented Dec 27, 2018

Closes #12796

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.

@rth rth mentioned this pull request Jan 2, 2019
Merged
@amueller
Copy link
Member

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

@rth
Copy link
Member Author

rth commented Jan 17, 2019

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

@amueller
Copy link
Member

ok, you'd know better than me ;)

amueller
amueller reviewed Jan 17, 2019
View reviewed changes
sklearn/datasets/_svmlight_format.pyx Outdated
@@ -4,6 +4,8 @@
# Lars Buitinck
# Olivier Grisel <olivier.grisel@ensta.org>
# License: BSD 3 clause
#
# cython: language_level=3str, boundscheck=False, wraparound=False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Copy link
Member Author

@rth rth Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@qinhanmin2014 qinhanmin2014 mentioned this pull request Feb 3, 2019
Merged
@amueller
Copy link
Member

what's the status? Wanna fix it up?

@rth
Copy link
Member Author

rth commented Feb 27, 2019

Yes, will try to finish this.

@rth rth changed the title WIP: MAINT Use cython language_level=3 directive MAINT Use cython language_level=3 directive Feb 27, 2019
@rth
Copy link
Member Author

rth commented Feb 27, 2019

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
ogrisel approved these changes Feb 28, 2019
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@massich
Copy link
Contributor

massich commented Feb 28, 2019

LGTM

@jeremiedbb
Copy link
Member

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
Copy link
Member Author

rth commented Feb 28, 2019

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
Copy link
Member

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

amueller
amueller approved these changes Feb 28, 2019
View reviewed changes
Copy link
Member

@amueller amueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jnothman jnothman merged commit 4957048 into scikit-learn:master Feb 28, 2019
@rth rth deleted the cython-language-level branch March 4, 2019 09:23
@rth rth mentioned this pull request Apr 13, 2019
Merged
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@rth
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
@rth @koenvandevelde
MAINT Use cython language_level=3 directive (scikit-learn#12873)
7d226f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@amueller amueller amueller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change cython language_level to 3
6 participants
@rth @amueller @massich @jeremiedbb @ogrisel @jnothman