ENH: enable OpenBLAS on windows. #10139

charris · 2017-11-30T18:40:52Z

Backport of #9645.

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-11-30T21:57:24Z

Note that this requires #9431 also, but that's a pretty big change. You might be better off using our distutils hack:

  # Replace numpy distutils with a version that can build with msvc + mingw-gfortran
  - ps: |
      $NumpyDir = $((python -c 'import os; import numpy; print(os.path.dirname(numpy.__file__))') | Out-String).Trim()
      rm -r -Force "$NumpyDir\distutils"
      $tmpdir = New-TemporaryFile | %{ rm $_; mkdir $_ }
      echo $env:NUMPY_HEAD
      echo $env:NUMPY_BRANCH
      git clone -q --depth=1 -b $env:NUMPY_BRANCH $env:NUMPY_HEAD $tmpdir
      mv $tmpdir\numpy\distutils $NumpyDir

njsmith · 2017-11-30T21:59:28Z

What's the motivation for backporting this to 1.13.x? Switching our wheels to OpenBLAS seems like it might be a risky change to do in a point release.

ghost · 2017-11-30T22:00:42Z

This is a significant performance improvement for large arrays. Whether that means anything relevant is not my say.

charris · 2017-11-30T22:12:38Z

@njsmith I canceled the backport as it isn't needed. However, note that 1.13.3 was built with OpenBLAS on windows, which was actually useful, as it got tested and a bug in the 32 bit version was uncovered. The bug also let us test the build version machinery of PIP, so it was a good all round learning experience :0

charris · 2017-11-30T23:36:23Z

@njsmith, oops, I thought we were in a different PR.

@xoviat How did we manage to do the 1.13.3 release without this or its dependency?

ghost · 2017-11-30T23:49:04Z

Wait, sorry. This configuration isn't needed to build with OpenBLAS if you standardize on the MSVC ABI:

               |
numpy          |    BLAS/LAPACK
               |
              MSVC
               ABI

The current build procedure (in master) standardizes on the MinGW ABI:

               |
numpy          |    BLAS/LAPACK
               |
              MinGW
               ABI

The reason for this is that this is the way that it was done in SciPy, because you currently must standarize on the MinGW ABI if you are using gfortran. But Numpy doens't have fortran sources.

ghost · 2017-11-30T23:53:26Z

I realize this is confusing and am I working on a solution to remove the second scenario entirely: #10047.

charris · 2017-11-30T23:56:47Z

@xoviat So if I do a 1.13.4 it should just work? After the license fix and check that the 32 bit OpenBLAS bug has been fixed.

ghost · 2017-12-01T00:00:44Z

Yes, but you should change the OPENBLAS_COMMIT to 5f998ef.

ghost · 2017-12-01T00:01:58Z

There are so many moving parts here that it's difficult to say for sure (ugh!). But it should work.

charris · 2017-12-01T19:05:44Z

@xoviat Numpy 1.13.x can not be built for 32 bit windows. I'm guessing something has changed in picking up the correct OpenBLAS module.

charris · 2017-12-01T19:31:30Z

That is with OPENBLAS_COMMIT: 5f998ef. I going to try against master (again), but I suspect that will still be a problem.

ghost · 2017-12-01T20:51:39Z

Ouch! The 64 bit wheels may not even be correct either because it looks like you picked up the mingwpy OpenBLAS, which has been shown to be broken. To resolve these issues, we need to do a few things: 1. Backport the changes from "make MSVC + mingw gfortran work" related to writing __config__.py. These should be about five lines of changes that should have no problem being accepted into a maintenance release. 2. Make absolutely sure that the OpenBLAS name does not contain "mingwpy." 3. Copy the OpenBLAS.dll file to the extra-dll directory. This can be done with one line in appveyor.yml.

ghost · 2017-12-01T20:56:14Z

The correct OpenBLAS file (for both 32 and 64 bit) is given here: https://ci.appveyor.com/project/scipy/scipy/build/1.0.1267/job/6t57x3f0v4gpdu1r#L62 https://3f23b170c54c2533c070-1c8a9b3114517dc5fe17b7c3f8c63a43.ssl.cf2.rackcdn.com/openblas-5f998ef_gcc7_1_0_win32.zip

ghost · 2017-12-01T21:18:47Z

Specifically, you need to backport these lines: https://github.com/numpy/numpy/pull/9431/files#diff-59c1de52780b698ea26e0cbbda5c951cR2284

charris · 2017-12-02T17:17:41Z

@xoviat Don't think so, as master doesn't build 32 bit window wheels using the new OpenBLAS libraries either. I think the script in windows-wheel-builder needs fixing, note that scipy bundles that script into appveyor.yml. Does the library ABI matter?

ghost · 2017-12-02T17:24:14Z

Master uses the same openblas as scipy. Please note that the mingwpy openblas is broken.

charris · 2017-12-02T23:02:13Z

Current status building NumPy wheels against master

openblas-5f998ef_gcc7_1_0_win32: Fails
openblas-5f998ef_gcc7_1_0_win64: Fails
openblas-5f998ef_win32: Fails
openblas-5f998ef_win64: OK
openblas-5f998ef_mingwpy_win32: OK, Cholesky test fails
openblas-5f998ef_mingwpy_win32: OK

@xoviat I assume that we need the gcc7_1_0 versions, but the NumPy wheels cannot be built against them with current master.

EDIT: Note that this PR was against maintenance/1.13.x and that the failures are against master, so backport doesn't apply.

EDIT2: the libraries are unpacked by https://github.com/numpy/windows-wheel-builder/unpack_openblas.bat

ghost · 2017-12-02T23:05:24Z

@charris Let me prepare a PR backporting the required fixes. It should take a few minutes.

charris · 2017-12-02T23:09:08Z

The failures are against master.

ghost · 2017-12-02T23:14:03Z

@charris The associated PR is #10152. After that is merged, I will submit a PR to numpy-wheels to fix the problem. The PR will not use the windows wheel builder.

charris · 2017-12-02T23:18:46Z

I assume the libraries are being unpacked into the wrong location. I'd rather fix that first and worry about the numpy-wheels repo later after implementing branch dependency.

ghost · 2017-12-02T23:20:37Z

Do you want to build master or maintenance/1.13x? I can do either but it's going to be different strategy for each.

Edit: building master is much simpler.

charris · 2017-12-02T23:24:07Z

Both, although I want to do master first and am considering dropping the 1.13.4 release. Because things will probably differ by branch, I'd rather wait to make major modifications till after implementing branches for numpy-wheels.

charris · 2017-12-02T23:25:32Z

Which is to say, I'd rather start by modifying windows-wheel-builder in a common way if possible.

ghost · 2017-12-02T23:35:36Z

MacPython/numpy-wheels#11 will build master with gcc_7_1_0, which is to say, zero issues.

TST: appveyor: Enable OpenBLAS via MinGW Gfortran

f5460f2

charris added 01 - Enhancement 08 - Backport Used to tag backport PRs component: build labels Nov 30, 2017

charris added this to the 1.13.4 release milestone Nov 30, 2017

charris closed this Nov 30, 2017

charris deleted the backport-9645 branch September 19, 2018 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: enable OpenBLAS on windows. #10139

ENH: enable OpenBLAS on windows. #10139

ENH: enable OpenBLAS on windows. #10139

ENH: enable OpenBLAS on windows. #10139

Conversation