search

Axect / Peroxide

Rust numeric library with R, MATLAB & Python syntax
https://crates.io/crates/peroxide
Apache License 2.0
509 stars 31 forks source link

Blas linkage #54

Closed gfaster closed 1 year ago

gfaster commented 1 year ago

As it stands now, Peroxide enforces use of OpenBLAS even though all the libraries and apis it uses are agnostic to the BLAS library variant. In order to use a different BLAS implementation, one must delete Peroxide's build.rs, everything works fine otherwise. Especially as this tutorial page is linked in the README, I think it should be expected that the user should be the one to include the linkage lines in their build.rs.

Realistically, I think there are a few good ways of changing this:

  1. Just remove Peroxide's build.rsand leave it to the user to include the right libraries (I believe this is the best course of action)
  2. Add a "no explicit linkage" feature that disables Peroxide's build.rs
  3. Change the linkage from openblas to blas - many distros/package managers will have libblas symlinked to the installed BLAS implementation, OpenBLAS included.
  4. Add links tag to Cargo.toml to allow for explicit overriding

Regardless, Peroxide explicitly linking to OpenBLAS is undesirable due to the plethora of other BLAS implementations (including open source ones like ClBlast) that can be used with no source code changes.

gfaster commented 1 year ago

Reading Cargo book and it seems like convention dictates build.rs should not link to any system libraries as it already depends on blas which in turn depends on blas-sys. blas-sys, according to Cargo convention, would be the package to link to the native library. However, as this issue shows, it's not nearly that simple.

From this, I would conclude that Peroxide should definitely not include any linkage in build.rs, but as blas-sys does not link as it should by convention, it should be left to the end user to link but continue manually linking for development.

Axect commented 1 year ago

@gfaster Thank you for great feedback! I totally agree with your suggestion to remove the linkage and leave it to the user to include the appropriate BLAS implementations in their build.rs. I had implemented it a while back for people unfamiliar with Rust and the BLAS ecosystem (I wasn't familiar with it at the time either), but I haven't gotten around to fixing it since :disappointed:.

Thank you again for reporting this issue. I'll fix this problem and publish new version soon :rocket:

Axect commented 1 year ago

I've released Ver 0.33.0 with your feedback and updated the BLAS description in the README accordingly. You should now be able to use the BLAS backend of your choice.