Skip to content

Enabling different linking options for MKL downstream. #2841

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Narsil
Copy link
Collaborator

@Narsil Narsil commented Mar 28, 2025

When using candle/mkl right now, it forces the use of static linking.

However, this is not optimal in some circumstances, for instance we need to hotpatch the library to enable faster runtime on non-Intel CPU: https://github.com/huggingface/text-embeddings-inference/blob/main/Dockerfile#L39-L40

In order for the thing to be practical it's easier to use dynamic linking and use clever ordering to force patch the library at runtime.

I made the change non breaking.

  • feature mkl becomes _mkl and doesn't include the actual intel-mkl-src/mkl-static-lp64-iomp feature, so it's agnostic the linking procedure.
  • Adding a new mkl feature which solely adds mkl-static-lp64-iomp to keep backward compatibility in examples and downstream crates
  • Propagate to candle-nn
  • Didn't propagate to candle-transformers (user can simply opt-out of mkl altogether since no code actually uses it)

So now for power-users than want to custom link mkl all they need to do is enable the _mkl feature, and manually enable the intel-mkl-src/mkl-dynamic-lp64-iomp in their own crate/binary.

I haven't modified the documentation since it's a power user feature anyway, but I can do it if deemed necessary.

@@ -33,7 +33,8 @@ criterion = { workspace = true }
default = []
accelerate = ["dep:accelerate-src", "candle/accelerate"]
cuda = ["candle/cuda"]
mkl = ["dep:intel-mkl-src", "candle/mkl"]
_mkl = ["dep:intel-mkl-src", "candle/_mkl"]
mkl = ["candle/mkl"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this different from the previous behavior where when mkl is enabled we would have a proper dependency to intel-mkl-src here (with the "mkl-static-lp64-iomp" feature)?

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.

2 participants