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

Blas linkage #54

Closed
gfaster opened this issue Mar 7, 2023 · 3 comments
Closed

Blas linkage #54

gfaster opened this issue Mar 7, 2023 · 3 comments
Assignees

Comments

@gfaster
Copy link

gfaster commented Mar 7, 2023

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
Copy link
Author

gfaster commented Mar 7, 2023

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.

@gfaster gfaster changed the title Remove linkage to openblas from build.rs, instead link to blas Blas linkage Mar 7, 2023
@Axect
Copy link
Owner

Axect commented Mar 8, 2023

@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 😞.

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

@Axect Axect self-assigned this Mar 8, 2023
Axect added a commit that referenced this issue Mar 8, 2023
Axect added a commit that referenced this issue Mar 8, 2023
* Remove `build.rs`
Axect added a commit that referenced this issue Mar 8, 2023
* Remove `build.rs`
@Axect
Copy link
Owner

Axect commented Mar 8, 2023

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.

@Axect Axect closed this as completed Mar 14, 2023
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