Skip to content
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.

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

Remove python < 3.5 from CI #12746

Merged
merged 32 commits into from
Dec 14, 2018

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Dec 9, 2018

Fixes #11989
Fixes #12184
Removing python < 3.5 from the CI.

Ubuntu trusty, however has python 3.4 but its support ends on April 2019, which I guess would be our next release?

Should we then focus on what's available on 16.04 instead, and move our min supported versions for other packages as well?

@adrinjalali
Copy link
Member Author

I'm not sure why travis fails on DISTIB="ubuntu". The setup seems to fail to find atlas (not sure if it's related), and fails with

cc1: some warnings being treated as errors
error: Command "/usr/lib/ccache/gcc -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes 
-g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time 
-D_FORTIFY_SOURCE=2 -fPIC -I/usr/lib/python3/dist-packages/numpy/core/include 
-I/usr/lib/python3/dist-packages/numpy/core/include -I/usr/include/python3.5m 
-I/home/travis/build/scikit-learn/scikit-learn/testvenv/include/python3.5m 
-I/usr/include/python3.5m -I/home/travis/build/scikit-learn/scikit-learn/testvenv/
include/python3.5m -c sklearn/neighbors/quad_tree.c -o build/temp.linux-x86_64-3.5/
sklearn/neighbors/quad_tree.o" failed with exit status 1

I must be missing something trivial I guess...

@qinhanmin2014
Copy link
Member

For appveyor, please refer to #11582.
I don't think we need this before 0.20.2.

@adrinjalali
Copy link
Member Author

I agree, but I guess 0.20.2 is gonna be in a few days?

Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

So, from now sklearn does not support python 2?

@jnothman
Copy link
Member

So, from now sklearn does not support python 2?

From 0.21, which master is tracking the development of.

@jnothman
Copy link
Member

Travis is unhappy

@adrinjalali
Copy link
Member Author

@jnothman , yeah I know, I was hoping one of you would have an easy answer to the error I'm getting there. Otherwise I'd have to launch a VM and check the error on the same environment. Right now I don't have an easy answer to the issue.

@jnothman
Copy link
Member

Have you tried increasing the cython version, e.g. latest, if only just to test?

@adrinjalali
Copy link
Member Author

Adding joblib to the ubuntu test, and using the latest cython, make travis happy. But codecov complains since the parts of the code specific to old packages are not used anymore. For instance, in preprocessing/data.py, it complains about:

if LooseVersion(np.__version__) < '1.9':
    references = references.tolist()

There are a few questions:

  • Is moving the min-supported versions to Ubuntu Xenial fine (it's the oldest Ubuntu supporting Python 3.5, and from April it will be the oldest supported Ubuntu).
  • What should be the minimum Cython version?
  • Should this PR handle removal of code pieces such as the one mentioned above?
  • Should this PR handle changes in other places such as setup.py? (changing constants such as SCIPY_MIN_VERSION for instace).

Anything else I'm missing?

@ogrisel
Copy link
Member

ogrisel commented Dec 10, 2018

Is moving the min-supported versions to Ubuntu Xenial fine (it's the oldest Ubuntu supporting Python 3.5, and from April it will be the oldest supported Ubuntu).

+1

What should be the minimum Cython version?

Would the version from xenial be recent enough?

Should this PR handle removal of code pieces such as the one mentioned above?

+1

Should this PR handle changes in other places such as setup.py? (changing constants such as SCIPY_MIN_VERSION for instance).

Yes and the documentation should also be updated to reflect what are the minimum version supported. In particular the windows builds instructions can be simplified.

@adrinjalali
Copy link
Member Author

Would the version from xenial be recent enough?

Unfortunately the build fails with even a minor version newer than what's provided in Xenial. I can kinda brute force and find the minimum cython which works with Xenial, but I'd personally rather use a new (or new-ish) cython, since we can expect developers and maintainers to have a recent cython (I suppose).

@ogrisel
Copy link
Member

ogrisel commented Dec 10, 2018

I can kinda brute force and find the minimum cython which works with Xenial, but I'd personally rather use a new (or new-ish) cython, since we can expect developers and maintainers to have a recent cython (I suppose).

Still it would be good to approximately know what is the oldest version of Cython that works for scikit-learn. Maybe try 2 manual iterations of bisect to have rough idea and use that for this PR.

@jeremiedbb
Copy link
Member

cython <= 0.27 won't work with python 3.7 for sure

@adrinjalali
Copy link
Member Author

@jeremiedbb thanks, good to know 👍

@qinhanmin2014
Copy link
Member

Maybe we need another job to take care of min dependency in Circle CI?
See #12184

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

The 0.20.X branch lives its own life independently of master at this point. We can drop CI support for Python 2.7 and 3.4 in master while keeping such CI configuration in the 0.20.X branch for the future 0.20.2, 0.20.3...

I was assuming 0.20.2 would be very soon in which case keeping py2 for a bit longer would simplify backports. But you are right, I suppose we cannot delay it indefinitely. LGTM as well.

@@ -1,13 +1,18 @@
version: 2

jobs:
python3:
python3-min-dependency-versions:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this, doc-min-dependencies as it shows in CI at each PR and the current name is a bit long. Everything is python3 now so putting it in the title no longer brings useful information..

Copy link
Member Author

Choose a reason for hiding this comment

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

So, doc-min-dependencies and doc?

@ogrisel
Copy link
Member

ogrisel commented Dec 14, 2018

For some reason the LGTM C++ code analysis fails by trying to import sklearn without building it first. This happens when apt is installing the python-typing package:

[2018-12-13 21:57:39] [build] Selecting previously unselected package python-typing. [2018-12-13 21:57:39] [build] (Reading database ... 68239 files and directories currently installed.) [2018-12-13 21:57:39] [build] Preparing to unpack .../python-typing_3.6.2-1_all.deb ... [2018-12-13 21:57:39] [build] Unpacking python-typing (3.6.2-1) ... [2018-12-13 21:57:40] [build] Setting up python-typing (3.6.2-1) ... [2018-12-13 21:57:41] [build] deptrace-server: Package installer exited with code 0 [2018-12-13 21:57:41] [build] Traceback (most recent call last): [2018-12-13 21:57:41] [build] File "setup.py", line 36, in <module> [2018-12-13 21:57:41] [build] import sklearn [2018-12-13 21:57:41] [build] File "/opt/src/sklearn/__init__.py", line 63, in <module> [2018-12-13 21:57:41] [build] from . import __check_build [2018-12-13 21:57:41] [build] File "/opt/src/sklearn/__check_build/__init__.py", line 46, in <module> [2018-12-13 21:57:41] [build] raise_build_error(e) [2018-12-13 21:57:41] [build] File "/opt/src/sklearn/__check_build/__init__.py", line 41, in raise_build_error [2018-12-13 21:57:41] [build] %s""" % (e, local_dir, ''.join(dir_content).strip(), msg)) [2018-12-13 21:57:41] [build] ImportError: No module named _check_build [2018-12-13 21:57:41] [build] ___________________________________________________________________________ [2018-12-13 21:57:41] [build] Contents of /opt/src/sklearn/__check_build: [2018-12-13 21:57:41] [build] setup.py __init__.py __init__.pyc [2018-12-13 21:57:41] [build] _check_build.pyx [2018-12-13 21:57:41] [build] ___________________________________________________________________________ [2018-12-13 21:57:41] [build] It seems that scikit-learn has not been built correctly.

@adrinjalali
Copy link
Member Author

That's really odd, I only changed circle-ci job names in the last commit!

@rth
Copy link
Member

rth commented Dec 14, 2018

I wouldn't worry too much about LGTM.com it tends to fail at the analysis step sometimes...

@ogrisel
Copy link
Member

ogrisel commented Dec 14, 2018

Ok let's merge before we get conflicts then.

@ogrisel ogrisel merged commit 2bd87f6 into scikit-learn:master Dec 14, 2018
@ogrisel
Copy link
Member

ogrisel commented Dec 14, 2018

@adrinjalali would you mind doing a new PR to update at least the README and maybe the build / installation doc to remove references to now unsupported python / numpy / scipy versions?

@adrinjalali
Copy link
Member Author

Will do my best (I'm still discovering new places with related documentation that I didn't know they're there).

@ogrisel
Copy link
Member

ogrisel commented Dec 14, 2018

The following yields some false positives but not too many:

git grep "\b2\.7\b" | grep -v ".csv" | grep -v "externals"

@ogrisel
Copy link
Member

ogrisel commented Dec 14, 2018

$ git grep "\b2\.7\b" | grep -v ".csv" | grep -v "externals"
README.rst:.. |Python27| image:: https://img.shields.io/badge/python-2.7-blue.svg
README.rst:- Python (>= 2.7 or >= 3.4)
build_tools/windows/windows_testing_downloader.ps1:    # Function returns a dictionary of packages to download for Python 2.7.
build_tools/windows/windows_testing_downloader.ps1:        "python" = "https://www.python.org/ftp/python/2.7.7/python-2.7.7.msi"
build_tools/windows/windows_testing_downloader.ps1:        "2.7" = Python27URLs
doc/conf.py:    mathjax_path = ('https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.0/'
doc/developers/contributing.rst:All scikit-learn code should work unchanged in both Python 2.7 and 3.4 or
doc/developers/contributing.rst:to code and it certainly requires testing on both 2.7 and 3.4 or newer.
doc/developers/performance.rst:     185        48          130      2.7      0.0              break
doc/index.rst:                    <li><strong>Scikit-learn 0.21 will drop support for Python 2.7 and Python 3.4.</strong>
doc/install.rst:- Python (>= 2.7 or >= 3.4),
doc/install.rst:    Scikit-learn 0.20 is the last version to support Python 2.7 and Python 3.4.
doc/whats_new/v0.18.rst:    Later versions of scikit-learn will require Python 2.7 or above.
doc/whats_new/v0.18.rst:    Later versions of scikit-learn will require Python 2.7 or above.
doc/whats_new/v0.20.rst:  :class:`neighbors.KDTree` and :class:`neighbors.BallTree` in Python 2.7 to
doc/whats_new/v0.20.rst:    Version 0.20 is the last version of scikit-learn to support Python 2.7 and Python 3.4.
examples/bicluster/plot_bicluster_newsgroups.py:    Like Counter.most_common in Python >=2.7.
examples/cluster/plot_feature_agglomeration_vs_univariate_selection.py:plt.figure(figsize=(7.3, 2.7))
examples/svm/plot_svm_kernels.py:          (0, -2.7),
sklearn/utils/testing.py:    Copied from Python 2.7.5 and modified as required.

@adrinjalali
Copy link
Member Author

@ogrisel awesome, thanks :)

@qinhanmin2014
Copy link
Member

Hmm, Circle CI isn't running on master?

@adrinjalali adrinjalali deleted the maint/remove-python2-ci branch December 14, 2018 13:11
@qinhanmin2014
Copy link
Member

Honestly I'd argue that this might make 0.20.2 much difficult to release. Since we've merged this, maybe we should release 0.20.2 ASAP.

adrinjalali added a commit to adrinjalali/scikit-learn that referenced this pull request Jan 7, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants