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.

Sign up for GitHub

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

Check and use only available FFTW libraries #278

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Check and use only available FFTW libraries #278

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
453aa11
Check which FFTW libraries are available
devmotion Sep 28, 2023
9a3850b
Do not unconditionally load MKL_jll but match platforms
devmotion Sep 28, 2023
9db530c
Apply suggestions
devmotion Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "FFTW"
uuid = "7a1cc6ca-52ef-59f5-83cd-3a7055c09341"
version = "1.7.1"
version = "1.7.2"

[deps]
AbstractFFTs = "621f4979-c628-5d54-868e-fcf4e3e8185c"
Expand Down
45 changes: 40 additions & 5 deletions src/providers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,42 @@
# Hardcoded list of supported platforms
# In principle, we could check FFTW_jll.is_available() and MKL_jll.is_available()
# but then we would have to load MKL_jll which we want to avoid (lazy artifacts!)
const platforms_providers = Dict(
Base.BinaryPlatforms.Platform("aarch64", "macos") => ("fftw",),
Base.BinaryPlatforms.Platform("aarch64", "linux"; libc = "glibc") => ("fftw",),
Base.BinaryPlatforms.Platform("aarch64", "linux"; libc = "musl") => ("fftw",),
Base.BinaryPlatforms.Platform("armv6l", "linux"; libc = "glibc", call_abi = "eabihf") => ("fftw",),
Base.BinaryPlatforms.Platform("armv6l", "linux"; libc = "musl", call_abi = "eabihf") => ("fftw",),
Base.BinaryPlatforms.Platform("armv7l", "linux"; libc = "glibc", call_abi = "eabihf") => ("fftw",),
Base.BinaryPlatforms.Platform("armv7l", "linux"; libc = "musl", call_abi = "eabihf") => ("fftw",),
Base.BinaryPlatforms.Platform("i686", "linux"; libc = "glibc") => ("fftw", "mkl"),
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"),
Base.BinaryPlatforms.Platform("x86_64", "linux"; libc = "glibc") => ("fftw",),
Base.BinaryPlatforms.Platform("x86_64", "linux"; libc = "musl") => ("fftw",),
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())
Copy link
Member

Choose a reason for hiding this comment

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

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.

if valid_fftw_providers === nothing
error("no valid FFTW library available")
end

function get_provider()
# Note: we CANNOT do something like have the `default` value be `get(ENV, "JULIA_FFTW_PROVIDER", "fftw")` here.
# This is because the only way the Julia knows that a default has changed is if the values on-disk change; so
# if your "default" value can be changed from the outside, you quickly run into cache invalidation issues.
# So the default here _must_ be a constant.
default_provider = "fftw"
default_provider = first(valid_fftw_providers)

# Load the preference
provider = @load_preference("provider", default_provider)

# Ensure the provider matches one of the ones we support
if provider ∉ ("fftw", "mkl")
@error("Invalid provider setting \"$(provider)\"; valid settings include [\"fftw\", \"mkl\"], defaulting to \"fftw\"")
if provider ∉ valid_fftw_providers
@error("Invalid provider setting \"$(provider)\"; valid settings include [$(join(map(x -> '"' * x * '"', valid_fftw_providers), ", "))]")
provider = default_provider
end
return provider
Expand All @@ -34,8 +59,8 @@ Also supports `Preferences` sentinel values `nothing` and `missing`; see the doc
`Preferences.set_preferences!()` for more information on what these values mean.
"""
function set_provider!(provider; export_prefs::Bool = false)
if provider !== nothing && provider !== missing && provider ∉ ("fftw", "mkl")
throw(ArgumentError("Invalid provider value '$(provider)'"))
if provider !== nothing && provider !== missing && provider ∉ valid_fftw_providers
throw(ArgumentError("Invalid provider value \"$(provider)\"; valid settings include [$(join(map(x -> '"' * x * '"', valid_fftw_providers), ", "))]"))
end
set_preferences!(@__MODULE__, "provider" => provider; export_prefs, force = true)
if provider != fftw_provider
Expand All @@ -48,6 +73,11 @@ end
# If we're using fftw_jll, load it in
@static if fftw_provider == "fftw"
import FFTW_jll
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")
Copy link
Member

Choose a reason for hiding this comment

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

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.

end
libfftw3[] = FFTW_jll.libfftw3_path
libfftw3f[] = FFTW_jll.libfftw3f_path

Expand Down Expand Up @@ -83,6 +113,11 @@ end
# If we're using MKL, load it in and set library paths appropriately.
@static if fftw_provider == "mkl"
import MKL_jll
Copy link
Member

@giordano giordano Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 👍)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if !MKL_jll.is_available()
# more descriptive error message if MKL is not available
# (should not be possible to reach this)
error("MKL library cannot be loaded: please switch to the `fftw` provider for FFTW.jl")
end
libfftw3[] = MKL_jll.libmkl_rt_path
libfftw3f[] = MKL_jll.libmkl_rt_path
const _last_num_threads = Ref(Cint(1))
Expand Down