improvement: BLAS support on all CI configurations on Github workflows

[luau-project]

What?

I have improved BLAS building/testing on all supported CI configurations on Github workflows (Ubuntu, MSYS2, MSVC).

Why?

It eases the process to check if future changes to the code is somewhat damaging the current state for BLAS support.

How?

• On Ubuntu, I simplified the building/testing with BLAS by installing the library libblas-dev;
• On MSYS2, there are builtin packages named mingw-w64-x86_64-blas, mingw-w64-ucrt-x86_64-blas and mingw-w64-clang-x86_64-blas for every MSYS2 environment (mingw64, ucrt64, clang64), respectively. So, I'm just installing it and building/testing against it;
• On MSVC, the most challenging changes, I had to do a major rework to support BLAS:
  1. First, since BLAS is a Fortran library, and there's not a "simple" manner to install a Fortran compiler which generates MSVC ABI-compatible binaries, I had to install LLVM flang-new https://flang.llvm.org/docs/ Fortran compiler from conda-forge channel using miniconda https://docs.anaconda.com/miniconda/;
  2. Then I download BLAS from its repository https://github.com/Reference-LAPACK/lapack and build the library with LLVM flang-new;
  3. Then I'm uploading the built BLAS library as artifact;
  4. All these steps from (i) to (iii) were done in a separate job, called build-BLAS;
  5. The main cminpack testing job (cminpack-visual-studio) downloads the built BLAS library from the build-BLAS job execution and performs the building / testing of cminpack.

Note

In the MSVC CI rework, I had to remove the building/testing of x86 binaries.

[devernay]

Honestly, I don't think we need to add a dimension to the matrix. Testing one configuration with BLAS is enough to me. This is why I did it that way for Ubuntu, and I would vote to do the same for Windows.

[luau-project]

It could be done / reverted to test a single combination of matrix configs. In my view, the chosen configuration could be the most common among users, or package maintainers (e.g.: shared library in Release mode).

My point in testing all configurations is that they are almost equivalent in terms of execution time (they finish almost all in the same time). So, I thought: why not? I understand that it could be a problem or unnecessary in the long run. It is just a matter of choice, really.

[devernay]

This is the configuration that is currently tested is the following. I'm ok with changing RelWithDebInfo to Release

          - BUILD_SHARED_LIBS: 'ON'
            CMAKE_BUILD_TYPE: 'RelWithDebInfo'
            CMINPACK_PRECISION: 'd'
            USE_BLAS: 'ON'

Why not test all matrix configurations? It may be free, but it has some impact. Because building and testing requires energy, which may be non-renewable, and I still care about this planet.

[devernay]

what is wrong with openblas? most - if not all - users would prefer openblas over the Fortran BLAS. Fortran BLAS is not optimized for modern CPU architectures

[devernay]

It's OK to exclude one test for OpenBLAS, until someone contributes a better testing mechanism. Maybe Fortran BLAS passes the tests, but it's not used anymore

[devernay]

why not openblas? Who would use the reference BLAS implementation? the following figure is from Wittwer 2008, and OpenBLAS is based on GotoBLAS.

[luau-project]

In regards of BLAS comments, to be honest, I don't have any experience with it and I'm unable to say anything about each implementation. I think it should not be too hard to rewrite such workflows logic to test for OpenBLAS.

When I saw this exclude regex in the tests, it sounded strange to me, and I thought it was a library-specific test fail such that other libraries could solve. I mean, when I'm learning how to build / test a library, I often check their workflows, in case they exist. The testing for openblas with such regex is not natural for users. Most CMake users expect a ctest --test-dir <dir> -C <config> as default testing for a build configuration.

In my view, such exclude tlmdifc pattern for OpenBLAS should be inside the test manager file https://github.com/devernay/cminpack/blob/master/examples/CMakeLists.txt, hidden from the users expectation.

[devernay]

I thought it was a library-specific test fail such that other libraries could solve.

No, it's just that the way unit tests are written in this package is not the best one. See:

• https://github.com/cbouilla/minpack-1.1?tab=readme-ov-file#testing-minpack
• Are all tests expected to pass? #37
• C/C++ MINPACK runs the full test suite, but this one is better. cbouilla/minpack-1.1#1

[devernay]

My main comment is that if testing has to be done, it shouldn't be against the reference BLAS implementation, which is basically the same as the native minpack code, but against a modern implementation. I think OpenBLAS fits that purpose, as it's 10x faster than the reference implementation. It's also available for any platform, including MSVC (see https://vcpkg.link/ports/openblas).

Then, I don't think we should add a new dimension to the testing matrix for every feature we want to test. Only one configuration has to be tested with BLAS support. This is what I did for Linux, please do the same for Windows. The same would hold if/when we add LAPACK support to the cmake builds (it's easy, the code is there already)

[luau-project]

Update

I have followed your advices:

• building BLAS only for a single matrix config;
• using OpenBLAS implementation for BLAS on all supported CI configurations (Ubuntu, MSYS2, MSVC).

Note

Since you voted for a compact build matrix, I took the choice to narrow the matrix for the most common build types on every workflow (Release, Debug).

[luau-project]

On a test branch (not committed here), I was able to also build / test cminpack (+ OpenBLAS) with clang-cl, which is clang with ABI-compatibility to MSVC toolchain. IMO, it is a nice addition to the MSVC CI.

Would you like to add it?