Set use_blas64 to false to fix MKL.jl interaction numpy problem #35
Conversation
|
Would be good to link to the other PR. |
|
This builds now with the new version. I don't see any reason for not merging this. |
|
What are the side effects of UPDATE: Oh, I asked this before. Anyways, I guess this 32-bit "limitation" should be mentioned in the README if this gets merged, no? |
|
is it easy to include a flag in the build for the individual user to choose? |
src/install.jl
Outdated
|
|
||
| @assert libblas_idx !== nothing && liblapack_idx !== nothing | ||
|
|
||
| lines[libblas_idx] = "const libblas_name = $(repr(name))" | ||
| lines[liblapack_idx] = "const liblapack_name = $(repr(name))" | ||
| if useblas64_idx != nothing | ||
| lines[useblas64_idx] = "const USE_BLAS64 = false" |
There was a problem hiding this comment.
This will set it to use 32 bit integers even when reverting to normal sysimage? Anyway, I can fix it up.
There was a problem hiding this comment.
Bump. @KristofferC Not sure I understand this - but it would be nice to get this PR merged.
It's probably fine to simply make it the default. But I think it's worth mentioning in the README.md (or somewhere else?). |
|
Some time ago I did some comparison between BLAS 64 and 32, and the performance gain of BLAS 64 was noticeable. I think BLAS 32 should not be the default behavior, but only be enabled by an option, |
It's a bit odd. Can you share the benchmarks? |
|
Even if there is a performance issue, we probably still need to ship 32-bit as the default, given the number of crash complaints we receive. We should file the performance issue with Intel. |
|
What is holding this PR from being merged? |
ViralBShah
changed the title
|
@stevengj If we merge this - I assume it will solve all the issues we are having on the python side. |
|
Probably. |
|
@ViralBShah I created an issue to remind myself to do some benchmarks If switching to 64 bit version be a matter of a special flag or option, definitely that is easy enough for everyone (like me) who does not use Python to use the 64bit version. |
|
I have added the option to build MKL.jl with the 64bit version of MKL by setting |
|
I guess once this is merged the comment in the readme: "frequently encountered compatibility issues" is already deprecated... |
|
Is there anything left to be done here? I'd be happy to see this move on and make MKL with Julia more end-user friendly. I'm still avoiding MKL because of Arpack.jl but I'm hoping if MKL.jl has less struggles steps will be taken there also. |
|
We need @andreasnoack to look at this - whenever he has a moment. |
|
@KristofferC Do you understand this well enough to know if it will work right? It would be nice to use MKL as 32-bit by default so as to not cause trouble with python. |
|
Failing on nightly. |
|
@KristofferC Is the failure on nightly known? |
|
https://travis-ci.org/github/JuliaComputing/MKL.jl/jobs/735062657#L262 |
|
I suppose it's an issue with PackageCompiler on 1.6? I'll merge, as tests are passing on 1.5. |
|
How will this work with MKLSparse? Will we lose the ability to do Int64 sparse matmuls? I'm not really sure how this interacts with the sparse MKL part. |
@fgerick Would you be able to test this? |
|
It seems MKL at compile time uses So I guess that means it would be restricted to |
|
Yes, presumably. I think that is also the behaviour we need by default, otherwise it leads to all those crashes in matplotlib. |
|
Yeah, all I'm saying is that right now, if someone installs this it most likely effectively turns off MKLSparse since everyone is using the default |
|
Oh I didn't quite understand that. Good to know. Is there a way to opt into the Int64 version? |
same as the other pr, updated to latest master.