Move MKL to an extension on 1.9+

[palday]

I've marked this as a breaking change because it requires explicitly loading MKL_jll (or MKL) to enable the support on 1.9+.

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 49.20%. Comparing base (b2cbb65) to head (e24752b).

Files	Patch %	Lines
ext/FFTWMKLExt.jl	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #299       +/-   ##
===========================================
- Coverage   73.08%   49.20%   -23.89%     
===========================================
  Files           5        6        +1     
  Lines         535      500       -35     
===========================================
- Hits          391      246      -145     
- Misses        144      254      +110     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stevengj]

Why is this a breaking change? At worst, someone currently using the MKL backend might use the FFTW backend instead, which could change the performance but not the API.

[giordano]

I can't have a look right now but I have a couple of questions:

• you can't control whether other packages in the environment load MKL or not, does it mean other packages may force you to use MKL as backend for FFTW.jj even if you didn't want to?
• what happens on platforms where MKL isn't available? Does loading the no-op MKL wreak FFTW.jl?

[palday]

Why is this a breaking change? At worst, someone currently using the MKL backend might use the FFTW backend instead, which could change the performance but not the API.

@stevengj The current logic (which I just copied over) doesn't fallback to the FFTW backend when MKL isn't available and the preference is set to MKL. Since MKL now has to be explicitly loaded to be available, this seemed like a breaking behavior to me. I can also change it to be a minor version bump.

you can't control whether other packages in the environment load MKL or not, does it mean other packages may force you to use MKL as backend for FFTW.jj even if you didn't want to?

@giordano No, the choice of backend is a user-set preference.

(Also, I view packages loading MKL as a form of piracy, at a much lower level than most people think of!)

what happens on platforms where MKL isn't available? Does loading the no-op MKL wreak FFTW.jl?

No, again because it still depends on the user preference. If MKL is loaded but the user preference is (the default) FFTW, then the package extension is also essentially a no-op (see the @static guards).

[palday]

@stevengj Maybe the solution is to unconditionally set things to the FFTW backend in the package and only conditionally update for the mkl preference in the extension. Then there things still work even when MKL isn't loaded.

[palday]

@stevengj given the amount of @static branches, it's surprisingly hard to set up the configuration to always fall back to FFTW if mkl is set as the backend but MKL isn't loaded. I could probably do it, but it is going to be a substantial overhaul and quite tricky to avoid overwriting methods. I'm not sure it's worthwhile. I know that FFTW is deep in the dependency stack for a lot of packages, but I suspect that it will be mostly a matter of "just" accepting a CompatHelper PR for the majority of developers.

Let me know what you think before I start working on a potential overhaul!

[palday]

FWIW I suspect Setting the backend to mkl in the current release on a platform without MKL support (e.g. Apple silicon) would result in breakage. So I guess one could argue that the current changes aren't breaking enough to warrant to warrant a breaking release.

[stevengj]

What is the advantage of making this an extension over the current approach? If it's not optional, it doesn't seem like it falls within the spirit of package extensions.

Is the main advantage just to save the storage space of downloading MKL_jll if it's not being used?

[palday]

Storage space, download times (which matters more for CI) and unnecessary compat restrictions.

[giordano]

Setting the backend to mkl in the current release on a platform without MKL support (e.g. Apple silicon) would result in breakage.

But hopefully one doesn't purposefully do something which would just not work. Instead, you can't control at all whether the MKL_jll package just happens to be loaded (and it'd be No-op on platforms where MKL isn't available). To me the latter looks much more troublesome, as it's outside of user's control.

[giordano]

Also, how do you control what's the FFT backend if loading another package toggle that? This mechanism looks very problematic to me.

[palday]

Also, how do you control what's the FFT backend if loading another package toggle that? This mechanism looks very problematic to me.

@giordano Again, the user controls which backend to use via their preferences. The extension merely enables an additional backend as an option. If the user has not set that option, then it is not used. On the other hand, if a user wants to use a different backend, I think it's fair that they are required to load that backend, i.e. opt in fully. They still control whether or not to set the MKL preference and whether or not to load MKL. With this PR, they can load MKL without using it as a backend if they just want MKL for BLAS, but they can't use MKL as the backend without loading it. The user is in more control, not less.

[stevengj]

What if the user has set the mkl' preference for their own usage, in which case they know to import MKL_jll before importing FFTW, but then they use some package X that (unknown to the user) also imports FFTW. Then, when the user does using X, it will break, because X didn't know to import MKL_jll?

This doesn't seem very composable.

[palday]

What if the user has set the mkl' preference for their own usage, in which case they know to import MKL_jll before importing FFTW, but then they use some package X that (unknown to the user) also imports FFTW. Then, when the user does using X, it will break, because X didn't know to import MKL_jll?

This doesn't seem very composable.

That's a very good point for global preferences. For project specific preferences, it seems reasonable for the user to only set MKL as the backend if they load it. But I don't know what to do about the global preference thing and that might be the best argument against this change.