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

Simplification of libblastrampoline stuff in LinearAlgebra #44360

Merged
merged 6 commits into from
Feb 28, 2022

Conversation

ViralBShah
Copy link
Member

@ViralBShah ViralBShah commented Feb 26, 2022

We traditionally hardcode libblas_name and liblapack_name into the system image. These do not get updated, even when you use an alternate set of libraries through MKL.jl. build_h.jl was reporting different names than what was in the BLAS and LAPACK modules in LinearAlgebra.

Now that we use LBT, it is best to mention libblastrampoline as the provider of these libraries. This revealed a lot of old code that could be simplified and cleaned up.

Perhaps we can remove these things altogether from build_h.jl, since these are no longer build time choices, but runtime ones.

Also undo the compatibility symbols added in #39845 - since the new APIs now have been around for a while.

cc @staticfloat

@ViralBShah ViralBShah added linear algebra Linear algebra backport 1.8 Change should be backported to release-1.8 and removed backport 1.8 Change should be backported to release-1.8 labels Feb 26, 2022
@ViralBShah ViralBShah added the backport 1.8 Change should be backported to release-1.8 label Feb 26, 2022
@ViralBShah ViralBShah changed the title Report libblastrampoline as Base.libblas_name and Base.liblapack_name Simplification of libblastrampoline stuff in LinearAlgebra Feb 26, 2022
@staticfloat
Copy link
Member

I think this makes sense, overall. It's probably worth a NEWS.md entry to state that we're now wholly reliant upon LBT, and that we straight up don't support building against any other BLAS/LAPACK library, and that any other usage should be done through LBT configuration at runtime.

@ViralBShah
Copy link
Member Author

How do you feel about removing blas and LAPACK from build_h.jl altogether?

@staticfloat
Copy link
Member

Yeah, I think that's the right thing to do. Those values are easily made into lies.

@ViralBShah ViralBShah removed the backport 1.8 Change should be backported to release-1.8 label Feb 27, 2022
@ViralBShah
Copy link
Member Author

ViralBShah commented Feb 27, 2022

@staticfloat Any thoughts on why this PR might cause the precompile tests to fail? I made some changes to a couple of JLL packages in terms of dependencies. Do I need to update some package related metadata?

Error in testset precompile:
Test Failed at /Users/viral/julia/test/precompile.jl:352
  Expression: Dict(modules) == merge(Dict((let m = Base.PkgId(s)
                m => Base.module_build_id(Base.root_module(m))
            end for s = ["Base", "Core", "Main", string(Foo2_module), string(FooBase_module)])), Dict((let m = Base.root_module(Base, s)
                Base.PkgId(m) => Base.module_build_id(m)
            end for s = [:ArgTools, :Artifacts, :Base64, :CRC32c, :Dates, :DelimitedFiles, :Distributed, :Downloads, :FileWatching, :Future, :InteractiveUtils, :LazyArtifacts, :LibCURL, :LibCURL_jll, :LibGit2, :Libdl, :LinearAlgebra, :Logging, :Markdown, :Mmap, :MozillaCACerts_jll, :NetworkOptions, :Pkg, :Printf, :Profile, :p7zip_jll, :REPL, :Random, :SHA, :Serialization, :SharedArrays, :Sockets, :SparseArrays, :Statistics, :SuiteSparse, :TOML, :Tar, :Test, :UUIDs, :Unicode, :nghttp2_jll])))
   Evaluated: Dict{Base.PkgId, UInt64}(Statistics [10745b16-79ce-11e8-11f9-7d13ad32a3b2] => 0x0002eb23c71144be, SparseArrays [2f01184e-e22b-5df5-ae63-d93ebab69eaf] => 0x0002eb2214c670f7, Downloads [f43a241f-c20a-4ad4-852c-f6b1247861c6] => 0x0002eb244b8f16f8, Base64 [2a0f44e3-6c83-55bd-87e4-b1978d98bd5f] => 0x0002eb1e01380cf9, F2oo4b3a94a1a081a8cb [top-level] => 0x0003278108320f26, Markdown [d6f4376e-aef5-505a-96c1-9c027394607a] => 0x0002eb206cfe7dba, Unicode [4ec0a83e-493e-50e2-b9ac-8f72acf5a8f5] => 0x0002eb1e4ee6f142, FooBase4b3a94a1a081a8cb [top-level] => 0x0003278129bca2c7, CompilerSupportLibraries_jll [e66e0078-7015-5450-92f7-15fbd957f2ae] => 0x0002eb1e5ba137d5, MozillaCACerts_jll [14a3606d-f60d-562e-9121-12d972cd8159] => 0x0002eb2436d83a53…) == Dict{Base.PkgId, UInt64}(Downloads [f43a241f-c20a-4ad4-852c-f6b1247861c6] => 0x0002eb244b8f16f8, Unicode [4ec0a83e-493e-50e2-b9ac-8f72acf5a8f5] => 0x0002eb1e4ee6f142, FooBase4b3a94a1a081a8cb [top-level] => 0x0003278129bca2c7, Main [top-level] => 0x0002eb05679d7af1, REPL [3fa0cd96-eef1-5676-8a61-b3b8758bbffb] => 0x0002eb22cf73e795, NetworkOptions [ca575930-c2e3-43a9-ace4-1e988b2c1908] => 0x0002eb1e14f739f3, DelimitedFiles [8bb1440f-4735-579b-a4ab-409b98df4dab] => 0x0002eb1e55dfc8eb, Serialization [9e88b42a-f829-5b0c-bbe9-9e923198166b] => 0x0002eb1e2c091fd6, Profile [9abbd945-dff8-562f-b5e8-e1ebf5ef1b79] => 0x0002eb220120a768, UUIDs [cf7118a7-6976-5b1a-9a39-7adc72f591a4] => 0x0002eb22cc2447a0…)

@ViralBShah
Copy link
Member Author

This is good to merge.

@staticfloat staticfloat merged commit e84b06c into master Feb 28, 2022
@staticfloat staticfloat deleted the vs/build-h-lbt branch February 28, 2022 16:32
@ViralBShah
Copy link
Member Author

@staticfloat How do you feel about also removing BLAS.vendor()? It is already deprecated in 1.8, so should be ok to remove in 1.9.

@staticfloat
Copy link
Member

staticfloat commented Feb 28, 2022

Sure, seems fine to me. A quick code search doesn't reveal too many uses: https://juliahub.com/ui/Search?q=BLAS.vendor%28%29&type=code

@Keno
Copy link
Member

Keno commented Mar 1, 2022

This broke a lot of the package ecosystem.

@ViralBShah
Copy link
Member Author

ViralBShah commented Mar 1, 2022

Fixes are incoming. I'll be happy to sort them all out over the next couple of days.

@ViralBShah
Copy link
Member Author

ViralBShah commented Mar 1, 2022

These are the packages that need updating:

Those calling BLAS: https://juliahub.com/ui/Search?q=libblas_name&type=code
Those calling LAPACK: https://juliahub.com/ui/Search?q=liblapack_name&type=code

staticfloat pushed a commit to JuliaCI/julia-buildkite-testing that referenced this pull request Mar 2, 2022
…#44360)

* Report libblastrampoline as Base.libblas_name and Base.liblapack_name

* Cleanup a lot of the LBT stuff

* Fix

* Remove test for LinearAlgebra.LAPACK.liblapack

* Fix

* Fix the precompile tests
@ViralBShah ViralBShah added the backport 1.8 Change should be backported to release-1.8 label Mar 3, 2022
@ViralBShah
Copy link
Member Author

ViralBShah commented Mar 3, 2022

In combination with #44379, this is no longer a breaking change, but fixes a bug where using other BLAS libraries (like MKL.jl) would not be reflected correctly in libblas_name and vendor.

I believe this PR should be backported first, and then #44379 on top of it.

@KristofferC KristofferC mentioned this pull request Mar 3, 2022
47 tasks
ViralBShah added a commit that referenced this pull request Mar 5, 2022
* Report libblastrampoline as Base.libblas_name and Base.liblapack_name

* Cleanup a lot of the LBT stuff

* Fix

* Remove test for LinearAlgebra.LAPACK.liblapack

* Fix

* Fix the precompile tests

(cherry picked from commit e84b06c)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…#44360)

* Report libblastrampoline as Base.libblas_name and Base.liblapack_name

* Cleanup a lot of the LBT stuff

* Fix

* Remove test for LinearAlgebra.LAPACK.liblapack

* Fix

* Fix the precompile tests
@KristofferC KristofferC mentioned this pull request May 28, 2022
36 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Jun 8, 2022
@KristofferC
Copy link
Member

KristofferC commented May 15, 2023

For reference, this broke Pardiso.jl on mac, JuliaSparse/Pardiso.jl#90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants