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

Use an environment variable to request MKL #44

Merged
merged 2 commits into from
Oct 3, 2017
Merged

Use an environment variable to request MKL #44

merged 2 commits into from
Oct 3, 2017

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Oct 1, 2017

No description provided.

deps/build.jl Outdated
# If BLAS was compiled with MKL and the user wants MKL-based FFTs, we'll oblige.
# In that case, we have to do this little dance to get around having to use BinDeps
# for a library that's already linked to Julia.
if get(ENV, "JULIA_FFTW_PROVIDER") == "MKL" && Base.BLAS.vendor() === :mkl
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this is that if you do a Pkg.update() it will "forget" your previous setting unless the environment variable is still set. That's why in PyCall I save the setting in a file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like the kind of thing that could go in a .juliarc.jl file

Copy link
Member

Choose a reason for hiding this comment

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

Requiring users to not only set an env var, but to set it "permanently" in a file somewhere, seems awfully error prone.

Besides, environment variables live in a global namespace, e.g. they "leak" into Julia subprocesses, so I greatly prefer setups where you only need to set them temporarily.

@ararslan
Copy link
Member Author

ararslan commented Oct 1, 2017

Okay, now it stores the setting after the first build.

deps/build.jl Outdated
else
provider = ""
end
if provider == "MKL" && Base.BLAS.vendor() === :mkl
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be an error if provider == "MKL" and MKL is not available?

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 that's probably better

deps/build.jl Outdated
if isfile(settings)
provider = readchomp(settings)
action = "delete " * normpath(settings)
elseif haskey(ENV, "JULIA_FFTW_PROVIDER")
Copy link
Member

Choose a reason for hiding this comment

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

These if statements seem to be in the wrong order to me — the environment variable should override the hidden settings file

deps/build.jl Outdated
@@ -16,6 +28,9 @@ if Base.BLAS.vendor() === :mkl
const libfftwf = "$mklpath"
""")
end
elseif provider == "MKL"
error("MKL build requested for FFTW but Julia was not built with MKL.\n",
"To fix this, " action, " and rerun Pkg.build(\"FFTW\").")
Copy link
Member

@stevengj stevengj Oct 2, 2017

Choose a reason for hiding this comment

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

With my above suggestion, you just need to set ENV["JULIA_FFTW_PROVIDER"]="FFTW" and it will work in all cases ... the user should normally not have to know about the settings file.

@stevengj
Copy link
Member

stevengj commented Oct 2, 2017

(I think it's really important for Pkg3 to standardize these kinds of persistent build-time options: e.g. JuliaLang/Juleps#38 is one rough draft of a possibility)

@ararslan
Copy link
Member Author

ararslan commented Oct 3, 2017

Squashed my previous commits and added an unrelated commit that should get around the failures on nightly.

@ararslan ararslan merged commit 7ee82cc into master Oct 3, 2017
@ararslan ararslan deleted the aa/mkl-env branch October 3, 2017 22:09
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