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

Fix BLAS portability issues with Ubuntu 20.04 R builds #238

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

glin
Copy link
Contributor

@glin glin commented Aug 20, 2024

Issue reported by @dpastoor about some Package Manager binary packages failing to load on Ubuntu 20, like RcppArmadillo.

When we switched the Ubuntu 20 builds to use OpenBLAS once again (#215), this inadvertently changed R to link against OpenBLAS instead of the generic libblas.so that all Ubuntu builds should be linking against. The generic libblas.so is what allows BLAS libraries to be freely swapped on Ubuntu/Debian without breaking binaries that use it.

So Package Manager binaries that use BLAS are now broken for anyone that installed R on Ubuntu 20 before May 2024. The workaround would be to manually install OpenBLAS via apt install libopenblas0-pthread.

This was also the root cause of the Cloud issue from a few months back. Switching the BLAS dependency to OpenBLAS shouldn't have actually changed what R linked to and broken anything. I just didn't realize this back then.

So currently, Ubuntu 20 R links to libopenblas.so:

$ readelf -d $(R RHOME)/lib/libR.so | grep blas
 0x0000000000000001 (NEEDED)             Shared library: [libopenblas.so.0]

This is what R should be linking to instead, from the Ubuntu 22/24 builds:

# jammy
$ readelf -d /opt/R/4.4.0/lib/R/lib/libR.so | grep blas
 0x0000000000000001 (NEEDED)             Shared library: [libblas.so.3]

The fix was to remove the development package for OpenBLAS. Apparently if both OpenBLAS and default BLAS dev headers are present, R will choose OpenBLAS first. Alternatively we can explictly specify the linker flags via --with-blas=-lblas I think, but I didn't want to change too much.

I've added a comment in the latest Ubuntu dockerfile to document this for the future.

@glin glin requested a review from stevenolen as a code owner August 20, 2024 21:06
@glin
Copy link
Contributor Author

glin commented Aug 20, 2024

Heads up @stevenolen, this will change the Ubuntu 20 builds but it should not break anything on Cloud. This reverts the Ubuntu 20 R binary to how it was before, linking to libblas.so, which should still be present on those images.

@glin glin requested review from gaborcsardi, a team, nodivbyzero and nunyunuymi and removed request for a team August 20, 2024 21:16
Copy link
Collaborator

@stevenolen stevenolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find! we have a much more consistent R upgrade path now, but to confirm based on what I'm reading, new R versions built by r-builds will continue to work in an environment with libopenblas-dev (our posit.cloud projects exist in this environment since the previous change)?

Is it preferable to remove libopenblas-dev from these environments? I think our path out of that is more difficult, since we'd need to replace every R version we support after this is built, but also after removing libopenblas-dev from our project environments (but how will those environments work with the previously built R versions)?

@gaborcsardi
Copy link
Contributor

FWIW, 22.04 has libopenblas-base, and so do the Debian Dockerfiles.

@glin
Copy link
Contributor Author

glin commented Aug 21, 2024

@stevenolen Yes, new R versions will continue to work either with libopenblas-dev or without libopenblas-dev. Having OpenBLAS there is better since it's faster than the default BLAS, but I think you do need to keep it anyway for the existing projects that have packages linked directly to it. Removing libopenblas-dev will probably break environments created in the last few months, and also have issues with using the PPM binaries with this issue in the last few months.

So no action required, but it'd still be good to keep an eye out when this deploys.

@glin
Copy link
Contributor Author

glin commented Aug 21, 2024

@gaborcsardi Ah yeah, there do seem to be inconsistencies with the OpenBLAS package names across images. Apparently libopenblas-base was an old package that just aliases to libopenblas0 now, which defaults to bringing in libopenblas0-pthread. Ubuntu 24 looks fine, but I can switch the package name in the Debian 12 image for future copy-paste updates.

@glin glin merged commit 964a433 into main Aug 21, 2024
228 checks passed
@glin glin deleted the focal-blas branch August 21, 2024 20:01
@glin
Copy link
Contributor Author

glin commented Aug 22, 2024

@stevenolen Rebuilding all the Ubuntu 20 binaries for production now, so they should be updated by tomorrow.

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

Successfully merging this pull request may close these issues.

3 participants