[MRG] Add Windows continuous integration with AppVeyor CI #3363

Merged
merged 2 commits into from Jul 14, 2014

Conversation

Projects
None yet
5 participants
Owner

ogrisel commented Jul 11, 2014

AppVeyor is a system similar to Travis CI that provides a Windows based build environment with MSVC++ Express 2008 and 2010 pre-installed along with the matching Windows SDKs for 64 bit builds. This PR configures how to build scikit-learn, generate .whl and .exe packages for 4 supported target platforms: Python 3.4 & 2.7, each with 32 and 64 bit.

This PR also has a workaround for #3255: some common tests that call fit_transform twice (with a fixed random state) do not deterministic output on 32 bit Python (both 2 and 3) for some estimators that use LAPACK routines like SVD or eigen solvers. Note the numpy / scipy libraries tested here come from Christoph Gohlke's distribution and are therefore linked with MKL. However I also had non-deterministic fit_transform for the same bunch of estimators when I manually built numpy and scipy with OpenBLAS on a Rackspace windows VM a while ago so the problem is not tied to the use of MKL. I therefore decided to catch AssertionError and raise SkipTest instead for this specific test_common check under 32 bit platforms.

Coverage Status

Coverage decreased (-0.01%) when pulling a62d661 on ogrisel:appveyor-ci into 178d0b6 on scikit-learn:master.

Owner

ogrisel commented Jul 11, 2014

I forgot to link to a working sample output of this build setup. Here is the report I get when I configured AppVeyor to watch my own fork of the sklearn github repo:

https://ci.appveyor.com/project/ogrisel/scikit-learn

Note that you can click on the artifact tab of each sub-entry of the build matrix to download the generated .whl and .exe packages.

Owner

ogrisel commented Jul 11, 2014

Once this is merge in master, I will reconfigure https://ci.appveyor.com/project/ogrisel/scikit-learn to point to https://github.com/scikit-learn/scikit-learn . Then existing PRs will have to be rebased on top of master to get properly tested under windows.

Ideally I would also like to backport that to 0.15.X to be able to use AppVeyor to build the binary release artifacts for Windows for scikit-learn 0.15.0.

Coverage Status

Coverage decreased (-0.01%) when pulling 35c3f74 on ogrisel:appveyor-ci into 1fe371e on scikit-learn:master.

jjhelmus referenced this pull request in scikit-image/scikit-image Jul 11, 2014

Closed

TST: Windows CI Testing using AppVeyor #1057

Coverage Status

Coverage decreased (-0.01%) when pulling 98c9f52 on ogrisel:appveyor-ci into e33b3b8 on scikit-learn:master.

@arjoly arjoly commented on an outdated diff Jul 12, 2014

sklearn/utils/testing.py
@@ -603,3 +604,18 @@ def check_skip_network():
with_network = with_setup(check_skip_network)
+
+
+def is_32bit():
@arjoly

arjoly Jul 12, 2014

Owner

I would add this to the __ALL__constant and add a small docstring.

@arjoly arjoly commented on an outdated diff Jul 12, 2014

sklearn/utils/testing.py
@@ -603,3 +604,18 @@ def check_skip_network():
with_network = with_setup(check_skip_network)
+
+
+def is_32bit():
+ return struct.calcsize("P") * 8 == 32
+
+
+def ignore_failure_32bit(test_callable):
@arjoly

arjoly Jul 12, 2014

Owner

Same here. I would add this to the __ALL__constant and add a small docstring.

@arjoly arjoly and 1 other commented on an outdated diff Jul 12, 2014

sklearn/tests/test_common.py
@@ -198,6 +199,7 @@ def test_transformers():
yield check_transformer, name, Transformer, X, y
+@ignore_failure_32bit
@arjoly

arjoly Jul 12, 2014

Owner

Should it be 32bit and windows?

@ogrisel

ogrisel Jul 12, 2014

Owner

Nobody uses 32 bit anymore in 2014 besides windows user. I don't know if those tests are unstable or not on non windows, 32 bit platforms.

@arjoly

arjoly Jul 12, 2014

Owner

Do you expect some failures?

@ogrisel

ogrisel Jul 12, 2014

Owner

It's likely but I don't know. Let me give it a try with a 32 bit vagrant box.

@ogrisel

ogrisel Jul 12, 2014

Owner

I ran the test_transformers test many times on ubuntu trusty i386 (the name of the 32-bit architecture for ubuntu) and I could never reproduce the instabilities of Python 32 bit builds under Windows. I will make that function more specific.

@arjoly arjoly and 2 others commented on an outdated diff Jul 12, 2014

continuous_integration/appveyor/install.ps1
@@ -0,0 +1,79 @@
+# Sample script to install Python and pip under Windows
+# Authors: Olivier Grisel and Kyle Kastner
+# License: CC0 1.0 Universal: http://creativecommons.org/publicdomain/zero/1.0/
@arjoly

arjoly Jul 12, 2014

Owner

I'm curious why not bsd?

@GaelVaroquaux

GaelVaroquaux Jul 12, 2014

Owner

I'm curious why not bsd?

Yes: in terms of audit for a project, it makes it much easier if all the
files are licensed under the same license. That way we can actually say:
"the project is BSD-license", with no small prints.

@ogrisel

ogrisel Jul 12, 2014

Owner

It's a copy and paste of the file that I wrote for python-appveyor-demo that I put under Public Domain. I will switch to BSD here.

@arjoly arjoly commented on an outdated diff Jul 12, 2014

+ PYTHON_VERSION: "2.7.8"
+ PYTHON_ARCH: "32"
+
+ - PYTHON: "C:\\Python27-64"
+ PYTHON_VERSION: "2.7.8"
+ PYTHON_ARCH: "64"
+
+ - PYTHON: "C:\\Python34"
+ PYTHON_VERSION: "3.4.1"
+ PYTHON_ARCH: "32"
+
+ - PYTHON: "C:\\Python34-64"
+ PYTHON_VERSION: "3.4.1"
+ PYTHON_ARCH: "64"
+
+# To avoid spurios builds and notification as long as not merged in master
@arjoly

arjoly Jul 12, 2014

Owner

spurious?

Owner

ogrisel commented Jul 13, 2014

@arjoly I addressed your comment and decided to disable the 32 bit windows skip to see exactly which estimators are failing. I just tried to run the tests on Python 3.4 32-bit on a Rackspace windows box and could not reproduce the failures any more in 10 consecutive runs. Let see if appveyor is failing or not.

Coverage Status

Coverage remained the same when pulling 157b997 on ogrisel:appveyor-ci into 851d914 on scikit-learn:master.

Coverage Status

Coverage remained the same when pulling 6ed1e38 on ogrisel:appveyor-ci into 851d914 on scikit-learn:master.

Owner

arjoly commented Jul 13, 2014

Hm, there is one failed build.

Coverage Status

Coverage remained the same when pulling ba7cb6a on ogrisel:appveyor-ci into fbfcdc5 on scikit-learn:master.

Owner

ogrisel commented Jul 13, 2014

Yes I am fixing a bunch of other heisen tests before tackling #3255.

Coverage Status

Coverage decreased (-0.0%) when pulling 6477ade on ogrisel:appveyor-ci into 514f073 on scikit-learn:master.

Owner

ogrisel commented Jul 14, 2014

I rebased on top of the current master where I pushed some fixes for other tests and rewrote this PR to only skip the fit_transform check for 3 specific transformers with Python 32 bit under Windows.

@GaelVaroquaux GaelVaroquaux commented on an outdated diff Jul 14, 2014

+ - PYTHON: "C:\\Python27-64"
+ PYTHON_VERSION: "2.7.8"
+ PYTHON_ARCH: "64"
+
+ - PYTHON: "C:\\Python34"
+ PYTHON_VERSION: "3.4.1"
+ PYTHON_ARCH: "32"
+
+ - PYTHON: "C:\\Python34-64"
+ PYTHON_VERSION: "3.4.1"
+ PYTHON_ARCH: "64"
+
+# To avoid spurious builds and notification as long as not merged in master
+branches:
+ only:
+ - appveyor-ci
@GaelVaroquaux

GaelVaroquaux Jul 14, 2014

Owner

How about we remove this for the merge!

@GaelVaroquaux GaelVaroquaux commented on an outdated diff Jul 14, 2014

sklearn/tests/test_common.py
@@ -240,6 +246,14 @@ def check_transformer(name, Transformer, X, y):
else:
assert_equal(X_pred.shape[0], n_samples)
@GaelVaroquaux

GaelVaroquaux Jul 14, 2014

Owner

How about you add a comment with "#FIXME" here. At some point, we'd like to fix these.

Owner

GaelVaroquaux commented Jul 14, 2014

👍 to merge with these 2 comments, if the tests pass.

Coverage Status

Coverage decreased (-0.0%) when pulling 1b296fd on ogrisel:appveyor-ci into d8a8a8d on scikit-learn:master.

Coverage Status

Coverage decreased (-0.0%) when pulling dbcc3ff on ogrisel:appveyor-ci into d8a8a8d on scikit-learn:master.

Coverage Status

Coverage decreased (-0.0%) when pulling 16dae39 on ogrisel:appveyor-ci into d8a8a8d on scikit-learn:master.

@vene vene commented on the diff Jul 14, 2014

continuous_integration/appveyor/requirements.txt
@@ -0,0 +1,6 @@
+--find-links http://28daf2247a33ed269873-7b1aad3fab3cc330e1fd9d109892382a.r6.cf2.rackcdn.com/index.html
@vene

vene Jul 14, 2014

Owner

What is this scary-looking url?

@ogrisel

ogrisel Jul 14, 2014

Owner

It's a temporary wheelhouse we host on the rackspace cloud account of the sklearn project to host numpy and scipy wheels built from Christoph Gohlke's installers. I will add an inline comment.

@vene

vene Jul 14, 2014

Owner

s/we ca delete/we can delete
Thanks for the clarification. The note about how they're obtained is definitely useful.

@vene vene commented on an outdated diff Jul 14, 2014

sklearn/tests/test_common.py
def check_transformer(name, Transformer, X, y):
+ if (name in ('CCA', 'LocallyLinearEmbedding', 'KernelPCA')
+ and _is_32bit_windows()):
+ # Those transformers yield non-deterministic output on Windows
+ # with a 32bit Python. The same transformerss are stable with 32bit
@vene

vene Jul 14, 2014

Owner

double s

Coverage Status

Changes Unknown when pulling 1481e53 on ogrisel:appveyor-ci into * on scikit-learn:master*.

Owner

ogrisel commented Jul 14, 2014

I rebased on master and it's all green (both appveyor and travis, although travis should not be affected).

Owner

GaelVaroquaux commented Jul 14, 2014

OK, merging this guy.

@GaelVaroquaux GaelVaroquaux added a commit that referenced this pull request Jul 14, 2014

@GaelVaroquaux GaelVaroquaux Merge pull request #3363 from ogrisel/appveyor-ci
[MRG] Add Windows continuous integration with AppVeyor CI
a9ad5a2

@GaelVaroquaux GaelVaroquaux merged commit a9ad5a2 into scikit-learn:master Jul 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment