Package OpenBLAS and use OpenBLAS in scipy

[lesteve]

Description

Opening a draft PR on this.

This could potentially get rid of some of the low-level issues like #3203.

Also OpenBLAS is more maintained than CLAPACK.

Related to #227.

Checklists

• Add a CHANGELOG entry

[lesteve]

So I have the same error locally as in https://circleci.com/gh/pyodide/pyodide/62097

make: *** No rule to make target 'Makefile.'

Note the . at the end. For now this is what I have discovered:
In OpenBLAS, Makefile.conf is auto-generated in Makefile.system by this line:

# Generating Makefile.conf and config.h
DUMMY := $(shell $(MAKE) -C $(TOPDIR) -f Makefile.prebuild CC="$(CC)" FC="$(FC)" HOSTCC="$(HOSTCC)" HOST_CFLAGS="$(GETARCH_FLAGS)" CFLAGS="$(CFLAGS)" BINARY=$(BINARY) USE_OPENMP=$(USE_OPENMP) DYNAMIC_ARCH=$(DYNAMIC_ARCH) TARGET_CORE=$(TARGET_CORE) ONLY_CBLAS=$(ONLY_CBLAS) TARGET=$(TARGET) all)

And somehow inside the Pyodide docker, the generated Makefile.conf has ARCH= and Makefile.system includes Makefile.$(ARCH) so Makefile. (with the . at the end) which does not exist.

I managed to build OpenBLAS on my machine not sure what is different compared to the Pyodide docker image.

A mystery that needs to be investigated a bit further ...

[lesteve]

For now I will use a recent commit on the OpenBLAS develop branch where the previous error does not show up.

[rth]

Thanks for working on this! So it looks like OpenBLAS is building OK. Next step could be to remove CLAPACK (also to make sure it's not used) and link to OpenBLAS instead in scipy.

Maybe we should build it with -Os instead of -O2 as currently, it produces a zip of 2.8MB while CLAPACK was 1.24MB

[rth]

I would have guessed yes, but you can also probably not include it and see if there are any related include errors as done now.

[lesteve]

There are some errors when linking scipy with OpenBLAS:

wasm-ld: error: function signature mismatch: daxpy_
>>> defined as (i32, i32, i32, i32, i32, i32) -> i32 in build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a(d_odr.o)
>>> defined as (i32, i32, i32, i32, i32, i32) -> void in /src/packages/.libs/lib/libopenblas.so

wasm-ld: error: function signature mismatch: dswap_
>>> defined as (i32, i32, i32, i32, i32) -> i32 in build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a(d_lpk.o)
>>> defined as (i32, i32, i32, i32, i32) -> void in /src/packages/.libs/lib/libopenblas.so

wasm-ld: error: function signature mismatch: drot_
>>> defined as (i32, i32, i32, i32, i32, i32, i32) -> i32 in build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a(d_odr.o)
>>> defined as (i32, i32, i32, i32, i32, i32, i32) -> void in /src/packages/.libs/lib/libopenblas.so

wasm-ld: error: function signature mismatch: dcopy_
>>> defined as (i32, i32, i32, i32, i32) -> i32 in build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a(d_odr.o)
>>> defined as (i32, i32, i32, i32, i32) -> void in /src/packages/.libs/lib/libopenblas.so

wasm-ld: error: function signature mismatch: dscal_
>>> defined as (i32, i32, i32, i32) -> i32 in build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a(d_lpk.o)
>>> defined as (i32, i32, i32, i32) -> void in /src/packages/.libs/lib/libopenblas.so

wasm-ld: error: function signature mismatch: drotg_
>>> defined as (i32, i32, i32, i32) -> i32 in build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a(d_odr.o)
>>> defined as (i32, i32, i32, i32) -> void in /src/packages/.libs/lib/libopenblas.so
emcc: error: '/src/emsdk/emsdk/upstream/bin/wasm-ld -o build/lib.emscripten_3_1_27_wasm32-3.10/scipy/odr/__odrpack.cpython-310-wasm32-emscripten.so --fatal-warnings -L/src/packages/.artifacts/lib/python3.10/site-packages/numpy//core/lib/ -L/src/packages/.artifacts/lib/python3.10/site-packages/numpy//random/lib/ -L/src/cpython/installs/python-3.10.2/lib/ build/temp.emscripten_3_1_27_wasm32-3.10/scipy/odr/__odrpack.o -Lbuild/temp.emscripten_3_1_27_wasm32-3.10 build/temp.emscripten_3_1_27_wasm32-3.10/libodrpack.a /src/packages/.libs/lib/libopenblas.so -L/src/emsdk/emsdk/upstream/emscripten/cache/sysroot/lib/wasm32-emscripten/pic -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr --import-undefined --import-memory --strip-debug --export-if-defined=PyInit___odrpack --export-if-defined=__start_em_asm --export-if-defined=__stop_em_asm --export-if-defined=__start_em_lib_deps --export-if-defined=__stop_em_lib_deps --export-if-defined=__start_em_js --export-if-defined=__stop_em_js --export-if-defined=__wasm_apply_data_relocs --export=__wasm_call_ctors --experimental-pic -shared --no-export-dynamic' failed (returned 1)
error: Command "/tmp/tmp_tdnjvie/gfortran -Wall -g -Wall -g -shared build/temp.emscripten_3_1_27_wasm32-3.10/scipy/odr/__odrpack.o -Lbuild/temp.emscripten_3_1_27_wasm32-3.10 -lodrpack -lgfortran -o build/lib.emscripten_3_1_27_wasm32-3.10/scipy/odr/__odrpack.cpython-310-wasm32-emscripten.so -I/src/packages/.libs/include /src/packages/.libs/lib/libopenblas.so" failed with exit status 1

There are already some similar warning when building OpenBLAS, so maybe this can be fixed in OpenBLAS: OpenMathLib/OpenBLAS#3640 (comment)

[hoodmane]

We've been patching all void return value functions to return i32. This has something to do with alternative returns.

[ryanking13]

Until it gets fixed upstream, you can try replacing all void return values with int.

In suitesparse, the same problem happened, but the patching was not hard because the problematic functions had all same prefixes, not sure about this one though...

pyodide/packages/suitesparse/meta.yaml

Lines 20 to 22 in 7241496

	# Fix return type to avoid linker error
	grep -rl "void LAPACK" . | xargs sed -i 's/void LAPACK/int LAPACK/g'
	grep -rl "void BLAS" . | xargs sed -i 's/void BLAS/int BLAS/g'

[lesteve]

OK I'll try something like this, thanks!

Out of curiosity, the convention to always return int rather than void is a Pyodide convention (always returning void could have worked too) or there is some reason behind it?

[hoodmane]

It's because of a fortran feature called alternative returns.

[hoodmane]

https://docs.oracle.com/cd/E19957-01/805-4940/6j4m1u7qv/index.html

[lesteve]

So scipy builds now but I get errors like this locally for example when doing import scipy.linalg in the Pyodide console:

pyodide.JsException: Error: bad export type for `ztrsm_`: undefined maybe

They happens in CircleCI as well e.g. this build https://app.circleci.com/pipelines/github/pyodide/pyodide/5095/workflows/d2b521c2-9683-4bc0-a994-ffd7520610e2/jobs/62303:

E   pytest_pyodide.runner.JavascriptException: Error: bad export type for `ztrsm_`: undefined

E       at Object.reportUndefinedSymbols (/root/repo/dist/pyodide.asm.js:10:346898)

E       at loadPackage (/root/repo/dist/pyodide.asm.js:10:142844)

E       at async Object.loadPackagesFromImports (/root/repo/dist/pyodide.asm.js:10:156427)

E       at async evalmachine.<anonymous>:5:1

E       at async evalmachine.<anonymous>:4:26

E       at async evalCode (/usr/local/lib/python3.10/site-packages/pytest_pyodide/node_test_driver.js:81:33)

Suggestions more than welcome!

[ryanking13]

You need to add ${SIDE_MODULE_LDFLAGS} (which adds -s SIDE_MODULE flag) to LDFLAGS when calling make. Without it, the built .so file actually becomes a static_library. You can find the warning message in the build log of openblas:

emcc: warning: linking a library with `-shared` will emit a static object file.  This is a form of emulation to support existing build systems.  If you want to build a runtime shared library use the SIDE_MODULE setting. [-Wemcc]

[lesteve]

The CI failure seems to be because the HTTPS certificate has expired for https://mpfr.loria.fr/mpfr-4.1.0/mpfr-4.1.0.tar.xz, oh well ...

In case this does not get fixed soonish, we can get the source from https://gitlab.inria.fr/mpfr/mpfr/-/archive/4.1.0/mpfr-4.1.0.tar.gz, but we then need to sudo apt-get install autoconf-archive to be able to do ./autogen.sh to generate the configure file.

[lesteve]

I have marked this PR as ready to review.

Here are the few things that I am not too sure about:

• I removed -std=c++14 but there may be a cleaner way to do organise Makefile variables? change and context
• For now I renamed CLAPACK to libf2c, but we still download the full CLAPACK, this was easier to do it this way since we already have the necessary patches. Is it worth only downloading libf2c? It seems possible e.g. https://netlib.org/f2c/libf2c.zip but the Makefiles seems slightly different and new patches will need to be written.

About the CI failure and the problematic HTTPS certificate, it seems like the mpfr people know so hopefully that should be fixed soon. Another alternative is to use https://ftp.gnu.org/gnu/mpfr/, which is simpler than the gitlab.inria.fr I mentioned above.

[rth]

That's great, thanks a lot @lesteve !

For now I renamed CLAPACK to libf2c, but we still download the full CLAPACK, this was easier to do it this way since we already have the necessary patches.

I think it's fine this way, considering all the other things we download a few extra MB is not going to matter.

Personally this LGTM, I'll let Hood do the final review/approval.

[rth]

Unresolving as I think it still needs the above comment in the patch description.

[rth]

About the CI failure and the problematic HTTPS certificate,

That was fixed in #3749

[lesteve]

For completeness, the macOS test-core failure does not seem related to this PR. It seems to happen from time to time on main e.g. https://github.com/pyodide/pyodide/actions/runs/4663743467/jobs/8255711856.

[ryanking13]

For completeness, the macOS test-core failure does not seem related to this PR. It seems to happen from time to time on main e.g. https://github.com/pyodide/pyodide/actions/runs/4663743467/jobs/8255711856.

Yes, macOS CI is a bit flaky

[ryanking13]

Thanks for your hard work @lesteve! I haven't been following this conversation closely, so I'll also let Hood approve it.

My only concern is that CI-wise this PR adds a library that takes over 10 minutes to build when it's not cached. But we are trying to work on it (#3544, #3341), so hopefully we can find a way around it.

[hoodmane]

Okay merging. Thanks again @lesteve! Really valuable to have someone contributing stuff like this.

[lesteve]

Great to see this one merged!

Thanks a lot for the comments and the help, in particular the session in Paris helped a lot to see how you debug the kind of issues I was facing!

[rth]

@lesteve Were you planning of doing some benchmarks for say matrix multiplication before and after this change? You can take for instance 0.23.0 with CLAPACK and compare it with the dev deployment with OpenBLAS. I would expect for it to be a bit better but not massively better, at least until we optionally allow SIMD. So a next step could be to go and ask OpenBLAS people what could be done to make it better ^

Although to evaluate matrix multiplication we would probably need to use some low level scipy functions, since numpy still currently uses the built-in reference BLAS.

[lesteve]

@lesteve Were you planning of doing some benchmarks for say matrix multiplication before and after this change?

I have to say that was not on my radar at all 😉. The first thing I wanted to do was to look at all the scipy issues in Pyodide and check again whether they still happen. I am pretty sure some of them should have been fixed by switching to OpenBLAS.

Although to evaluate matrix multiplication we would probably need to use some low level scipy functions, since numpy still currently uses the built-in reference BLAS.

I remember asking you about using OpenBLAS for numpy as well, but I don't remember the answer I have to admit.

[rth]

That works. Opened #3763 about it. Maybe I'll have a look later.