Skip to content

CI [scipy-dev] now uses python 3.7 #11409

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

Merged
merged 5 commits into from
Jul 20, 2018
Merged

CI [scipy-dev] now uses python 3.7 #11409

merged 5 commits into from
Jul 20, 2018

Conversation

jnothman
Copy link
Member

@jnothman jnothman commented Jul 3, 2018

Fixing #11320...?

@rth
Copy link
Member

rth commented Jul 3, 2018

Maybe it could be better to add python 3.7 as another build to the Travis build matrix? Since scipy-dev is a cron, it's less systematically checked. I don't think build time would increase that much with another build as Travis appears to use multiple parallel workers...

@jnothman
Copy link
Member Author

jnothman commented Jul 3, 2018 via email

@rth
Copy link
Member

rth commented Jul 3, 2018

Travis doesn't yet have Python 3.7 natively available, as far as I
understand from the related SciPy issues.

Yes, but we could very well install Python 3.7 with conda then numpy & scipy with pip as you doing there. Essentially it could be just copying the scipy-dev under a separate section for Py3.7 and adding a new build entry to the matrix. Though the logic of distrib in ['conda', 'scipy-dev'] might need to be adapted, maybe distrib="conda-py37" ?

@jnothman
Copy link
Member Author

jnothman commented Jul 3, 2018 via email

@rth
Copy link
Member

rth commented Jul 3, 2018

Why? The scipy/numpy manylinux wheels with OpenBlas would be comparable in speed, aren't they? In my experience in other projects I was originally biased toward using conda, but it turns out that due to slow dependency resolution in conda + downloading/unpacking mkl which is huge, it's often faster to just use wheels from PyPi, if test runtime is comparable.

@jnothman
Copy link
Member Author

jnothman commented Jul 3, 2018

Well I'm not entirely certain why, but (here)[https://travis-ci.org/scikit-learn/scikit-learn/builds/399462496?utm_source=github_status&utm_medium=notification] it takes 10 more minutes to build for 3.7.

@rth
Copy link
Member

rth commented Jul 3, 2018

Running build_tools/travis/install.sh with Py3.7 takes very long (because it appears not to use the numpy py3.7 wheel and build it from sources), tests are actually faster.

  • for 3.6 with conda : install 289s, test 859s
  • for 3.7 with conda + pip: install 1174s, test 621s

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.

We would need to run the test Python 3.7 under windows on appveyor before claiming Python 3.7 support.

But I am fine merging this PR as it is (besides minor comment).

@@ -82,8 +87,8 @@ elif [[ "$DISTRIB" == "scipy-dev" ]]; then
# Set up our own virtualenv environment to avoid travis' numpy.
Copy link
Member

Choose a reason for hiding this comment

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

s/virtualenv/conda env/

Copy link
Member

@lesteve lesteve Jul 3, 2018

Choose a reason for hiding this comment

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

If we end up using conda to have Python 3.7 I think we can get rid of this comment ("Set up our own virtualenv ...") and the one below:

# This venv points to the python interpreter of the travis build
# matrix.

@ogrisel
Copy link
Member

ogrisel commented Jul 3, 2018

Actually, the test runtime is an issue but it should improve quickly as soon as the binary packages get uploaded.

@rth
Copy link
Member

rth commented Jul 3, 2018

Actually, the test runtime is an issue but it should improve quickly as soon as the binary packages get uploaded.

They are already uploaded for stable numpy and scipy . The reason this build takes so long ,I think, is because it uses scipy-dev (i.e. numpy dev wheels aren't available for Py3.7 yet from what I gather). So having one separate build for scipy-dev (cron) and one for Py3.7 (non cron) using stable numpy and scipy would be a way around it..

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.

I have not realized that Py3.7 is tested in Appveyor already. Merged master in and removed "virtualenv" related comments as suggested by @lesteve .

This LGTM, will merge when CI is green (unless someone has other comments).

# matrix.
virtualenv --python=python ~/testvenv
source ~/testvenv/bin/activate
make_conda python=3.7
Copy link
Member Author

Choose a reason for hiding this comment

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

I now wonder if we should just use python here without pinning the version.

Copy link
Member

Choose a reason for hiding this comment

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

Anticipating Python 4 ? )

I think it doesn't hurt to indicate explicitly for the reader that it tests 3.7 ..

@rth
Copy link
Member

rth commented Jul 20, 2018

(merged master to resolve merge conflict)

@rth
Copy link
Member

rth commented Jul 20, 2018

CI passed except for the PyPy3 build which is now temporary disabled.

Merging, thanks @jnothman !

@rth rth merged commit 1984dac into scikit-learn:master Jul 20, 2018
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.

4 participants