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

Scipy 1.9.1 #3037

Closed
wants to merge 3 commits into from
Closed

Scipy 1.9.1 #3037

wants to merge 3 commits into from

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Aug 30, 2022

WIP. This seems like it will be quite hard. Meson detects the fact that an Emscripten compiler is being used and wants to be told that it is cross compiling. It might be possible to figure out (e.g., from the arguments) that meson is trying to detect the compiler and lie about it, but the better way is to use a proper cross compiling toolchain. Unfortunately, mesonpy does not allow cross compilation or really any modification of the meson parameters at all so I forked it and inserted our cross toolchain:
hoodmane/meson-python@ff68f2a

Currently the error seems to be something about pkg-config. I get an error message like:

A full log can be found at /src/packages/scipy/build/scipy-1.9.1/.mesonpy-udi3w61t/build/meson-logs/meson-log.txt

Aggravatingly, this log file does not actually exist. Probably mesonpy is deleting it, and I should remove that deletion in my fork.

I had exe_wrapper = 'node' in the toolchain but that led to it detecting that our fortran compiler doesn't work. I am not sure why exactly.

Currently I'm stuck with blas/lapack detection. This part might be too hard for me to fix so I'll probably have to hope that @ryanking13 decides to look into it at some point.

Alternatively maybe we can just stay on scipy 1.8 until we get a working Fortran compiler.

@rgommers @tylerjereddy

@hoodmane hoodmane marked this pull request as draft August 30, 2022 05:52
@rgommers
Copy link
Contributor

Unfortunately, mesonpy does not allow cross compilation or really any modification of the meson parameters at all so I forked it and inserted our cross toolchain:

We have a cross-compilation job for macOS arm64 in CI, that may be a useful reference: https://github.com/scipy/scipy/blob/main/.github/workflows/wheels.yml#L172-L181. The way it uses meson setup --cross-file followed by pip wheel should work here too I think (and that does go through meson-python).

Aggravatingly, this log file does not actually exist. Probably mesonpy is deleting it, and I should remove that deletion in my fork.

This shouldn't depend on meson-python, it's the standard way pip and build work. If you use pip, add --no-clean to keep build dirs. For build, IIRC using --outdir preserves all build output.

Currently I'm stuck with blas/lapack detection.

Note that you need pkg-config or cmake. Meson will try both and ask for OpenBLAS (by default) or another BLAS/LAPACK library that you can select like so: http://scipy.github.io/devdocs/dev/contributor/meson_advanced.html#select-a-different-blas-or-lapack-library. Does one of those options work for you?

Alternatively maybe we can just stay on scipy 1.8 until we get a working Fortran compiler.

Note that SciPy 1.9.x ships with two build systems. If you want to avoid this topic for one more release, you can add a one line patch to delete build-backend = 'mesonpy' from pyproject.toml, and the build will be done with numpy.distutils.

@ryanking13
Copy link
Member

Currently I'm stuck with blas/lapack detection. This part might be too hard for me to fix so I'll probably have to hope that @ryanking13 decides to look into it at some point.

I can help, but I've never look into scipy build before and I don't know much about blas things so it might take some time for me to learn how scipy build and detect libraries.

@rgommers
Copy link
Contributor

I should be able to help with answering questions related to that. BLAS and LAPACK detection is implemented here: https://github.com/scipy/scipy/blob/cc63003326b7ae2e9dd41cd6af0e6e3079c32b95/scipy/meson.build#L124-L142

Not sure if you use openblas in Pyodide, or Netlib BLAS/LAPACK? If the latter, see the link above to add flags like -Dblas=blas for selecting plain blas and lapack.

Meson first tries pkg-config. The way this should work is described under "Current support" at https://www.freedesktop.org/wiki/Software/pkg-config/CrossCompileProposal/. If the BLAS/LAPACK you ship now does not have any .pc files, it should be quite easy to add them (they're only a few lines long). You can just look at what conda-forge or a Linux distro ships, and adapt paths from there.

@hoodmane
Copy link
Member Author

Thanks for the tips @rgommers. I think I will try to start by using the old build backend to make sure the rest of the package is working and then switch build backends in a separate pr.

@hoodmane
Copy link
Member Author

Okay, update with no meson builds successfully #3043 though I haven't tried to run the full scipy test suite.

@eli-schwartz
Copy link

https://github.com/pyodide/pyodide/blob/main/packages/scipy/patches/0005-disable-blas-detection.patch
https://github.com/pyodide/pyodide/blob/main/packages/scipy/patches/0012-Disable-lapack-detection.patch

It's interesting that you're currently disabling "detection" of blas/lapack for the numpy.distutils route. How does it make use of that instead?

I recall that emscripten ships with a couple pkg-config files itself... https://github.com/emscripten-core/emscripten/tree/main/system/lib/pkgconfig

These are actually stubs, there to solemnly swear that it will absolutely totally work if you try compiling software against these packages, without actually passing any flags (or just passing -sUSE_*).

If there's something similar you need to do for blas (? I know nothing about emscripten) you can try writing your own pkg-config files and installing them in your blas/lapack package.

...

Hmm. As you're apparently using Netlib CLAPACK and cp'ing artifacts around instead of using a Makefile installation rule (which CLAPACK does not appear to have), I guess it is not so surprising that you don't have a pkg-config file for it. It shouldn't be too complicated to write one, anyway.

@hoodmane
Copy link
Member Author

@eli-schwartz Does numpy.distutils even use pkg-config? I can't see any evidence that it does.

@rgommers
Copy link
Contributor

It's interesting that you're currently disabling "detection" of blas/lapack for the numpy.distutils route. How does it make use of that instead?

NumPy contains a fallback called lapack_lite, which is basically just f2c'd BLAS and LAPACK routines that are needed for the build. Bad performance, but it works. That's only for NumPy itself, and cannot be used by SciPy.

Also see numpy/numpy#21895 (comment) for more details on how it works in Pyodide right now.

@eli-schwartz Does numpy.distutils even use pkg-config? I can't see any evidence that it does.

It doesn't indeed, it just looks for libraries in a bunch of hardcoded paths. Which is nice/easy for the simple cases, and horrible for (e.g.) cross-compiling.

@hoodmane
Copy link
Member Author

Bad performance, but it works.

Presumably the bad performance you refer to is because it doesn't use anything specific hardware features like vectorization or put acceleration? "Bad performance, but it works" is kind of wasm's motto...

Passing the path to clapack_all.so to the scipy loader does have the benefit that we don't need to load it with RTLD_GLOBAL.

@rgommers
Copy link
Contributor

Presumably the bad performance you refer to is because it doesn't use anything specific hardware features like vectorization or put acceleration?

Mostly, yes. There are a few algorithmic improvements in libraries as well, but that's more rare.

"Bad performance, but it works" is kind of wasm's motto...

:)

@lesteve
Copy link
Contributor

lesteve commented Nov 14, 2022

I haven't tried to run the full scipy test suite.

FYI I started a repo https://github.com/lesteve/scipy-tests-pyodide to run the scipy tests and get an idea of which modules are problematic.

There are plenty of Pyodide fatal errors that need to be debugged (in the modules fft, linalg, signal, sparse, spatial, stats).

I am guessing that some of them are similar to #3203.

@rth
Copy link
Member

rth commented Nov 14, 2022

Thanks a lot @lesteve !

@hoodmane
Copy link
Member Author

Yeah I've been gradually fixing the fatal errors but they are quite time consuming. Most are simple signature mismatches and I pretty much have a technique for these. For the ones that corrupt memory I'm at a total loss.

One thing I noticed is that even just attempting to statically link the package the compiler does extra static analysis and reports a lot of errors. So even if we can't get it to actually run correctly without a bunch of work trying the first step might provide info on all the remaining signature mismatches all at once. Less sure about the memory corruption, the memory corruption is extremely cursed.

@hoodmane
Copy link
Member Author

Really appreciate you looking into these things @lesteve and the test repo looks great. I think we could really benefit from a more systematic approach here.

@rgommers
Copy link
Contributor

So even if we can't get it to actually run correctly without a bunch of work trying the first step might provide info on all the remaining signature mismatches all at once.

One thing I happened to notice when looking at scipy/scipy#16098 is that there was overlap between that issue and the ones that affect Pyodide (e.g., the *_IIR_forback issue you fixed in scipy#15955 also shows up in the -flto build log). So injecting a single global build flag like -Wlto-type-mismatch may be enough to get the list. The Meson build is clean of warnings with GCC on Linux now, so the list of warnings you get with that would probably be complete.

@hoodmane hoodmane closed this Mar 1, 2023
@hoodmane hoodmane deleted the scipy-1.9 branch March 1, 2023 17:24
@rth rth mentioned this pull request Mar 10, 2023
@lesteve lesteve mentioned this pull request May 24, 2023
2 tasks
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.

None yet

6 participants