Add CI job to build LAPACK with optimised BLAS

[mohawk2]

Description

There is currently a CI job to build LAPACKE + CBLAS with optimised BLAS and LAPACK, but not LAPACK with optimised BLAS. This is intended to exercise the upcoming SET_XERBLA (#1407), which will need to function when the BLAS it's built against doesn't yet have SET_BLAS_XERBLA.

Checklist

• The documentation has been updated. (N/A)
• If the PR solves a specific issue, it is set to be closed on merge. (N/A)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.36%. Comparing base (99a37fd) to head (d5ea3f3).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1419   +/-   ##
=======================================
  Coverage   69.36%   69.36%           
=======================================
  Files        6122     6122           
  Lines      486711   486711           
  Branches    23268    23268           
=======================================
  Hits       337584   337584           
  Misses     148689   148689           
  Partials      438      438           

Components	Coverage Δ
BLAS	97.94% <ø> (ø)
CBLAS	96.98% <ø> (ø)
LAPACK	82.38% <ø> (ø)
LAPACKE	2.17% <ø> (ø)
TMGLIB	55.69% <ø> (ø)
BLAS testing	88.33% <ø> (ø)
CBLAS testing	89.63% <ø> (ø)
LAPACK testing	82.20% <ø> (ø)
LAPACKE testing	∅ <ø> (∅)

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99a37fd...d5ea3f3. Read the comment docs.

[mohawk2]

Thanks for the CI run, last go failed because I hadn't made it install libblas64-dev. Rectified and force-pushed.

[ACSimon33]

Thanks for the CI run, last go failed because I hadn't made it install libblas64-dev. Rectified and force-pushed.

I'm not sure if this will work. My guess is that it will pick only one of the libraries . Also does libblas64-dev export the extended API symbols or just the normal symbols with 64bit integers?

I think the easiest would be to just disable the extended API.

[mohawk2]

That may indeed turn out to be needed. Thank you for initiating the CI, I'll wait and see. If I understand right, that does raise the question of how to interface with optimised BLAS with 64-bit integers; may need additional CMake args like BLAS64_LIBRARIES.

[ACSimon33]

That may indeed turn out to be needed. Thank you for initiating the CI, I'll wait and see. If I understand right, that does raise the question of how to interface with optimised BLAS with 64-bit integers; may need additional CMake args like BLAS64_LIBRARIES.

Well, I think the idea of the extended API was to have both versions (32-bit and 64-bit integers) in the same library. So ideally, libblas-dev would provide it (maybe in the next release). libblas64-dev would then provide either just the standard API with 64-bit integers (like it does now) or both the standard and extended APIs, both with 64-bit integers (a bit redundant, but it is what it is).

MKL does exactly that. Which means it doesn't matter against which version we link, we always get both APIs which is what we need.

[mohawk2]

Providing both 32 and 64 with the same library is definitely nice. But if Debian splits them, that suggests to me that LAPACK needs to handle that. What do you think?

It seems LAPACK indeed can't handle the BLASes being split, as it's just failed again. I've scope-reduced this by turning off BUILD_INDEX64_EXT_API and force-pushed. An obvious additional bit of work would be to enable a split, but that would be a separate PR.

[martin-frbg]

Well, the entire idea (and initial implementation) of the extended API came from the MKL team IIRC , so small wonder it does this. OpenBLAS optionally does the "64" suffixing, but only in the shared library, and its build system hasn't been updated yet to support combined builds

[ACSimon33]

Providing both 32 and 64 with the same library is definitely nice. But if Debian splits them, that suggests to me that LAPACK needs to handle that. What do you think?

It seems LAPACK indeed can't handle the BLASes being split, as it's just failed again. I've scope-reduced this by turning off BUILD_INDEX64_EXT_API and force-pushed. An obvious additional bit of work would be to enable a split, but that would be a separate PR.

I mean they have to be split, since the symbol names are the same. The Debian libraries libblas.so (build with -DBUILD_INDEX64=OFF) and libblas64.so (build with -DBUILD_INDEX64=ON) both contain only the standard API, i.e. the same symbols. So we can't handle both at the same time. When we build LAPACK with -DBUILD_INDEX64=ON, we have to use libblas64.so and if we build with -DBUILD_INDEX64=OFF we have to use libblas.so. To do that, we have to set BLA_SIZEOF_INTEGER=8 or BLA_SIZEOF_INTEGER=4, respectively, in CMake so find_package(BLAS) selects the correct one (we don't do that at the moment ... we should fix that).

Regarding BUILD_INDEX64_EXT_API, we probably should tell whoever builds the release packages for Debian, to build with -DBUILD_INDEX64_EXT_API=ON for the next release. My guess is that they use the Makefiles, which don't support the extended API.

[mohawk2]

Providing both 32 and 64 with the same library is definitely nice. But if Debian splits them, that suggests to me that LAPACK needs to handle that. What do you think?
It seems LAPACK indeed can't handle the BLASes being split, as it's just failed again. I've scope-reduced this by turning off BUILD_INDEX64_EXT_API and force-pushed. An obvious additional bit of work would be to enable a split, but that would be a separate PR.

I mean they have to be split, since the symbol names are the same. The Debian libraries libblas.so (build with -DBUILD_INDEX64=OFF) and libblas64.so (build with -DBUILD_INDEX64=ON) both contain only the standard API, i.e. the same symbols. So we can't handle both at the same time. When we build LAPACK with -DBUILD_INDEX64=ON, we have to use libblas64.so and if we build with -DBUILD_INDEX64=OFF we have to use libblas.so. To do that, we have to set BLA_SIZEOF_INTEGER=8 or BLA_SIZEOF_INTEGER=4, respectively, in CMake so find_package(BLAS) selects the correct one (we don't do that at the moment ... we should fix that).

I was forming a plan to handle that split, and BLA_SIZEOF_INTEGER with FindBLAS was part of it. That's reassuring to me!

Regarding BUILD_INDEX64_EXT_API, we probably should tell whoever builds the release packages for Debian, to build with -DBUILD_INDEX64_EXT_API=ON for the next release. My guess is that they use the Makefiles, which don't support the extended API.

Quite right! I'd looked at their debian/rules (https://salsa.debian.org/science-team/lapack/-/blob/master/debian/rules) but not at that point realised the import of the make.inc-32. They're not using CMake and almost certainly should. I've already emailed with the main packagers for it, and they seem responsive. I've just checked and you're quite right that their libblas64 symbols are not suffixed; the only build difference is the Fortran compile flags add -fdefault-integer-8, and the C flags get -DLAPACK_ILP64 (very similar effect on C side).

My thinking is the Debian packaging should switch to using CMake (I now know how to do that pretty painlessly, having figured it out with FlexiBLAS), and their 64-bit libraries should start having the suffixed names too. That would allow applications that needed to have both 32- and 64-bit versions available to work, so long as they put the 32-bit version first in the link line so the 32-bit version of un-suffixed stuff got picked up. Thoughts?

By the way, as I write this, those CI jobs have been "queued" for 5 hours 41 mins (thanks whoever started them, that's awesome). Might be time to cancel and restart them? EDIT: Actually it looks like it's just because rmlarsen has rebased all his PRs, so that might take a while!

[ACSimon33]

Quite right! I'd looked at their debian/rules (https://salsa.debian.org/science-team/lapack/-/blob/master/debian/rules) but not at that point realised the import of the make.inc-32. They're not using CMake and almost certainly should. I've already emailed with the main packagers for it, and they seem responsive.

Nice, thanks for the link. We should probably coordinate to do the 3.13 release (#1210) when they are ready to switch to CMake.

My thinking is the Debian packaging should switch to using CMake (I now know how to do that pretty painlessly, having figured it out with FlexiBLAS), and their 64-bit libraries should start having the suffixed names too. That would allow applications that needed to have both 32- and 64-bit versions available to work, so long as they put the 32-bit version first in the link line so the 32-bit version of un-suffixed stuff got picked up. Thoughts?

I don't understand exactly why one would need both libraries on the link line? Both libraries will contain both the standard and extended APIs, the only difference is that in the libblas64 library the standard API also uses 64bit integers. Basically, libblas64 will become obsolete and should only used for older code that expects 64bit integers in the standard API.

[ACSimon33]

We should expect test failures when building against an optimized BLAS, so we should disable the fatal errors in the pipeline. Three options:

1. Remove the test step entirely, and build with -D BUILD_TESTING:BOOL=OFF
2. Just remove the summary from the tests, i.e. run ctest with -E LAPACK_Test_Summary or build with -D LAPACK_TESTING_USE_PYTHON:BOOL=OFF (same effect)
3. Add a new CMake option to disable the fatal test errors. For example LAPACK_FAIL_ON_ERROR, which is on by default and adds the "--fail-on-error" flag to the LAPACK_Test_Summary call if it's ON.

[mohawk2]

We should expect test failures when building against an optimized BLAS, so we should disable the fatal errors in the pipeline. Three options:

1. Remove the test step entirely, and build with -D BUILD_TESTING:BOOL=OFF
2. Just remove the summary from the tests, i.e. run ctest with -E LAPACK_Test_Summary or build with -D LAPACK_TESTING_USE_PYTHON:BOOL=OFF (same effect)
3. Add a new CMake option to disable the fatal test errors. For example LAPACK_FAIL_ON_ERROR, which is on by default and adds the "--fail-on-error" flag to the LAPACK_Test_Summary call if it's ON.

It makes sense what you're saying about a "normal" optimised BLAS. My approach here is to definitely run tests, and they shouldn't fail because the "optimised" BLAS is just the Debian-packaged reference one, which even if it's behind, shouldn't fail. My approach very much includes failing, so the additional config wouldn't be used here.

Without having fully checked, my belief is the packaged BLAS "should" only fail because of xerbla problems, because Debian might not be enabling weak symbols right. If that belief is right, then it would make sense to park this PR until that's merged, and then consider fine-turning the tests so the xerbla-needing ones aren't included when SET_BLAS_XERBLA isn't available. The good news on that fine-tuning is the latest commit on #1407 implements such a probe (all 4-ish lines of it), so that is an easily implement-able thing now.

Edit: I see what you mean about the "test summary" (and it's not XERBLA related). I'd regard any numerical error at all as a problem given it's reference BLAS. I'm very new to this numerical computing (though I've done probably too much other programming), do you have insights into what's causing that?

[mohawk2]

Quite right! I'd looked at their debian/rules (https://salsa.debian.org/science-team/lapack/-/blob/master/debian/rules) but not at that point realised the import of the make.inc-32. They're not using CMake and almost certainly should. I've already emailed with the main packagers for it, and they seem responsive.

Nice, thanks for the link. We should probably coordinate to do the 3.13 release (#1210) when they are ready to switch to CMake.

My feeling is there's no reason for them to wait; I believe they "should" be using the CMake build already, including because it's cleaner and has the extended API as an option.

My thinking is the Debian packaging should switch to using CMake (I now know how to do that pretty painlessly, having figured it out with FlexiBLAS), and their 64-bit libraries should start having the suffixed names too. That would allow applications that needed to have both 32- and 64-bit versions available to work, so long as they put the 32-bit version first in the link line so the 32-bit version of un-suffixed stuff got picked up. Thoughts?

I don't understand exactly why one would need both libraries on the link line? Both libraries will contain both the standard and extended APIs, the only difference is that in the libblas64 library the standard API also uses 64bit integers. Basically, libblas64 will become obsolete and should only used for older code that expects 64bit integers in the standard API.

The way I see it, the current state of affairs, which they won't accept breaking, but we can surely add to (and by the way, I think you might be right that blas64 might become obsolete as a separate thing); I'll just use dgemm to keep it concise:

Debian package	Symbol	Size of integer params
libblas3	dgemm	4
libblas64-3	dgemm	8

The proposed new scheme:

Debian package	Symbol	Size of integer params
libblas3	dgemm	4
libblas3	dgemm_64	8
libblas64-3	dgemm	8
libblas64-3	dgemm_64	8

The reason I mention having both is just that they might not wish to add that second line in the nominally 32-bit library. I shall email and ask (and loop you in)!

[ACSimon33]

It makes sense what you're saying about a "normal" optimised BLAS. My approach here is to definitely run tests, and they shouldn't fail because the "optimised" BLAS is just the Debian-packaged reference one, which even if it's behind, shouldn't fail. My approach very much includes failing, so the additional config wouldn't be used here.

Without having fully checked, my belief is the packaged BLAS "should" only fail because of xerbla problems, because Debian might not be enabling weak symbols right. If that belief is right, then it would make sense to park this PR until that's merged, and then consider fine-turning the tests so the xerbla-needing ones aren't included when SET_BLAS_XERBLA isn't available. The good news on that fine-tuning is the latest commit on #1407 implements such a probe (all 4-ish lines of it), so that is an easily implement-able thing now.

Edit: I see what you mean about the "test summary" (and it's not XERBLA related). I'd regard any numerical error at all as a problem given it's reference BLAS. I'm very new to this numerical computing (though I've done probably too much other programming), do you have insights into what's causing that?

Not sure yet what causes the errors, but I'll investigate. The problem is that if reference BLAS had fixes between the last release and the current master, the tests might fail with the current test suite. Also, the reference BLAS would probably be compiled with a different compiler version, which can also have an effect on the results (different vectorization, contractions, etc.), which might change the LAPACK results enough that some tests reach the error threshold.

I think the most important thing this CI job should check is whether LAPACK builds with an optimized BLAS. We could even add jobs that build with MKL, AOCL, OpenBLAS, etc.

[mohawk2]

I think you're right on the scope: it's adding value just to check for building against other BLASes; the XERBLA stuff failed to link (i.e. build stage) before I added the probe, so this would catch that and other similar things, thereby (I hope) adding value. I've rebased against current master and removed the test phase.

One approach that might work for the testing failure might be a CMake parameter to force a particular precision, which could be made a bit less sensitive for this test.

This also raises the issue I brought up in #1420; master LAPACK won't currently build against BLAS without dgemmtr, which includes Apple Accelerate. That's new in 3.12.1 (and Apple's NEW_LAPACK stuff takes them to 3.12.0). What should be done about that?