Check and use only available FFTW libraries #278

devmotion · 2023-09-28T12:43:34Z

I think it would be good to check if FFTW_jll and MKL_jll are actually available before trying to load them, as suggested in #274 (comment). I ran into this issue with a project that specified provider = "mkl" in a LocalPreferences.toml - precompilation will error on my MacBook since MKL is not available for Apple silicon. With this PR, FFTW will use FFTW_jll instead (if it's not available either an error is thrown) but present the user with the error message:

┌ FFTW [7a1cc6ca-52ef-59f5-83cd-3a7055c09341]
│  ┌ Error: Invalid provider setting "mkl"; valid settings include ["fftw"]
│  └ @ FFTW ~/.julia/dev/FFTW/src/providers.jl:23
└

Fixes #274.

src/FFTW.jl

codecov · 2023-09-28T12:46:20Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ef8fc5b) 73.08% compared to head (9db530c) 73.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   73.08%   73.15%   +0.07%     
==========================================
  Files           5        5              
  Lines         535      529       -6     
==========================================
- Hits          391      387       -4     
+ Misses        144      142       -2

Files	Coverage Δ
src/providers.jl	`66.66% <60.00%> (+7.40%)`	⬆️

... and 2 files with indirect coverage changes

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

giordano · 2023-09-28T12:48:35Z

src/providers.jl

@@ -82,7 +90,6 @@ end

 # If we're using MKL, load it in and set library paths appropriately.
 @static if fftw_provider == "mkl"
- import MKL_jll


Why not doing the test is_available here? People are not going to be happy to having to unconditionally download MKL, that was the whole point of #133 (which you seemed to agree with, judging by the 👍)

I was not aware of this issue. Mainly, I thought it would be more user-friendly to figure out the supported libraries in advance, before trying to set them. Only moving the check here won't help with my use case and the linked issue above.

Is there any other way to check whether a library can be loaded apart from ..._jll.is_available()? Maybe checking the system architecture is an OKish workaround?

Is there any other way to check whether a library can be loaded apart from ..._jll.is_available()?

No.

Maybe checking the system architecture is an OKish workaround?

That's going to hard-code answers which may potentially change over time.

Only moving the check here won't help with [...] the linked issue above.

Why not? Capture the fact that the artifact isn't available and throw a helpful error message to explain what to do, instead of getting a cryptic UndefVarError that some users don't know what to do with.

Also, if I understand this implementation correctly this is automagically changing the provider if the requested one isn't actually available, which doesn't sound very deterministic.

Yeah, switching to the supported default provider was the main point - changing the LocalPreferences.toml file manually on my end creates git conflicts when moving branches and pulling all the time (and if I forget to adjust it, compilation will fail). To some extent that's already done in the current code: If you specify an invalid provider other than fftw and mkl, FFTW will just display an @error and switch to the default provider (which is fixed to fftw currently). So basically I'm in the same situation, on my computer mkl is also an invalid provider so I would like to switch to the default one. And I guess you could hypothetically think about an analogous scenario where fftw is invalid and you would like to switch to the mkl as default provider.

Displaying a more descriptive error message is an improvement but I think a solution that just defines the (in)valid providers correctly would be much more convenient.

That's going to hard-code answers which may potentially change over time.

Yes, a bit annoying. But since I didn't see any other alternative to define the valid providers correctly, I switched to hardcoding the supported platforms.

I guess the supported platforms could always be updated easily - and probably supported platforms for MKL won't change much at least? If we're more ambitious, probably we could even automatically extract it from the Artifacts.toml files in FFTW_jll and MKL_jll.

I guess the supported platforms could always be updated easily - and probably supported platforms for MKL won't change much at least?

It did change.

giordano · 2023-10-04T17:57:09Z

src/providers.jl

+ Base.BinaryPlatforms.Platform("x86_64", "freebsd") => ("fftw",),
+ Base.BinaryPlatforms.Platform("x86_64", "windows") => ("fftw", "mkl"),
+)
+const valid_fftw_providers = Base.BinaryPlatforms.select_platform(platforms_providers, Base.BinaryPlatforms.HostPlatform())


If you do something like

const valid_fftw_providers = let platforms_providers = Dict(...) Base.BinaryPlatforms.select_platform(platforms_providers, Base.BinaryPlatforms.HostPlatform()) end

you avoid storing in the compile cache a useless dictionary.

giordano · 2023-10-04T17:57:56Z

src/providers.jl

+ if !FFTW_jll.is_available()
+ # more descriptive error message if FFTW is not available
+ # (should not be possible to reach this)
+ error("FFTW library cannot be loaded: please switch to the `mkl` provider for FFTW.jl")


Maybe provide a more helpful error message to explain how to to do this? Most users won't have a clue of what this is about.

giordano · 2023-12-05T21:47:31Z

src/providers.jl

+ Base.BinaryPlatforms.Platform("i686", "linux"; libc = "musl") => ("fftw",),
+ Base.BinaryPlatforms.Platform("i686", "windows") => ("fftw", "mkl"),
+ Base.BinaryPlatforms.Platform("powerpc64le", "linux"; libc = "glibc") => ("fftw",),
+ Base.BinaryPlatforms.Platform("x86_64", "macos") => ("fftw", "mkl"),


#280 shows that the hardcoded list a bad idea: there's no MKL 2024 build for this platform.

Yeah, I always thought that a hardcoded list is suboptimal. Ideally we would just parse the Artifacts.toml file, I think, similar to https://github.com/JuliaPackaging/JLLWrappers.jl/blob/a1025a86bc58ac40e2d1df30f4d6bf6c390d51e1/src/toplevel_generators.jl#L132-L185 - but in the best case such a functionality would be available somewhere else. I would imagine that this would be a more common use case - figuring out whether a user's platform is supported without actually loading the JLL package.

But why not removing the automagic fallback, and only do what the user asks, and loudly and clearly fail if the artifact isn't available? In that case you can use is_available safely, since the user explicitly requested it

Because the motivation for the PR and the problem I was trying to solve was that it failed. As a user I don't want to run into an error, I just want to use the provider that is available on my platform.

See e.g. also https://julialang.slack.com/archives/C6A044SQH/p1701979919655409

To clarify a bit more: In my case the MKL provider was chosen as the default for me by a package I was using, but in principle it could also be the other way around (that the currently hardcoded default FFTW_jll provider does not work but MKL_jll does). I think it would be less surprising and more user-friendly if the default provider would not be hard-coded to e.g. FFTW_jll but that it is checked which providers are actually available for the user and the default one is chosen as the first one among those. I don't think one should override explicit user settings but I think the default should work (and the error messages/list of valid providers should match the ones that are actually available).

In my case the MKL provider was chosen as the default for me by a package I was using

I'm not sure packages should do that, preferences should be up to users. Especially if packages get it wrong.

I agree. But I want to emphasize that the PR tries to address the problem of incorrect lists of valid providers and non-working default providers in FFTW.jl itself, and does aim (anymore) to fix incorrect settings in some downstream package.

Check which FFTW libraries are available

453aa11

giordano reviewed Sep 28, 2023

View reviewed changes

src/FFTW.jl Outdated Show resolved Hide resolved

giordano reviewed Sep 28, 2023

View reviewed changes

Do not unconditionally load MKL_jll but match platforms

9a3850b

devmotion force-pushed the dw/valid_fftw_providers branch from 4bc920c to 9a3850b Compare September 28, 2023 14:33

giordano reviewed Oct 4, 2023

View reviewed changes

Apply suggestions

9db530c

giordano reviewed Dec 5, 2023

View reviewed changes

giordano mentioned this pull request Dec 12, 2023

mkl build error on Intel Mac #287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check and use only available FFTW libraries #278

Check and use only available FFTW libraries #278

devmotion commented Sep 28, 2023

codecov bot commented Sep 28, 2023 •

edited

Loading

giordano Sep 28, 2023 •

edited

Loading

devmotion Sep 28, 2023

devmotion Sep 28, 2023

giordano Sep 28, 2023

giordano Sep 28, 2023

giordano Sep 28, 2023

devmotion Sep 28, 2023

devmotion Sep 28, 2023

devmotion Sep 28, 2023

giordano Dec 5, 2023

giordano Oct 4, 2023

giordano Oct 4, 2023

giordano Dec 5, 2023

devmotion Dec 7, 2023

giordano Dec 7, 2023

devmotion Dec 7, 2023

devmotion Dec 7, 2023

devmotion Dec 7, 2023

giordano Dec 7, 2023 •

edited

Loading

devmotion Dec 7, 2023

Check and use only available FFTW libraries #278

Are you sure you want to change the base?

Check and use only available FFTW libraries #278

Conversation

devmotion commented Sep 28, 2023

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

giordano Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giordano Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2023 •

edited

Loading

giordano Sep 28, 2023 •

edited

Loading

giordano Dec 7, 2023 •

edited

Loading