Skip to content
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

Add mkl_path preference to support using "system" MKL #84

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

carstenbauer
Copy link
Member

@carstenbauer carstenbauer commented Jul 14, 2021

Addressing #82, this PR adds the option to use a "system" MKL, i.e. an external libmkl_rt, instead of the one provided by MKL_jll.jl.

Specifically, a user may specify a mkl_path via a preference, or the environment variable JULIA_MKL_PROVIDER. Supported are the values mkl_jll (default) and system. If system is chosen, we check that libmkl_rt is available on the linker search path, i.e. Libdl.find_library is able to find it, or otherwise throw an error. The user may also specify the path to libmkl_rt explicitly, either via the environment variable JULIA_MKL_PATH or the preference mkl_path. The function set_mkl_path can be used to change the MKL provider preference (it retriggers compilation in contrast to changing the environment variable, which won't have any effect).

Note that with this PR we only call using MKL_jll if it is really needed. Combined with the laziness of the MKL_jll artifact this ensures that the ~1.5 GB (on linux) MKL dependency is only downloaded if mkl_path = "mkl_jll" (default) but not when mkl_path = "system".

Credit: This PR is largely inspired by a draft implementation by @staticfloat which originated out of a Slack discussion.

TODOs (OLD):

Closes #82

@carstenbauer
Copy link
Member Author

Would be great if someone could approve CI for this to test 1.6 behavior (which hopefully shouldn't have been effected) etc.

src/MKL.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
@carstenbauer
Copy link
Member Author

I included the comments and, in particular, dropped support for Julia versions < 1.6. I also adjusted the CI accordingly.

@carstenbauer
Copy link
Member Author

Still need to check that 1.6 (no LBT) still works but maybe CI can try that for me?

staticfloat
staticfloat previously approved these changes Jul 20, 2021
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
deps/build.jl Outdated Show resolved Hide resolved
deps/build.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
@carstenbauer
Copy link
Member Author

The 1.6 CI runners seem to fail for different reasons...?

@staticfloat
Copy link
Member

It looks to me like it's failing because it can't find the MKL_Set_Threading_Layer symbol; I'm guessing that during precompilation, libmkl_rt doesn't point to where we think it should, and so it can't find that symbol within whatever library it's pointing at.

@jishnub
Copy link
Member

jishnub commented Aug 25, 2022

Gentle bump. Looks like julia v1.6 support is the holdup here. Since MKL.jl now requires Julia v1.8+, perhaps we may go ahead with this?

@jishnub
Copy link
Member

jishnub commented Feb 7, 2023

Gentle bump @carstenbauer

@carstenbauer
Copy link
Member Author

Alright, I've resurrected this PR. Should be close to the finish line now (it works fine for me locally). Let's see if CI fails (but it seems to be broken anyways 😞).

I've dropped the environment variable option as the preference option is generally superior. Let's see if we get complaints.

The only thing I'm not super sure about is if !(mkl_path == "mkl_jll" && !MKL_jll.is_available()) in __init__. If a user provides a path to a system MKL we (currently) don't know anything about it and can't set the interface layer etc. correctly. I guess we could also add preferences for the interface etc. but maybe the current option is just fine? (@staticfloat)

@carstenbauer carstenbauer changed the title Add mkl_provider option to support using "system" MKL Add mkl_path preference to support using "system" MKL Jul 4, 2023
src/MKL.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
src/MKL.jl Outdated Show resolved Hide resolved
@staticfloat
Copy link
Member

I guess we could also add preferences for the interface etc. but maybe the current option is just fine?

You don't need to add a preference for this, because LBT automatically figures out the interface through the magic of scary calling convention hacks.

@carstenbauer
Copy link
Member Author

Kind bump.

@carstenbauer
Copy link
Member Author

From a code POV, the PR can be merged. I don't know why CI is failing on Windows/macOS though. For Windows, there are lots of these errors

The `LinearAlgebra/qr` test set mutated ENV and did not restore the original values

which I don't think originate from this PR. For macOS, we have three numerical test errors. Again, I don't see how this could be related to the changes in this PR.

@ViralBShah
Copy link
Contributor

ViralBShah commented Feb 14, 2024

@carstenbauer Please rebase and merge if you think this is good. We have disabled macOS CI anyways, since it is now unsupported and agree it shouldn't hold it back.

Do you have commit access to this repo?

@ViralBShah ViralBShah marked this pull request as draft May 14, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use system Intel MKL
5 participants