MAINT: Increase minimum required LAPACK version to 3.3.1 #6051
Conversation
@@ master #6051 diff @@
======================================
Files 238 238
Stmts 43908 43908
Branches 8233 8233
Methods 0 0
======================================
Hit 34336 34336
Partial 2604 2604
Missed 6968 6968
|
|
What does OSX Accelerate provide? |
|
Hmm. It appears that, as of OSX 10.10, it still only provides 3.2.1. Since that was the last LAPACK version to support CLAPACK, I don't know if they have any plans to update. |
|
The only option for Accelerate would be to ship and build LAPACK 3.3.1 against the BLAS in Accelerate. It is basically just a bunch of Fortran files we can build as a library. No special build process is needed. It will incur a small performance hit though. (Albeit not much worse than what we get for ATLAS and OpenBLAS.) |
|
Yep. In general, I see three ways forward here:
None of these options are at all good. Dropping support for Accelerate seems like the "least bad" option to me personally, but others are very likely to disagree. |
|
Accelerate makes it very easy to build SciPy on OSX. Dropping it seems like a bad idea, at least I would object. If I have to mess with ATLAS again, chances are I would not bother to contribute. It would also set back the performance of free SciPy builds tremendously. The part of Accelerate we really need is BLAS. If we do a real house cleaning, we could build something like libVecFort, which would overshadow most of BLAS and (in our case) all of LAPACK, and we could even do away with the ABI wrappers. |
|
My suggestion would then be a forth option, not on your list. |
|
|
It would be a shame to make it difficult to compile scipy with Accelerate. First - that is by far the easiest way to get BLAS / LAPACK on Mac. Second - some people want Accelerate specifically - see : http://vtk.1045678.n5.nabble.com/VTK-and-Python-on-OS-X-El-Capitan-tp5734265p5734280.html |
|
Dropping support for Accelerate would be quite painful, doesn't seem worth it. Here's another suggestion:
@sturlamolden this seems a bit different from what you suggest, although I don't completely understand your libVecFort idea. |
|
@rgommers Your auggestion would work too, but it might require a BLAS with gfortran ABI (depending on which BLAS function the added routines will call). Your suggestion certainly means shipping less code than mine. What I suggested is when Accelerate is present:
This requires us to use a feature in OSX called DLL overshadowing. This is what we use to correct SGEMV and what libVecFort use to create a BLAS with gfortran ABI on top of Accelerate. I would also like to point out that libVecFort already does most of this. So if their license allows it we could just use their code. |
|
@rgommers If we are going to use your suggestion, we have to be careful to compile the missing parts of LAPACK with g77 ABI, and then make sure they are exposed to SciPy with gfortran ABI. |
|
|
I think we can fairly easily bundle the missing routines, for use with Accelerate. |
|
Of the 3 ways to use vecLibFort, the static linking one seems most appropriate. It's not that much code, so could be a decent option. |
|
I'm not sure if we need vecLibFort --- afaics, it is similar to what we currently do, with the difference that it shadows the symbols rather than renaming them. Namely, remember that we have patched our fortran source codes with replaced names for the functions. Since MKL also appears to need g77 abi wrappers, if we switch to shadowing (and revert the replaced names in the fortran sources), we need to verify that the way we shadow works on all supported platforms, not only osx. |
|
However, since the deprecated/new lapack routines afaics are just subroutines, they are abi compatible as-is. So afaics, the discussion about abi wrappers is not directly relevant for including the missing lapack routines. (Provided we make the corresponding symbol renames in the lapack sources.) |
|
Ok, but we need to compile the missing routines with -ff2c in case they happen to call functions like SDOT in Accelerate. |
|
Sorry, we can of course patch the lapack sources. |
|
Yes, we are currently using the patching approach everywhere, so we'd need to do that also to the new LAPACK sources. |
|
For ARPACK, the necessary patching incantation is here: https://github.com/scipy/scipy/blob/master/scipy/sparse/linalg/eigen/arpack/ARPACK/README.scipy#L14 |
|
I was not aware that we are currently patching all our Fortran sources. Let's just continue with that, although it is more tedious in the long run. |
|
Thanks for reviving this. I'll go ahead and start updating it to include the missing routines. |
generating script to no longer exclude functions not available with a uniform interface before version 3.3.1.
BLAS/LAPACK API. The line endings aren't handled automatically by GIT since the files are currently flagged as binary. Most compilers can handle both types of line endings, so it's not a huge issue. This makes sure that the autogeneration script can also handle both types of line endings.
wrappers for Cython BLAS/LAPACK API. This is needed for some of the routines included in LAPACK releases 3.3.1 and up.
specifiers for different arguments used by the subroutine wrappers for Fortran functions (as opposed to subroutines) that are included in the Cython BLAS/LAPACK API. These special cases allow the wrappers to be generated for LAPACK version 3.3.1 as opposed to LAPACK 3.1.0.
signature generating script for Cython BLAS/LAPACK API.
|
Please consider LAPACK <= 3.4.0 as long as building Scipy for Python 2.7 on Windows with Visual Studio 2008, Intel Fortran compilers, and Intel MKL is supported. |
|
Thanks @cgohlke, I didn't think about that! 3.4.0 it is then, for a couple more years. |
That's a common argument made when discussing decisions like this, and it's hard to prove or disprove. @GaelVaroquaux lectures us regularly on users in labs he's worked in that are stuck on clusters with old Linux distros though, so they do exist:) Anyway, moot argument now that @cgohlke pointed out the MKL + Python 2.7 problem I think. We can't not support that combo, so can only go past LAPACK 3.4.0 once we drop Python 2.7. |
I'm not saying that people stuck in legacy machines don't exist. I'm just saying that they won't be able to upgrade in any case. Hence my argument. @cgohlke Does it help to use CMAKE system introduced in 3.4.0 for your wheels? Because 3.4.1 and 3.4.2 are bug fix releases. You can find additional details here |
|
IIUC, the build system of LAPACK is irrelevant for MKL --- Intel ships
prebuilt binaries which bundle some fixed LAPACK version.
|
|
Yah, as I understand it the issue with newer versions of MKL is having to use a different C standard library than the one linked to the Python 2.7 binaries. It's probably possible to get the older versions of the Intel compilers to build against a newer MKL and then rely on the fact that BLAS/LAPACK don't let resources cross their interface boundaries, but I'm not sure how much of a pain that'd be to set up. |
Still part of a commonly heard argument, and not really the case. It's fairly straightforward to compile SciPy on Linux, and that doesn't need admin rights. |
|
@rgommers If we build LAPACK on top of Accelerate (i.e. overshadow) we are left with that one bug which is also present in ATLAS. |
|
Personally I am in favor of dropping support for anything but MKL and OpenBLAS. |
Well there must be a reason for it to be common. I don't see the point of trying to be up-to-date on a legacy system. Either you are stuck or you update your system. Out of our own clusters in Eindhoven we are not even allowed to adjust the time let alone building something. |
|
I've added this in the 1.0.0 release notes: Will now move this issue to the 1.1.0 milestone. |
|
This PR needs an update, but can now be merged in principle. Could be extended as well, to LAPACK 3.4.0 (not higher, due to MKL + py27, see comments higher up). We're going to branch 1.1.x in the near future, can be either before or after that. @insertinterestingnamehere are you still interested in updating this PR? |
|
Definitely still interested. I'll see if I can get the ball rolling on this again. |
|
Great! |
|
@insertinterestingnamehere If you need an extra hand for wrapping LAPACK routines let me know please :) |
|
@ilayn the wrappers should be autogenerated once I update the script. That's not so bad. I could really use help getting the ci tests updated for osx and removing the accelerate specific code though. I can do a lot of that, but I don't have an osx machine to test with locally so it'll be a slow process waiting for builds to finish. I'm planning on improving the wrapper updates here in a separate PR, but things like removing the osx ci fixes could probably be done separately anyway. |
|
@insertinterestingnamehere - I am sure we can find you an OSX machine to log into, if that would help? Let me know via email? |
|
@matthew-brett sent. Do you have a current download link for OpenBLAS on OSX as well? |
|
@insertinterestingnamehere - I'm just asking round now, but we'll find you a machine soon. Latest OpenBLAS at https://3f23b170c54c2533c070-1c8a9b3114517dc5fe17b7c3f8c63a43.ssl.cf2.rackcdn.com/openblas-0.2.20-345-g6a6ffaff-Darwin-x86_64-1becaaa.tar.gz |
|
Awesome, thanks for the link. I'll try pointing the SciPy OSX CI tests there. |
|
Okay, I've posted an updated version of the work here in #8670. Changes for OSX are still in progress, but it's a start. |
This needs some discussion on the mailing list.
It increases the minimum required LAPACK version to 3.3.1 (it's currently 3.1.0). The version of LAPACK available by default on Travis CI is 3.3.1, so I don't expect this to cause trouble with the CI systems. Since 3.3.1 isn't quite as old as 3.1.0, I still need to mention the requirement in various places in the docs. Other than some extra documentation, this should all be working now though.
There are a few advantages. One is that explicitly requiring more recent versions lets us avoid errors like #5039. Another is that this lets us provide a more complete Cython LAPACK API. The functions exported to Cython currently include only the routines common to every LAPACK version from 3.1.0 to 3.6.0. Increasing the minimum requirement lets us export another 169 routines through that interface. In particular, this adds: