ENH: enable OpenBLAS on windows. #9645

ghost · 2017-09-02T20:30:03Z

Previously, #9431 allowed distutils to make use of gfortran to create fortran wrappers that worked with MSVC. For a quick review:

Distutils detects objects that cannot be linked with an MSVC lib.
Distutils compiles unlinkable objects into a DLL
Distutils links Python extension against the DLL
Distutils writes __config__ that includes the DLLs in the PATH.

Here, we use distutils the link against the "unlinkable object" openblas.a. Distutils compiles openblas with gfortran as a DLL and then links against it. As a result of using OpenBLAS, performance restrictions on windows are addressed.

Because gfortran enables BLAS and fortran functionality, some new tests are run that were previously skipped. Some of these newly run tests fail. Such tests are marked as "known failures" on windows because they were previously untested on the CI, thus they are not regressions.

ghost · 2017-09-02T20:45:04Z

@charris Is pytest supported?

rgommers · 2017-09-02T20:54:27Z

appveyor.yml


-os: Visual Studio 2015


We still want to test MSVC as well, it needs to stay working. So instead of removing it all, I suggest to use a build matrix with a mix of the two build configs (without increasing the total number of builds).

Which MSVC are you concerned about? This configuration should test 2008, 2010, and 2015. The "os" has nothing to do with that.

Ralf - I'm guessing you mean a lapack-lite build, without OpenBLAS?

No, I didn't mean lapack-lite, that's okay to only test on Linux I think. The install and build_script part here explicitly add MINGW to the path and in the whole file there are only references to Cygwin and MinGW. So if MSVC is still being used, it's very much non-obvious (plus it's unclear what the purpose of the MinGW stuff is).

So either something is wrong (like MSVC being dropped) or this needs a lot of comments explaining what is going on.

MinGW is used to compile the OpenBLAS library. MSVC is used to compile everything else.

IIUC, all the compilers are installed in the appveyor VM regardless of this setting, and Python distutils generally finds MSVC based on registry entries rather than PATH.

If present, MSVC is used by default over mingw C compilers, even if the latter is on PATH.

That's my understanding too.

@pv @matthew-brett not sure if you're arguing against including more documentation or not. My comment is not particular to the line os: Visual Studio 2015. There's a lot of opaque stuff in this PR that simply needs explaining.

No, I'm just explaining why it works.

Yes, more documentation would be good.

I hate to be annoying, but I have previously done the same work in the windows-wheel-builder repo - here's a PR : numpy/windows-wheel-builder#3 in case it's helpful.

ghost · 2017-09-02T21:05:08Z

So we have File "C:\Python36\lib\site-packages\numpy\f2py\tests\util.py", line 149, in build_module responding "DLL load failed," probably due to skipping the configuration.

ghost · 2017-09-02T21:35:30Z

Would it be okay to xfail the three failing f2py tests since there is no way that they could have been run before?

charris · 2017-09-03T00:16:56Z

@xoviat We don't yet support pytest.

ghost · 2017-09-03T18:41:21Z

@charris Let me clarify the question. There are additional tests run on this pull request because numpy.distutils is more capable now after my previous gfortran PR with gfortran on the PATH.

Previously these tests were not run:

numpy.f2py.tests.test_block_docstring.TestBlockDocString.test_block_docstring ... SKIP: No C compiler available
numpy.f2py.tests.test_callback.TestF77Callback.test_string_callback ... SKIP: No C compiler available
numpy.f2py.tests.test_common.TestCommonBlock.test_common_block ... SKIP: No C compiler available

Now the are run but they fail, hence the red cross. So would it be acceptable to simply xfail these for now on windows?

charris · 2017-09-04T00:08:15Z

You can skip tests by adding dec to the imports from numpy.testing and use @dec.skipif(...). Whether you should skip the tests is the question. Is something missing that needs to be added?

ghost · 2017-09-04T01:41:55Z

The error that I receive is:

ImportError: DLL load failed: The specified module could not be found.

The tests apparently invoke the fortran compiler which generates a DLL that must be on the path for the generated module to work correctly. Normally this isn't a problem because the __config__ module automatically extends the path, but I suspect that the __config__ module generated here is thrown away.

And then you have a couple Python 2.7 failures, because well, it's Python 2.7.

ghost · 2017-09-04T04:29:29Z

Hrm...the tests that I marked should be skipped are still run.

pv · 2017-09-04T20:30:24Z

Nose runs setup() regardless of whether the test is marked skipped or not. You'd probably need to add new setup() methods that raise SkipTest if to be skipped, and otherwise call super().setup. 4.9.2017 6.29 "xoviat" <notifications@github.com> kirjoitti:

…

Hrm...the tests that I marked should be skipped are still run. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9645 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5gZ8Pw8xbYBcuMSb7gkMQOd89B-Jks5se3ypgaJpZM4PLANo> .

pv · 2017-09-04T20:41:24Z

Make that KnownFailure actually -- this stuff is supposed to work also on windows. The issue probably is that due to the extra-dll stuff, plain modules without __config__.py just don't work with the msvc+gfortran hack. 4.9.2017 6.29 "xoviat" <notifications@github.com> kirjoitti:

…

Hrm...the tests that I marked should be skipped are still run. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9645 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5gZ8Pw8xbYBcuMSb7gkMQOd89B-Jks5se3ypgaJpZM4PLANo> .

1. fail tests related to DLL load failure as they were previously untested. 2. fix have_compiler to return false on old compilers 3. xfail some tests that were not working on old Python versions.

ghost · 2017-09-05T04:51:07Z

I've updated the description; the PR should be ready for review.

pv

The overall build config is the same as for Scipy, so I think everything is fine from implementation POV.

There are some outdated comments, and the non-f2py xfails could use extra comment.

pv · 2017-09-09T12:35:12Z

appveyor.yml

+
+  # Install the BLAS library 
+  #  - install "openblas.lib" to PYTHON\lib
+  #  - install OpenBLAS.dll to MINGW\bin


These comments are outdated (only openblas.a is installed)

pv · 2017-09-09T12:36:05Z

appveyor.yml

+      PYTHON_ARCH: 64
+      TEST_MODE: full
+
+    - PYTHON: C:\Python27


I wonder if the tag builds are actually needed for Numpy. The production wheels are built by a different CI config, this one is used only for integration testing.

I don't think they're needed but they could be used to compare differences between the released wheels and the wheels used for testing. A major advantage of this appveyor configuration is that you can actually download the built wheels and inspect them yourself.

pv · 2017-09-09T12:38:49Z

numpy/ma/tests/test_core.py

@@ -3605,6 +3607,8 @@ def test_varstd(self):
            assert_almost_equal(np.sqrt(mXvar0[k]),
                                mX[:, k].compressed().std())

+    @dec.knownfailureif(sys.platform=='win32' and sys.version_info < (3, 6),
+                        msg='Fails on Python < 3.6')


Do we actually understand why this fails only on Windows?
If yes, best to add that to the msg= so we don't need to wonder about it.

No, but this appears to have not been tested on the CI before. It's probably a new discovery.

So I opened a new issue for this and updated the description with the issue number.

Thanks. I now remember these also appeared on Scipy, it's probably some issue in old MSVC CRTs.

pv · 2017-09-09T12:39:27Z

numpy/testing/tests/test_utils.py

@@ -263,6 +263,8 @@ def test_complex(self):
        self._assert_func(x, x)
        self._test_not_equal(x, y)

+    @dec.knownfailureif(sys.version_info < (3, 0),
+                        msg="Error message different on Python 2")


Why wasn't this issue noticed before?

Or maybe it's only on win32 (in which case there should be a platform check in the knownfailureif).

From log: https://ci.appveyor.com/project/charris/numpy/build/1.0.6743/job/b9ro4kpqo4h29ljf

AssertionError: '\nArrays are not equal\n\n(shapes (2L,), (1L, 2L) mismatch)\n x: array([1, 2])\n y: [repr failed for <matrix>: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()]' != '\nArrays are not equal\n\n(shapes (2,), (1, 2) mismatch)\n x: array([1, 2])\n y: [repr failed for <matrix>: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()]'

Probably would be simple to fix to accept also a second variant
msg2 = msg.replace("shapes (2,), (1, 2)", "shapes (2L,), (1L, 2L)")

Good catch.

@pv To clarify, should we replace the message only on Python2 win32?

I'd maybe accept either one, assert_(err == msg or err == msg2, err).
iirc the Ls are due to the integer sizes on win32-32 so they could also appear on other plats.

ghost · 2017-09-09T14:58:27Z

numpy/distutils/tests/test_system_info.py

@@ -68,7 +68,7 @@ def have_compiler():
        try:
            if not compiler.initialized:
                compiler.initialize()  # MSVC is different
-        except DistutilsError:
+        except (DistutilsError, ValueError):


BTW this is because MSVC 2008 is a crappy compiler.

ghost · 2017-09-09T15:00:36Z

numpy/f2py/tests/util.py

@@ -319,6 +319,7 @@ class F2PyTest(object):
    module = None
    module_name = None

+    @dec.knownfailureif(sys.platform=='win32', msg='Fails with MinGW64 Gfortran')


Where would the best place be to put the comments? There are multiple tests all with the same cause.

Maybe open an issue and just put the issue number.

Allow the error message to contain "large" arrays.

pv · 2017-09-09T15:10:55Z

numpy/testing/tests/test_utils.py

-                str(e),
-                "\nArrays are not equal\n\n"
+            msg = str(e)
+            msg2 = msg.replace("shapes (2,), (1, 2)", "shapes (2L,), (1L, 2L)")


Arguments the other way around.
Also, the try: except: isn't necessary if you do replacement in str(e).

I was not thinking here; I copied the replace statement directly from the comment without looking at it myself.

use the correct replace() order

ghost · 2017-09-09T15:25:49Z

@rgommers Do you have any other questions about this?

pv · 2017-09-09T15:29:58Z

numpy/testing/tests/test_utils.py

+            try:
+                self.assertEqual(msg, msg_reference)
+            except AssertionError:
+                self.assertEqual(msg2, msg_reference)


The try: except is not needed. You can make it simpler via msg = msg.replace(...)

rgommers

Thanks @xoviat, clearer already. I have put some comments for all places that are still hard to understand.

rgommers · 2017-09-10T04:23:03Z

appveyor.yml

+      }
+
+install:
+  - C:\cygwin\bin\du -hs "%LOCALAPPDATA%\pip\Cache"


Need a comment above this line, I think something like

This `install` step only builds and installs OpenBLAS plus build/test dependencies. SciPy itself is built with `build_script`. Cygwin is only needed for WHAT?

Cygwin is probably only needed for the unix tools (du, find etc) used for cleaning up the pip cache.

Cygwin has nothing to do with the build. This comes from experience dealing with appveyor caches.

rgommers · 2017-09-10T04:26:07Z

appveyor.yml

+  - C:\cygwin\bin\du -hs "%LOCALAPPDATA%\pip\Cache"
+
+on_finish:
+  - ps: |


Add comment, uploads what to where? Not needed for all Appveyor scripts, so good to know why here

rgommers · 2017-09-10T04:28:02Z

appveyor.yml

+      mkdir dist
+      pip wheel -v -v -v --wheel-dir=dist .
+
+      ls dist -r | Foreach-Object {


would be good to have a comment here, not sure what PushArtifact does and why there's a Foreach-Object (there should be only one wheel I think, built with pip wheel -v -v -v --wheel-dir=dist . above?)

rgommers · 2017-09-10T04:32:36Z

appveyor.yml

+      } Else {
+          $OPENBLAS = $env:OPENBLAS_64
+      }
+      $clnt = new-object System.Net.WebClient


A comment here or above ps would be useful. I think it's "downloads an already built OpenBLAS library from the path given by $OPENBLAS". That build was done from repo XXX.

I actually don't know the repo but matthew is in charge of these builds.

It's here: https://github.com/matthew-brett/build-openblas

I updated the comment above the ps block.

rgommers · 2017-09-10T19:30:30Z

Thanks @xoviat, all LGTM now

matthew-brett · 2017-09-30T23:15:23Z

I think this one's ready to merge?

charris · 2017-09-30T23:59:05Z

Let's give it a shot. The folks who know more about this than I do have signed off.

charris · 2017-10-01T00:05:12Z

Thanks @xoviat .

rgommers reviewed Sep 2, 2017

View reviewed changes

rgommers added 01 - Enhancement component: build labels Sep 2, 2017

charris changed the title ~~appveyor: enable OpenBLAS on windows.~~ ENH: enable OpenBLAS on windows. Sep 3, 2017

xoviat added 2 commits September 4, 2017 23:32

TST: appveyor: Enable OpenBLAS via MinGW Gfortran

37878bf

TST: fix failures:

917652a

1. fail tests related to DLL load failure as they were previously untested. 2. fix have_compiler to return false on old compilers 3. xfail some tests that were not working on old Python versions.

pv reviewed Sep 9, 2017

View reviewed changes

xoviat added 2 commits September 9, 2017 09:36

appveyor: update documentation

67a175b

tests: core: update failure message

06b5318

ghost commented Sep 9, 2017

View reviewed changes

TST: utils: fix test_error_msg

0957a1f

Allow the error message to contain "large" arrays.

pv reviewed Sep 9, 2017

View reviewed changes

xoviat added 6 commits September 9, 2017 10:16

TST: util: update documentation

bcc5d1d

TST: common: update documentation

41407a2

TST: block docstring: update comment

e149d29

TST: callback: update comment

167ca98

TST: util: fix test_error_message

84fdfb3

use the correct replace() order

🎨

66029c7

ghost closed this Sep 9, 2017

ghost reopened this Sep 9, 2017

pv reviewed Sep 9, 2017

View reviewed changes

pv approved these changes Sep 9, 2017

View reviewed changes

TST: utils: fix string continuation

79c476e

rgommers requested changes Sep 10, 2017

View reviewed changes

appveyor: update documentation

448bb82

rgommers approved these changes Sep 10, 2017

View reviewed changes

pv mentioned this pull request Sep 30, 2017

Wrong result from np.linalg.cholesky on 32-bit win32 in numpy 1.13.3 #9795

Closed

charris merged commit d05fd30 into numpy:master Sep 30, 2017

ghost deleted the appveyor branch October 1, 2017 00:07

ghost mentioned this pull request Oct 1, 2017

appveyor: build with openblas MacPython/numpy-wheels#3

Closed

lesteve mentioned this pull request Nov 10, 2017

Using OpenBLAS for the released wheels scikit-learn/scikit-learn#10105

Closed

charris mentioned this pull request Nov 30, 2017

ENH: enable OpenBLAS on windows. #10139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: enable OpenBLAS on windows. #9645

ENH: enable OpenBLAS on windows. #9645

ENH: enable OpenBLAS on windows. #9645

ENH: enable OpenBLAS on windows. #9645

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment