-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
@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.