Use pkg-config for OpenBLAS #1778
Conversation
|
Capturing logs from failing steps in Windows builds for further analysis on some other day (as I don't know log retention policies here): windows-blas (Release, Win32, ON, ON) (pull_request) Configurewindows-blas (Release, x64, ON, ON) (pull_request) ConfigurePassing builds from recent commit 1f50a7d have: (before this PR there were fewer things messaged related to OpenBLAS) which makes me wonder what, where and how is built for Windows, because AFAIK Maybe some tweaks in CI config / workflow steps or actions will be also necessary to clean things up. Hard to tell at this stage. |
|
Thanks for looking into this! I don't have access to Windows systems, so I won't be able to provide much help about the build process. But let me know if you hit any roadblocks along the way |
build.yml uses a downloaded openblas, which has libopenblas.dll.a. Our current build.yml doesn't use MSYS2, so our LIB_SUFFIX/PREFIX are different from those of openblas. Maybe we shouldn't use such PREFIX/SUFFIX.
|
|
Sure, @ggerganov, appreciated! @tamo, thank you for useful pointers and your PR (przemoc/pull/1) with suggested changes to my PR. Your changes seem to be overfit for particular environment and assumptions regarding OpenBLAS (similar sin that is present in whisper.cpp nowadays until my PR will improve that). I didn't have time yet to investigate Windows build process in whisper.cpp and OpenBLAS builds, but just wanted to reply now regardless. It's a shame if OpenBLAS really does use only MSYS2 and does not provide MSVC-based builds. Mixing MSVC and non-MSVC binaries is literally asking for trouble. It was true for msvcrt-based MinGW / MinGW-w64 and I suspect it's true for ucrt-based MinGW-w64. Well, at least for GCC, as CLANG actually aims for MSVC compatibility, so it's safer to mix MSVC and CLANG binaries on Windows. OpenBLAS's CI configuration does not necessarily reflect how assets added to releases are built, though. In workflows they build using both UCRT64 and CLANG64 (among other variants), but in each
(surprisingly I didn't find anywhere clear information that I provided above, had to dig into differences between What is also sad is that their variants do not differ by any suffix for actual library, which is called And those My PR is meant to make stuff work in native Windows (MSVC), MSYS2 environment, and *nix, at least to the extent possible by what CMake provides (versus make that by convention is meant for *nix-like environments only, so don't have to bother with MSVC). MSYS2 started working with my changes, but I broke MSVC builds due to not spending any time with them and not being familiar with existing whisper.cpp build flow for Windows. Regarding prefix, if everyone in Windows world builds OpenBLAS with Regarding suffix, they are different between MSVC, MSYS2 and *nix, so we cannot get rid of it. Regarding FindBLAS, I already looked into it back then, and left various comments regarding it in the code. IMHO CMake is not a good build solution, users are inclined to use its batteries, but they seem to be not well-maintained, and if you happen to have older CMake, which may have older versions of the modules, you're even more screwed. But it's unrealistic to expect anyone to support various CMake versions with varying degree of features their modules provide... After spending as little time as necessary for writing this comment and glancing at OpenBLAS
I will do some quick fixes to address those issues, but even if the build will successfully pass, please do not merge it yet, as I may need to think about it more whether we're achieving LGTM state where potential further tweaks can be done via separate PRs, or if some basic tweaks still need to be done before merging. |
|
Completely agreed. |
przemoc
force-pushed
the
use-pkg-config-for-openblas
branch
from
fa9dfc9
to
5b845f4
Compare
|
Rebased and resolved minor merge conflict. No other changes. Sorry for letting it rot for 2 months. To be honest, I was not pondering more on the PR during this timeframe. I do think it is a step up over what we currently have in whisper.cpp, so I would suggest to merge it unless there are some particular concerns. In the worst case it can be always improved later via another PR. |
|
Looks like GH checks are somehow stuck. |
That way building on *nix like environments (including MSYS2 on Windows) with WHISPER_OPENBLAS=1 works out of the box. Fix handling of WHISPER_OPENBLAS, so that empty value or 0 won't be misinterpreted by make as enabled. Mind that it's not intended to detect CMake false constants (OFF NO FALSE N). make is not CMake. By default OpenBLAS with 64-bit interface is used, but that can be changed with `WHISPER_OPENBLAS_INTERFACE64=0` if 32-bit one is desired. If OpenBLAS headers and library are respectively in include/ and lib/ subdirectories of given path, then you can specify it, e.g. `OPENBLAS_PATH=/usr/local/openblas`, and this will take precedence over any pkg-config file. If there is no pkg-config file (.pc) for OpenBLAS and OPENBLAS_PATH is empty, then headers are assumed to be in /usr/include/openblas and library as assumed to be called 'openblas64' (or 'openblas' if `WHISPER_OPENBLAS_INTERFACE64=0`). If different headers location should be used, then it can be done, e.g. `WHISPER_BLAS_CFLAGS=-I/usr/local/include/openblas`. If different library should be used, it can be specified, e.g. `WHISPER_BLAS_LIB=openblasp64` (pthreads version as seen on Fedora), or you can provide LDFLAGS needed to link with OpenBLAS directly: `WHISPER_BLAS_LDFLAGS="-L/usr/local/lib/openblas -lopenblas64"`. Current solution is flexible enough to handle most cases out there without needlessly hardcoding possible OpenBLAS installation details.
…ries needed by OpenBLAS
That way building on *nix like environments (including MSYS2 on Windows)
with -DWHISPER_OPENBLAS=ON should work out of the box as long as you
have CMake 3.25 or newer.
Make OPENBLAS_PATH environment variable supported not only on Windows.
It sets OpenBLAS include dir to ${OPENBLAS_PATH}/include and library to
${WHISPER_BLAS_LIB} (name without prefixes and suffixes) in
${OPENBLAS_PATH}/lib and avoids further package finding.
By default OpenBLAS with 64-bit interface is used (equivalent to setting
`-DWHISPER_BLAS_LIB=openblas64`), but that can be changed with
`-DWHISPER_OPENBLAS_INTERFACE64=OFF` (equivalent to setting
`-DWHISPER_BLAS_LIB=openblas`) if 32-bit one is desired.
Turn on BLA_STATIC for FindBLAS only when WHISPER_STATIC is enabled.
BLA_STATIC may not work as expected for pkg-config based operation.
Get rid of supporting BLAS_HOME environment variable. If OPENBLAS_PATH
is insufficient in your case, there is no pkg-config file to rely on,
then you can manually specify include dir, e.g.
`-DBLAS_INCLUDE_DIRS=/usr/local/include/openblas`, and library, e.g.
`-DBLAS_LIBRARIES=/usr/local/lib/libopenblas.so`.
OpenBLAS w/o INTERFACE64=1 vel USE_64BITINT=1 seems to be more common.
Most *nix like environments (including MSYS2 on Windows) have OpenBLAS packages that allow coexistence of OpenBLAS builds with 32-bit and 64-bit interface (w/o and w/ OPENBLAS_USE64BITINT defined) and they differ by not having or having "64" suffix in their library filenames. That's not the case for OpenBLAS prebuilt libraries for Windows.
przemoc
force-pushed
the
use-pkg-config-for-openblas
branch
from
5b845f4
to
b6ee38b
Compare
|
Rebased and resolved minor merge conflict. |
* make : use pkg-config for finding CFLAGS & LDFLAGS needed by OpenBLAS
That way building on *nix like environments (including MSYS2 on Windows)
with WHISPER_OPENBLAS=1 works out of the box.
Fix handling of WHISPER_OPENBLAS, so that empty value or 0 won't be
misinterpreted by make as enabled. Mind that it's not intended to
detect CMake false constants (OFF NO FALSE N). make is not CMake.
By default OpenBLAS with 64-bit interface is used, but that can be
changed with `WHISPER_OPENBLAS_INTERFACE64=0` if 32-bit one is desired.
If OpenBLAS headers and library are respectively in include/ and lib/
subdirectories of given path, then you can specify it, e.g.
`OPENBLAS_PATH=/usr/local/openblas`, and this will take precedence over
any pkg-config file.
If there is no pkg-config file (.pc) for OpenBLAS and OPENBLAS_PATH is
empty, then headers are assumed to be in /usr/include/openblas and
library as assumed to be called 'openblas64' (or 'openblas' if
`WHISPER_OPENBLAS_INTERFACE64=0`). If different headers location should
be used, then it can be done, e.g.
`WHISPER_BLAS_CFLAGS=-I/usr/local/include/openblas`.
If different library should be used, it can be specified, e.g.
`WHISPER_BLAS_LIB=openblasp64` (pthreads version as seen on Fedora), or
you can provide LDFLAGS needed to link with OpenBLAS directly:
`WHISPER_BLAS_LDFLAGS="-L/usr/local/lib/openblas -lopenblas64"`.
Current solution is flexible enough to handle most cases out there
without needlessly hardcoding possible OpenBLAS installation details.
* cmake : fix how pkg-config is used for finding include dirs and libraries needed by OpenBLAS
That way building on *nix like environments (including MSYS2 on Windows)
with -DWHISPER_OPENBLAS=ON should work out of the box as long as you
have CMake 3.25 or newer.
Make OPENBLAS_PATH environment variable supported not only on Windows.
It sets OpenBLAS include dir to ${OPENBLAS_PATH}/include and library to
${WHISPER_BLAS_LIB} (name without prefixes and suffixes) in
${OPENBLAS_PATH}/lib and avoids further package finding.
By default OpenBLAS with 64-bit interface is used (equivalent to setting
`-DWHISPER_BLAS_LIB=openblas64`), but that can be changed with
`-DWHISPER_OPENBLAS_INTERFACE64=OFF` (equivalent to setting
`-DWHISPER_BLAS_LIB=openblas`) if 32-bit one is desired.
Turn on BLA_STATIC for FindBLAS only when WHISPER_STATIC is enabled.
BLA_STATIC may not work as expected for pkg-config based operation.
Get rid of supporting BLAS_HOME environment variable. If OPENBLAS_PATH
is insufficient in your case, there is no pkg-config file to rely on,
then you can manually specify include dir, e.g.
`-DBLAS_INCLUDE_DIRS=/usr/local/include/openblas`, and library, e.g.
`-DBLAS_LIBRARIES=/usr/local/lib/libopenblas.so`.
* make / cmake : use OpenBLAS with 32-bit interface by default.
OpenBLAS w/o INTERFACE64=1 vel USE_64BITINT=1 seems to be more common.
* cmake : hardcode "lib" prefix for OpenBLAS lib filename (even on Windows)
* cmake : hardcode OpenBLAS library name when building in MSVC (Windows)
Most *nix like environments (including MSYS2 on Windows) have OpenBLAS
packages that allow coexistence of OpenBLAS builds with 32-bit and
64-bit interface (w/o and w/ OPENBLAS_USE64BITINT defined) and they
differ by not having or having "64" suffix in their library filenames.
That's not the case for OpenBLAS prebuilt libraries for Windows.
* make : use pkg-config for finding CFLAGS & LDFLAGS needed by OpenBLAS
That way building on *nix like environments (including MSYS2 on Windows)
with WHISPER_OPENBLAS=1 works out of the box.
Fix handling of WHISPER_OPENBLAS, so that empty value or 0 won't be
misinterpreted by make as enabled. Mind that it's not intended to
detect CMake false constants (OFF NO FALSE N). make is not CMake.
By default OpenBLAS with 64-bit interface is used, but that can be
changed with `WHISPER_OPENBLAS_INTERFACE64=0` if 32-bit one is desired.
If OpenBLAS headers and library are respectively in include/ and lib/
subdirectories of given path, then you can specify it, e.g.
`OPENBLAS_PATH=/usr/local/openblas`, and this will take precedence over
any pkg-config file.
If there is no pkg-config file (.pc) for OpenBLAS and OPENBLAS_PATH is
empty, then headers are assumed to be in /usr/include/openblas and
library as assumed to be called 'openblas64' (or 'openblas' if
`WHISPER_OPENBLAS_INTERFACE64=0`). If different headers location should
be used, then it can be done, e.g.
`WHISPER_BLAS_CFLAGS=-I/usr/local/include/openblas`.
If different library should be used, it can be specified, e.g.
`WHISPER_BLAS_LIB=openblasp64` (pthreads version as seen on Fedora), or
you can provide LDFLAGS needed to link with OpenBLAS directly:
`WHISPER_BLAS_LDFLAGS="-L/usr/local/lib/openblas -lopenblas64"`.
Current solution is flexible enough to handle most cases out there
without needlessly hardcoding possible OpenBLAS installation details.
* cmake : fix how pkg-config is used for finding include dirs and libraries needed by OpenBLAS
That way building on *nix like environments (including MSYS2 on Windows)
with -DWHISPER_OPENBLAS=ON should work out of the box as long as you
have CMake 3.25 or newer.
Make OPENBLAS_PATH environment variable supported not only on Windows.
It sets OpenBLAS include dir to ${OPENBLAS_PATH}/include and library to
${WHISPER_BLAS_LIB} (name without prefixes and suffixes) in
${OPENBLAS_PATH}/lib and avoids further package finding.
By default OpenBLAS with 64-bit interface is used (equivalent to setting
`-DWHISPER_BLAS_LIB=openblas64`), but that can be changed with
`-DWHISPER_OPENBLAS_INTERFACE64=OFF` (equivalent to setting
`-DWHISPER_BLAS_LIB=openblas`) if 32-bit one is desired.
Turn on BLA_STATIC for FindBLAS only when WHISPER_STATIC is enabled.
BLA_STATIC may not work as expected for pkg-config based operation.
Get rid of supporting BLAS_HOME environment variable. If OPENBLAS_PATH
is insufficient in your case, there is no pkg-config file to rely on,
then you can manually specify include dir, e.g.
`-DBLAS_INCLUDE_DIRS=/usr/local/include/openblas`, and library, e.g.
`-DBLAS_LIBRARIES=/usr/local/lib/libopenblas.so`.
* make / cmake : use OpenBLAS with 32-bit interface by default.
OpenBLAS w/o INTERFACE64=1 vel USE_64BITINT=1 seems to be more common.
* cmake : hardcode "lib" prefix for OpenBLAS lib filename (even on Windows)
* cmake : hardcode OpenBLAS library name when building in MSVC (Windows)
Most *nix like environments (including MSYS2 on Windows) have OpenBLAS
packages that allow coexistence of OpenBLAS builds with 32-bit and
64-bit interface (w/o and w/ OPENBLAS_USE64BITINT defined) and they
differ by not having or having "64" suffix in their library filenames.
That's not the case for OpenBLAS prebuilt libraries for Windows.
Information can be found in commit messages. There are various tweaks done along the way.
Makefile changes are simpler. CMakeLists.txt changes are a bit more complex.
This may not be perfect, but overall should be a noticeable improvement over what we had before regarding OpenBLAS handling.
OPENBLAS_PATHremains supported.BLAS_HOMEis no longer supported. I don't believe in kitchen sink approach to support any possible ad hoc conventions created out there. As long as we expose sufficient knobs to control build process, downstream can adapt appropriately.Most likely some CI checks will fail. Will try to fix it some other day. MacOS may blow up as usual.
(My CMake experience is small, and the more I learn about it, the less I like it. And I do not want to rewrite some of the batteries they provide...).
I will also appreciate some testing. I did limited testing in one of MSYS2 environments (UCRT64), Alpine Linux 3.18.5 (w/
WHISPER_OPENBLAS_INTERFACE64=0), Ubuntu 22.04.It's not clear to me if
WHISPER_OPENBLAS_INTERFACE64should be really enabled by default, I just followed existing intention expressed viaset(BLA_SIZEOF_INTEGER 8). I think having it disabled (using OpenBLAS w/ 32-bit interface) would be a better out of the box experience for most folks out there, but maybe I'm missing something?Another thing, that I definitely do not intend to address in this PR, is support for OpenBLAS built w/
SYMBOLSUFFIX=64_, which I have not yet researched how it should be dealt. That's also a reason why OpenBLAS w/ 32-bit interface is a "safer" choice by default.