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

SDP option to use blas-sys and lapack-sys #61

Open
jwnimmer-tri opened this issue Oct 20, 2023 · 5 comments
Open

SDP option to use blas-sys and lapack-sys #61

jwnimmer-tri opened this issue Oct 20, 2023 · 5 comments

Comments

@jwnimmer-tri
Copy link

jwnimmer-tri commented Oct 20, 2023

Here's a feature request ...

Is your feature request related to a problem? Please describe.

In RobotLocomotion/drake#19449, I'm working to add Clarabel integration into Drake. Most users of Drake use our pre-compiled binary releases instead of building from source.

As such, it is not practical for us to force a choice of BLAS/LAPACK at compile time. Instead, we dynamically link to the libblas.so and liblapack.so (libblas.dylib and liblapack.dylib) from the user's operating system, so that their choice of the best BLAS for their own computer applications is respected. It allows us to ship one binary release of Drake that can perform well on many different computers.

At the moment, it does not appear that Clarabel.rs or Clarabel.cpp offer a feature to use the system BLAS, which prevents us from extending our BLAS convention to Clarabel as well.

Describe the solution you'd like

See https://github.com/blas-lapack-rs/blas-lapack-rs.github.io/wiki for background.

The Cargo.toml as of 0.6.0 currently offers these features:

# enables blas/lapack for SDP support, with blas/lapack src unspecified
sdp = ["blas","lapack"]

# explicit configuration options for different blas flavours
sdp-accelerate = ["sdp", "blas-src/accelerate", "lapack-src/accelerate"]
sdp-netlib     = ["sdp", "blas-src/netlib", "lapack-src/netlib"]
sdp-openblas   = ["sdp", "blas-src/openblas", "lapack-src/openblas"]
sdp-mkl        = ["sdp", "blas-src/intel-mkl", "lapack-src/intel-mkl"]
sdp-r          = ["sdp", "blas-src/r", "lapack-src/r"]

I'd like to request a feature option to use the system blas, something like:

sdp-sys        = ["sdp", "blas-sys", "lapack-sys"]

I believe it also might require changes to src/algebra/dense/blas.rs etc to spell things a little differently.

Describe alternatives you've considered

I've been trying to prepare my own patches to accomplish this, but doing it with downstream patching it fairly awkward, and my unfamiliarity with Rust is slowing me down a bit. Since I would plan to upstream the patches anyway, I thought I'd ask here to see if we could find a way to ship 0.7.0 with blas-sys out of the box. Y'all might be able to do this more quickly than myself.

@goulart-paul
Copy link
Member

I am happy to try to support this but I am not clear exactly how it should work. My understanding of the blas-sys and lapack-sys options is that they only provide an alternative style of interface to BLAS, i.e. "C-style" rather than "rust-style".

I think specifying something like

sdp-sys        = ["sdp", "blas-sys", "lapack-sys"]

would only really be promising that the C-style interface is there - it doesn't promise anything about linkage and doesn't go looking for any system-supplied library to load.

The other statuc options, e.g. "sdp-accelerate", just enable one of the BLAS sources for linking. This is done by enabling additional compile-time options.
Example.

A possible model might be to handle access to some system installation in the way we do it in Python. In that case we do not link to BLAS at all, but rather query back to Python and request function pointers for whichever BLAS functions Python is already using. See here.

I don't have a strong feeling about the best way of doing this for Drake. My suspicion is that if it were easy to dynamically link to some "system BLAS" as a standard features in the rust blas crate then this would already exist. It may be very difficult to write a really general dynamically linked option that will always work.

I would therefore suggest that if Drake already knows about some system BLAS and can provide pointers to individual functions, then we write something like the Python loader mechanism for you. That might also avoid issues with loading two different blas libraries at the same time.

@jwnimmer-tri
Copy link
Author

Thanks for the feedback! Let me explore a bit more on my end to see what might suit us best.

@jwnimmer-tri
Copy link
Author

I found https://github.com/rust-ndarray/ndarray#how-to-enable-blas-integration which seems to indicate that openblas-src can be configured to use the system dynamic library. I see if I can wrangle it (or something like it) to get it working.

@jwnimmer-tri
Copy link
Author

Okay, I found a pretty good solution. The Drake PR is RobotLocomotion/drake#20475.

At the lowest layer, Rust is/was already calling into the normal BLAS (and LAPACK) ABI (e.g., dgemm_). The additional capability that e.g. blas-src provides is to control how Rust links to the proper BLAS library (and if necessary, building that library from source). Since in Drake we're not using Rust to create our executables, that part doesn't matter -- I don't need (or want) Rust to control that linking. I can use my normal C++ linker to link to my normal (system) BLAS library alongside Clarabel.cpp, and everything works fine already.

The only change I needed to make to Clarabel.rs was to disable the instruction to use Rust to load the blas_src library that control the Rust linking. Here's that patch:

[clarabel] Opt-out of Rust linking for BLAS/LAPACK

The blas_src and lapack_src crates are the Rust ecosystem's solution
for inversion of control when linking the BLAS and LAPACK libraries.
Library code refers to those crates and then the top-level Cargo file
of the ultimate application binary turns on a feature flag of those
crates that adds the right lines to the linker (and can also rebuild
BLAS and LAPACK from source, if necessary).

However, since Drake's ultimate "application" is to ship our own
shared library, we must not allow Rust to govern which libraries to
link. Thus, we need to delete the lines that perform the linking.

--- src/algebra/dense/blas.rs
+++ src/algebra/dense/blas.rs
@@ -8,8 +8,6 @@ cfg_if::cfg_if! {
     }
         else {   
-        // standard imports via blas-lapack-rs crates 
-        extern crate blas_src;
-        extern crate lapack_src;
+        // Use whatever shared libraries Drake provided.
         use lapack::*;
         use blas::*;
     }

That patch is pretty small so I don't mind at all to carry it locally within Drake indefinitely. If you wanted to, though, I could imagine an upstream feature flag in Clarabel.rs that turned off those two lines, which would allow me to drop my patch, and might make it easier for other users in the future who encounter my same problem.

From my perspective though, Clarabel.rs basically already offers what I need, so I'm OK if you'd like to close this issue.

BTW There was also one tiny cleanup to Clarabel.cpp that I'll file separately.

@jwnimmer-tri
Copy link
Author

jwnimmer-tri commented Nov 3, 2023

I should also mention for the record that turning on Clarabel's sdp-netlib and then setting netlib-src.features = ["system"] in Cargo also mostly achieves the same result (of linking to the system BLAS, instead of building BLAS from source), but with a lot of extra clutter in the Cargo files. The challenge there is that the netlib-src crate has default features enabled of cblas and lapacke and tmg, which adds those extra shared libraries to the linker, but they are not provided (with those names) on Ubuntu. (On Ubuntu, libcblas.so is not a separate library; is part of libblas.so) So for that to be a solution, we need to ensure default-features = false is used for netlib-src and that ends up being a lot of hassle, with changes to at least two (or more?) different Cargo files to support it. For me at least, sidestepping blas-src entirely was much easier. But if you wanted to pursue a Cargo-centric solution that your CMakeLists.txt could use to link the system BLAS, that would probably be the way to do it.

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

No branches or pull requests

2 participants