-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Remove BLAS.vendor() that was deprecated in 1.7 #44379
Conversation
Pinging @chriselrod @ChrisRackauckas @YingboMa @DilumAluthge @dmbates @kshyatt Since it appears that your packages may be using |
It seems like it doesn't cost anything to keep the function and warning around, or? |
@ViralBShah Do you have a list of the packages that currently use this function? |
The function is hard-coded to provide the answer as These are the list of affected packages as @staticfloat reported: Note that in all these cases, even if you load another BLAS, it will report openblas as the vendor. This was meant to be for the use case when we built other BLAS libraries into the system image, and we no longer do that. |
FWIW the ping list seems incomplete, as @tkf should have been pinged for BenchmarkCI.jl. |
I feel that most of the package ecosystem that needs this will be able to quickly update to |
Apologies - the pinging list was built by eyeballing package names whose authors I knew in my head. |
Just from a quick eyeball of the list, many uses seem to be just printing the output of Here's one interesting use that will be a little more work: https://github.com/tkf/BenchmarkCI.jl/blob/5191667898565b80baf36e72503ded0f178413a2/src/runtimeinfo.jl#L50
Can this be achieved by just doing |
SciML usage is already version specific: https://github.com/SciML/LinearSolve.jl/blob/main/src/init.jl#L4-L9 @static if VERSION < v"1.7beta"
blas = BLAS.vendor()
IS_OPENBLAS[] = blas == :openblas64 || blas == :openblas
else
IS_OPENBLAS[] = occursin("openblas", BLAS.get_config().loaded_libs[1].libname)
end Other packages can copy this workaround and we can move on without a major issue. It's mostly the SciML squad anyways. There's probably a few spots that haven't done this yet, but oh well rip the bandaid off and we'll find it. |
Yes, and this is why there is a deprecation warning. Isn't a result better than an error? If the result of the function causes problems downstream that will be noticed anyway. Even if new code can be updated, it causes unecessary churn and everyone needs to do new releases etc etc. |
We can update MixedModels do something like the suggestion above but it feels a little weird to drop a function that was deprecated after LTS before the next LTS is blessed. |
And looking again, we're already doing that: @static if VERSION ≥ v"1.7.0-DEV.620"
@show getproperty.(BLAS.get_config().loaded_libs, :libname)
else
@show BLAS.vendor()
if startswith(string(BLAS.vendor()), "openblas")
println(BLAS.openblas_get_config())
end
end |
The SciML workaround exists because checking the vendor at runtime is very expensive. The drawback here is that users must change |
Yeah, it was like 3 orders of magnitude slower than what we had before, so we had to resort to fixing it at |
I should note that, in the event that you actually deeply care about the library that you're using, it's not enough to just scan the list of loaded libraries, you need to either ensure that there is only one library loaded, or inspect things symbol by symbol. I kind of don't expect most people to care about this that much, so I would just ensure that there's only one ILP64 library loaded at once and throw an error otherwise. For the curious, you can get the nitty-gritty details via something like the following: julia> using LinearAlgebra
BLAS.lbt_find_backing_library("dgemm_", :ilp64)
LinearAlgebra.BLAS.LBTLibraryInfo
├ Library: libopenblas64_.so
├ Interface: ilp64
├ Complex return style: normal
├ F2C: plain
└ CBLAS: conformant |
We need to check if something is using OpenBLAS because it is slow enough that we want to use a Julia implementation for more matrix sizes. If the user is not using OpenBLAS, then we assume it's something better. |
We just log it for debugging purposes at the start of |
Checking takes ~121μs on my machine:
I agree, 5k allocations is a bit much though. This happens because there's a lot of extra information that gets pulled out every time. I've got a PR here to cache things until someone calls one of the julia> using LinearAlgebra, BenchmarkTools
@btime BLAS.lbt_get_config().loaded_libs
2.855 ns (0 allocations: 0 bytes)
1-element Vector{LinearAlgebra.BLAS.LBTLibraryInfo}:
LBTLibraryInfo(libopenblas64_.so, ilp64) |
Note that this function is not exported and not documented. The actual number of packages impacted is low enough that I feel we can safely execute this. |
The result will be incorrect when using MKL.jl and non-default BLAS, and is strictly worse than an error. If we choose to keep it, my preference would be to generate an exception. |
julia> using LinearAlgebra, BenchmarkTools
julia> A = rand(10,10);
julia> @btime lu(A);
5.300 μs (3 allocations: 1.05 KiB) so checking if it's OpenBLAS would give a 24x performance regression! We want to check if it's OpenBLAS because even that is slow, so we want to move to RecursiveFactorization.jl in that case. But MKL is good enough that we want to just use that if the user is using MKL. And 5us to determine the right algorithm would be an insane cost, hence we cache it. 3ns is fine though 😅. |
The other possibility is that we know the different kinds of BLAS supported in LBT. So we just respond with whichever ILP64 BLAS is loaded by looking at the output of |
FWIW: julia> A = rand(100,100);
julia> using LinearAlgebra
julia> @benchmark lu($A)
BenchmarkTools.Trial: 3913 samples with 1 evaluation.
Range (min … max): 526.514 μs … 83.364 ms ┊ GC (min … max): 0.00% … 0.00%
Time (median): 837.870 μs ┊ GC (median): 0.00%
Time (mean ± σ): 1.272 ms ± 5.036 ms ┊ GC (mean ± σ): 0.09% ± 1.89%
▁ ▄█
█▃██▃▃▂▂▂▃▃▂▂▂▂▂▂▂▁▂▂▁▂▂▂▂▁▁▁▁▂▁▂▂▁▁▁▂▂▁▂▁▁▂▁▁▂▁▁▁▁▁▂▁▁▂▁▁▁▂ ▂
527 μs Histogram: frequency by time 6.69 ms <
Memory estimate: 79.05 KiB, allocs estimate: 3.
julia> using MKL
julia> @benchmark lu($A)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
Range (min … max): 44.580 μs … 1.654 ms ┊ GC (min … max): 0.00% … 93.10%
Time (median): 45.746 μs ┊ GC (median): 0.00%
Time (mean ± σ): 51.352 μs ± 44.320 μs ┊ GC (mean ± σ): 2.87% ± 3.56%
▄█▇▃▁ ▁▂▂▁ ▁▂▂▃▂▂▂ ▁▁▁▁ ▂
██████▇████▇▇▆▃▄▄▁▄▃▁▁▁▁▃▁▁▁▁▁▁▃▅▄▃▁▃▁▁▃▁▁▁▅███████████████ █
44.6 μs Histogram: log(frequency) by time 78.2 μs <
Memory estimate: 79.05 KiB, allocs estimate: 3.
julia> using RecursiveFactorization
julia> @benchmark RecursiveFactorization.lu($A)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
Range (min … max): 41.652 μs … 37.723 ms ┊ GC (min … max): 0.00% … 1.94%
Time (median): 44.728 μs ┊ GC (median): 0.00%
Time (mean ± σ): 110.351 μs ± 1.551 ms ┊ GC (mean ± σ): 1.13% ± 0.08%
▃▅▇██▇▆▄▃▃▂▂ ▁▁▁▁▁ ▂
██████████████▇▅▁▁▁▄▃▃▃▁▁▄▅▃▁▁▃▃▁▁▁▃▁▁▁▁▁▁▁▁▁▃▆▆▇███████████ █
41.7 μs Histogram: log(frequency) by time 76.2 μs <
Memory estimate: 79.12 KiB, allocs estimate: 5. LinearSolve.jl will use RecursiveFactorization at this size regardless of BLAS vendor. It'll choose RecursiveFactorization over OpenBLAS but not some other library over the size range 101:500. Below that, it's always RF, and above it's always the other library. If the check were expensive, it'd be best to just always use RF. |
I generally support this idea and this can be removed even without a ping to the package authors. I don't mind if this breaks my package if it's my "fault." (That said, JuliaLang/LinearAlgebra.jl#847 indicates that we want to "be nice" with people using this function. I think we can also just swap |
I've made |
dcddded
to
e52fa73
Compare
As recommended by @fredrikekre in JuliaLinearAlgebra/GenericLinearAlgebra.jl#85 (review), I am updating this PR to now instead do:
This solves the issue of giving the correct answers, and maintaining backward compatibility. @staticfloat and I have plans to provide more structured ways of querying for vendor inside the |
Removing backport label since this causes the following to error on 1.8 (https://build.julialang.org/#/builders/41/builds/3672):
|
Note that this and #44360 are a pair (so either both backport or neither do). It should be possible to figure out this segfault, since these PRs are fairly straightforward. I'll try this out on 1.8 and see if I can resolve it asap. |
Actually this is happening because it seems like #44360 is not backported to 1.8 before this one. |
BLAS.vendor()
had a deprecation warning in 1.7 and 1.9. I think it should be ok to remove in 1.9.