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

Require Julia 1.6+, use Preferences, eliminate mutable state #189

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

staticfloat
Copy link
Collaborator

This adds support for Preferences.jl as the backing store for setting
a computation backend (currently only supports fftw and mkl as valid
choices). The convenience method set_provider!() is given, but it is
little more than a wrapper around Preferences.set_preferences!().
Users can alter their LocalPreferences.toml or Project.toml files by
hand as well; in the event that an illegal value is provided, an error
will be displayed and fftw will be chosen as the default value.

Preferences is a relatively new subsystem in Julia and it has some
lacking support in Pkg; so for now we are adding a separate
test/Project.toml, which will have preferences added into it on CI, as
there is a bug in propagating preferences when a single Project.toml
is used with extra dependencies for the tests.

@staticfloat
Copy link
Collaborator Author

This closes #188

X-ref: JuliaLang/Pkg.jl#2500

@staticfloat staticfloat linked an issue Apr 9, 2021 that may be closed by this pull request
@stevengj
Copy link
Member

stevengj commented Apr 9, 2021

LGTM, assuming CI is okay…

@staticfloat staticfloat force-pushed the sf/preferences branch 5 times, most recently from 3e2eebb to 3534101 Compare April 9, 2021 20:47
This adds support for `Preferences.jl` as the backing store for setting
a computation backend (currently only supports `fftw` and `mkl` as valid
choices).  The convenience method `set_provider!()` is given, but it is
little more than a wrapper around `Preferences.set_preferences!()`.
Users can alter their `LocalPreferences.toml` or `Project.toml` files by
hand as well; in the event that an illegal value is provided, an error
will be displayed and `fftw` will be chosen as the default value.

Preferences is a relatively new subsystem in Julia and it has some
lacking support in `Pkg`; so for now we are adding a separate
`test/Project.toml`, which will have preferences added into it on CI, as
there is a bug in propagating preferences when a single `Project.toml`
is used with extra dependencies for the tests.
@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #189 (2972104) into master (41e3cda) will increase coverage by 1.30%.
The diff coverage is 56.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   65.04%   66.35%   +1.30%     
==========================================
  Files           4        5       +1     
  Lines         412      428      +16     
==========================================
+ Hits          268      284      +16     
  Misses        144      144              
Impacted Files Coverage Δ
src/fft.jl 63.44% <ø> (+0.38%) ⬆️
src/providers.jl 53.84% <53.84%> (ø)
src/FFTW.jl 87.50% <75.00%> (-4.17%) ⬇️
src/precompile.jl 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07eeabb...2972104. Read the comment docs.

@staticfloat staticfloat merged commit 2fe1568 into master Apr 9, 2021
@giordano giordano deleted the sf/preferences branch April 9, 2021 23:11
@giordano
Copy link
Member

giordano commented Apr 9, 2021

@staticfloat perhaps it'd also be useful to have some documentation about how to set the preference 🙂 For example in https://juliamath.github.io/FFTW.jl/stable/#Installation

Comment on lines +76 to +80
@static if fftw_provider == "mkl"
using IntelOpenMP_jll, MKL_jll
const libfftw3 = MKL_jll.libmkl_rt_path
const libfftw3f = libfftw3
end
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem compatible with PackageCompiler. The paths will end up being compiled into the system image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, this should be done within __init__() if we need this to work with PackageCompiler.

Copy link
Member

Choose a reason for hiding this comment

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

Generally we ned all packages to work with PackageCompiler and specifically we very much need this to work with PackageCompiler

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.

Eliminate build step, use Preferences
4 participants