-
Notifications
You must be signed in to change notification settings - Fork 572
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
make MKL_jll a BuildDependency only for SCS_MKL #6766
make MKL_jll a BuildDependency only for SCS_MKL #6766
Conversation
I don't understand this: if this package has a package which requires MKL (I don't know), how's this optional? |
SCS.jl is a very small package which is <1MB; The last two solvers depend on libs which are over 1GB in size so we thought shipping those by default is an overkill. But we were still unconditionally forcing users to download and install |
MKL is a lazy artifact, it won't be downloaded until it's Yggdrasil/M/MKL/build_tarballs.jl Line 93 in 5b1dd3e
|
well if I do
and then
So it seems that MKL_jll has been eagerly downloaded; How to make it truly lazy then? Note that we Requires.@require(
MKL_jll = "856f044c-d86e-5d09-b602-aeab76dc8ba7",
begin
import SCS_MKL_jll
if SCS_MKL_jll.is_available()
import SCS_MKL_jll.MKL_jll
global mkldirect = SCS_MKL_jll.libscsmkl
push!(available_solvers, MKLDirectSolver)
end
end
) |
Because something has done |
More direct example:
This shows that
is false and the motivation of this PR is ill-based. |
hmm, I see your point but it's precisely SCS_MKL_jll downloading MKL_jll which I'd like to change with this PR:
|
But this PR still doesn't make any sense to me. Generating a broken JLL is a no-go. I believe you're trying to address the problem at the wrong level. |
SCS_GPU_jll also has CUDA_full_jll only as If this is not the right way of achieving this, what are other possibilities? for reference this is our |
This PRSo this PR is breaking if people to use SCS_MKL_jll directly, and they haven't also run But @kalmarek's point is that no one is using SCS_MKL_jll outside of SCS.jl, and that SCS.jl doesn't load SCS_MKL_jll unless MKL_jll is already loaded. This is also the case for SCS_GPU_jll, which only loads if CUDA.jl is also loaded: Yggdrasil/S/SCS_GPU/build_tarballs.jl Line 42 in d55279a
Alternative@giordano's suggestion is to remove ConclusionTo be honest, I'd prefer this PR for now. But we'll need to bump the MAJOR version number because it's a breaking change. Then once Julia 1.9 or 1.10 is the LTS, we can switch to a weakdep and fix this properly? |
I see, I didn't think of asking users to |
@odow So this is a breaking change for As I said, since we're the only users of |
This is what I had in mind.
But yes, I'd agree that this is a bug fix, so I'm okay if you want to leave as-is. |
I can't say I am happy. This won't even work in the registration in General.
My point is that you can't control that. You can't forbid users from using this package directly. I hope it's clear on record that I don't like this and I don't agree with the change. I'm going to merge this just to move on but I won't deal with any breakage this should cause. |
so that MKL_jll is a truly optional dependency for SCS ;)
@giordano does that require version bump, or is +1 fine?
cc: @odow