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

Ipopt_jll with LBT #327

Merged
merged 18 commits into from
Oct 19, 2022
Merged

Ipopt_jll with LBT #327

merged 18 commits into from
Oct 19, 2022

Conversation

amontoison
Copy link
Contributor

@amontoison amontoison commented Aug 24, 2022

close #6

I updated and recompiled MUMPS_seq_jll / Ipopt_jll with libblastrampoline.
We can easily switch the BLAS backend now with the version 300.1400.401 of Ipopt_jll.

import LinearAlgebra, OpenBLAS32_jll
LinearAlgebra.BLAS.lbt_forward(OpenBLAS32_jll.libopenblas_path)

or

import LinearAlgebra, MKL_jll
LinearAlgebra.BLAS.lbt_forward(MKL_jll.libmkl_rt_path)

Because Ipopt_jll and MUMPS_seq_jll are compiled with LBT_jll now, we must add OpenBLAS32_jll as dependency here to have a least one 32 bit BLAS/LAPACK backend.

Project.toml Show resolved Hide resolved
src/Ipopt.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Aug 24, 2022

Can we test with MKL on CI machines?

@amontoison
Copy link
Contributor Author

Can we test with MKL on CI machines?

Yes, you just need to add MKL_jll as a test dependency.
This simple modification should work.

@testset "C" begin
    include("C_wrapper.jl")
end

@testset "MathOptInterface" begin
    include("MOI_wrapper.jl")
end

import LinearAlgebra, MKL_jll
LinearAlgebra.BLAS.lbt_forward(MKL_jll.libmkl_rt_path)

@testset "C" begin
    include("C_wrapper.jl")
end

@testset "MathOptInterface" begin
    include("MOI_wrapper.jl")
end

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Base: 91.52% // Head: 93.22% // Increases project coverage by +1.70% 🎉

Coverage data is based on head (7441c41) compared to base (7162015).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   91.52%   93.22%   +1.70%     
==========================================
  Files           4        4              
  Lines         743      797      +54     
==========================================
+ Hits          680      743      +63     
+ Misses         63       54       -9     
Impacted Files Coverage Δ
src/Ipopt.jl 100.00% <100.00%> (ø)
src/utils.jl 99.58% <0.00%> (+0.07%) ⬆️
src/MOI_wrapper.jl 91.20% <0.00%> (+2.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@odow
Copy link
Member

odow commented Aug 24, 2022

Windows segfault seems related: https://github.com/jump-dev/Ipopt.jl/runs/7986459839?check_suite_focus=true

Does LBT not work on Windows?

@amontoison
Copy link
Contributor Author

Windows segfault seems related: https://github.com/jump-dev/Ipopt.jl/runs/7986459839?check_suite_focus=true

Does LBT not work on Windows?

I never tested LBT on Windows. I don't know if it's related to the new version of MUMPS (5.5.1) or LBT.
I also use LBT for HSL.jl but Windows was not tested.

@amontoison
Copy link
Contributor Author

Windows segfault seems related: https://github.com/jump-dev/Ipopt.jl/runs/7986459839?check_suite_focus=true

Does LBT not work on Windows?

It seems to be related to OpenBLAS32.
We should test with v0.3.10 and v0.3.12 to see if we also have this error.

@odow
Copy link
Member

odow commented Aug 24, 2022

The null pointer suggests that something isn't loaded properly.

@ViralBShah is there anything we need to do for LBT on Windows?

@ViralBShah
Copy link

ViralBShah commented Aug 24, 2022

Not to the best of my knowledge. @giordano perhaps.

@ViralBShah
Copy link

I suggest passing all the arguments to lbt_forward to make sure we are not clearing the ilp64 symbols.

@ViralBShah
Copy link

Also why such an old OpenBLAS32?

@ViralBShah
Copy link

ViralBShah commented Aug 24, 2022

Also, perhaps we want to only forward if the user has not already set up a forward. Otherwise you have to load packages in the right order so that MKL.jl forwards are not overwritten by this package for example. Eventually we need a better Julia wide mechanism to use Preferences to pick BLAS etc.

Project.toml Outdated Show resolved Hide resolved
src/Ipopt.jl Outdated Show resolved Hide resolved
@amontoison
Copy link
Contributor Author

Also, perhaps we want to only forward if the user has not already set up a forward. Otherwise you have to load packages in the right order so that MKL.jl forwards are not overwritten by this package for example. Eventually we need a better Julia wide mechanism to use Preferences to pick BLAS etc.

Is it possible to detect if the user has already set up a 32 bits BLAS/LAPACK backend ?

@ViralBShah
Copy link

Is it possible to detect if the user has already set up a 32 bits BLAS/LAPACK backend ?

Yes, LBT allows you to query with BLAS.lbt_get_config()

@ViralBShah
Copy link

ViralBShah commented Aug 24, 2022

Does LBT not work on Windows?

Julia base uses LBT by default, and so this is checked in Julia base CI etc. Also MKL.jl CI tests Windows, and it works fine. We don't do anything special there, AFAICT.

@giordano
Copy link

is there anything we need to do for LBT on Windows?

I'm not aware of anything special being needed for Windows

@ViralBShah
Copy link

Can someone fix the CI and re-trigger?

@odow
Copy link
Member

odow commented Aug 24, 2022

I added a check for LP64 BLAS, but the problem is still there.

@odow
Copy link
Member

odow commented Aug 24, 2022

Does it matter that OpenBLAS32_jll and OpenBLAS_jll are different versions?

@amontoison
Copy link
Contributor Author

I confirm that we have also the null pointer with MUMPS 5.4.1 + LBT.
https://github.com/JuliaSmoothOptimizers/MUMPS.jl/runs/8124551353?check_suite_focus=true#step:6:208

@ViralBShah
Copy link

Can we also try 32-bit Windows here?

@odow
Copy link
Member

odow commented Sep 2, 2022

Can we also try 32-bit Windows here?

Added. Running now: https://github.com/jump-dev/Ipopt.jl/runs/8148650553?check_suite_focus=true

@odow
Copy link
Member

odow commented Sep 2, 2022

Same null pointer error on 32-bit

@amontoison
Copy link
Contributor Author

@odow, I rebuild MUMPS_seq and Ipopt. I hope that it works now 🤞

test/C_wrapper.jl Outdated Show resolved Hide resolved
test/C_wrapper.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Oct 8, 2022

@amontoison happy? So we've just disabled LBT on Windows for now?

@amontoison
Copy link
Contributor Author

amontoison commented Oct 9, 2022

@amontoison happy? So we've just disabled LBT on Windows for now?

Yes, it's finally working!
We will not use LBT on Windows until Julia v"1.9". Like OpenBLAS the version of LBT is fixed for each Julia version.
With Julia v"1.9", the issues with Windows should be fixed and we will be able to recompile again MUMPS and Ipopt (if we want to support the same feature on Windows).

@ViralBShah Is it possible to do a new release of MKL.jl such that using MKL automatically load a LP64 BLAS?
We already have this feature but we must use the branch master for now.

For Apple platforms, we could use directly Apple Accelerate what do you think?
We could have a significant speed-up with the new M1 architecture.
It's a LP64 BLAS/LAPACK, which is exactly what we need for MUMPS / Ipopt.

if VERSION >= v"1.8"
    config = LinearAlgebra.BLAS.lbt_get_config()
    if !any(lib -> lib.interface == :lp64, config.loaded_libs)
        if !Sys.isapple()
            LinearAlgebra.BLAS.lbt_forward(OpenBLAS32_jll.libopenblas_path, verbose = true, clear = false)
        else
            blas = "/System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/libBLAS.dylib"
            lapack = "/System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/libLAPACK.dylib"
            LinearAlgebra.BLAS.lbt_forward(blas, verbose = true, clear = false)
            LinearAlgebra.BLAS.lbt_forward(lapack, verbose = true, clear = false)
        end
    end
end

@ViralBShah
Copy link

I have been holding off on a new release because IIRC, the latest master does not pass on one of the windows.

I will trigger another build today. On Mac it does make sense to link to MKL.

@amontoison
Copy link
Contributor Author

MKL is not efficient on Mac because it's only working with one thread:
JuliaLinearAlgebra/MKL.jl#111

@ViralBShah
Copy link

Oops - sorry I was typing on my phone in a hurry. I meant to say that on mac, it does make sense to link to Accelerate.

src/Ipopt.jl Show resolved Hide resolved
@ViralBShah
Copy link

ViralBShah commented Oct 14, 2022

I've tagged MKL.jl 0.6

@ViralBShah
Copy link

If 32-bit BLAS is being linked against through LBT, then on Apple, we should also be able to set up forwarding to Accelerate and see if performance is better.

@amontoison
Copy link
Contributor Author

Thanks for the new release of MKL @ViralBShah. For Apple Accelerate, we did some tests with @dpo for HSL and the performances seem similar.
JuliaSmoothOptimizers/HSL.jl#124 (comment)
We decided to use OpenBLAS32 by default and add documentation to explain how to load MKL or Apple Accelerate.

@odow
Copy link
Member

odow commented Oct 19, 2022

@amontoison are you happy for this to be merged?

@amontoison
Copy link
Contributor Author

Yes, It's a nice feature for Ipopt.jl.

@odow odow merged commit 5fb1c6e into jump-dev:master Oct 19, 2022
@amontoison amontoison deleted the ipopt_lbt branch October 19, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

distribute with MKL?
5 participants