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

py-scipy: add v1.9 #31810

Merged
merged 19 commits into from Nov 19, 2022
Merged

py-scipy: add v1.9 #31810

merged 19 commits into from Nov 19, 2022

Conversation

adamjstewart
Copy link
Member

@adamjstewart adamjstewart commented Jul 29, 2022

https://github.com/scipy/scipy/releases/tag/v1.9.0

Successfully builds on macOS 12.4 (Apple M1 Pro) with Python 3.9.13 and Apple Clang 13.1.6.

Scipy 1.9.0 switches from setuptools to meson. Pros: build times are down from ~6 min to ~2.5 min! Cons: I don't yet see a way to define which BLAS/LAPACK library is used. It seems meson uses pkg-config and picks up whichever BLAS/LAPACK it finds first. It's unclear what the behavior is when multiple BLAS/LAPACK libraries are installed on the system. @rgommers do you know of any way to force scipy to use the same BLAS/LAPACK as numpy? Are there any plans to port numpy to use meson too?

Depends on #31809

@rgommers
Copy link
Contributor

@rgommers do you know of any way to force scipy to use the same BLAS/LAPACK as numpy?

The SciPy build itself never provided such a thing, in an automated fashion at least. Spack does it with quite a bit of custom code:

with open('site.cfg', 'w') as f:
if '^intel-mkl' in spec or \
'^intel-parallel-studio+mkl' in spec or \
'^intel-oneapi-mkl' in spec:
f.write('[mkl]\n')
# FIXME: as of @1.11.2, numpy does not work with separately
# specified threading and interface layers. A workaround is a
# terribly bad idea to use mkl_rt. In this case Spack will no
# longer be able to guarantee that one and the same variant of
# Blas/Lapack (32/64bit, threaded/serial) is used within the
# DAG. This may lead to a lot of hard-to-debug segmentation
# faults on user's side. Users may also break working
# installation by (unconsciously) setting environment variable
# to switch between different interface and threading layers
# dynamically. From this perspective it is no different from
# throwing away RPATH's and using LD_LIBRARY_PATH throughout
# Spack.
f.write('libraries = {0}\n'.format('mkl_rt'))
write_library_dirs(f, lapackblas_lib_dirs)
f.write('include_dirs = {0}\n'.format(lapackblas_header_dirs))
if '^blis' in spec or \
'^amdblis' in spec:
f.write('[blis]\n')
f.write('libraries = {0}\n'.format(blas_lib_names))
write_library_dirs(f, blas_lib_dirs)
f.write('include_dirs = {0}\n'.format(blas_header_dirs))
if '^openblas' in spec:
f.write('[openblas]\n')
f.write('libraries = {0}\n'.format(lapackblas_lib_names))
write_library_dirs(f, lapackblas_lib_dirs)
f.write('include_dirs = {0}\n'.format(lapackblas_header_dirs))
if '^libflame' in spec or \
'^amdlibflame' in spec:
f.write('[flame]\n')
f.write('libraries = {0}\n'.format(lapack_lib_names))
write_library_dirs(f, lapack_lib_dirs)
f.write('include_dirs = {0}\n'.format(lapack_header_dirs))
if '^atlas' in spec:
f.write('[atlas]\n')
f.write('libraries = {0}\n'.format(lapackblas_lib_names))
write_library_dirs(f, lapackblas_lib_dirs)
f.write('include_dirs = {0}\n'.format(lapackblas_header_dirs))
if '^veclibfort' in spec:
f.write('[accelerate]\n')
f.write('libraries = {0}\n'.format(lapackblas_lib_names))
write_library_dirs(f, lapackblas_lib_dirs)
if '^netlib-lapack' in spec or \
'^cray-libsci' in spec:
# netlib and Cray require blas and lapack listed
# separately so that scipy can find them
if spec.satisfies('+blas'):
f.write('[blas]\n')
f.write('libraries = {0}\n'.format(blas_lib_names))
write_library_dirs(f, blas_lib_dirs)
f.write('include_dirs = {0}\n'.format(blas_header_dirs))
if spec.satisfies('+lapack'):
f.write('[lapack]\n')
f.write('libraries = {0}\n'.format(lapack_lib_names))
write_library_dirs(f, lapack_lib_dirs)
f.write('include_dirs = {0}\n'.format(lapack_header_dirs))
if '^fujitsu-ssl2' in spec:
if spec.satisfies('+blas'):
f.write('[blas]\n')
f.write('libraries = {0}\n'.format(spec['blas'].libs.names[0]))
write_library_dirs(f, blas_lib_dirs)
f.write('include_dirs = {0}\n'.format(blas_header_dirs))
f.write(
"extra_link_args = {0}\n".format(
self.spec["blas"].libs.ld_flags
)
)
if spec.satisfies('+lapack'):
f.write('[lapack]\n')
f.write('libraries = {0}\n'.format(spec['lapack'].libs.names[0]))
write_library_dirs(f, lapack_lib_dirs)
f.write('include_dirs = {0}\n'.format(lapack_header_dirs))
f.write(
"extra_link_args = {0}\n".format(
self.spec["lapack"].libs.ld_flags
)
)

I'm guessing we want a bit more custom code, that maps the BLAS/LAPACK determined from the py-numpy package to the CLI interface: http://scipy.github.io/devdocs/dev/contributor/meson_advanced.html#select-a-different-blas-or-lapack-library. From there, pkg-config should do the job.

Are there any plans to port numpy to use meson too?

Yes, there are. We need to get that done before Python 3.12 becomes a thing. Once distutils is gone from the stdlib, NumPy won't build anymore. We're a little bandwidth-constrained for NumPy (it's time to start looking for a full-time build & CI engineer), but hopefully getting that done over the next 6 months.

@adamjstewart
Copy link
Member Author

  1. Where can I find a list of all -Dblas=... and -Dlapack=... values accepted? So far I only see blas/blis/mkl/openblas and lapack/mkl/openblas
  2. The docs you link to mention how to pass this flag to the build for meson setup build and python -m build but not pip install. Would it be pip install --install-option=-Dblas=... --install-option=-Dlapack=...?

@rgommers
Copy link
Contributor

  1. Where can I find a list of all -Dblas=... and -Dlapack=... values accepted? So far I only see blas/blis/mkl/openblas and lapack/mkl/openblas

Those are the ones I have tested. I'd expect some hiccups with MKL, because of the multiple ways you can build against it, and IIRC the pkgconfig files it ships are not great.

Other than that, any valid name will work. If you have a libmyblas.so, you pass -Dblas=myblas. There's no special handling for certain names, in scipy/meson.build it simply gets translated to blas = dependency(myblas).

2. Would it be pip install --install-option=-Dblas=... --install-option=-Dlapack=...?

--install-option is for setup.py, it should be --config-settings. I haven't tried that before, need to test a bit. First tries indicate that these arguments are not passed on by pip, or not understood by meson-python.

@rgommers
Copy link
Contributor

This indeed doesn't work with pip/build, only with meson. It looks like that needs fixing in meson-python. If you want to get 1.9.0 packages out without delay, perhaps you should still build them with numpy.distutils - all that's needed I think is a one-line patch to remove build-backend = 'mesonpy' from pyproject.toml.

@adamjstewart
Copy link
Member Author

We can always use meson setup build if that's the recommended way to install from source.

@adamjstewart
Copy link
Member Author

--install-option is for setup.py, it should be --config-settings.

I don't think --config-settings is a real pip flag.

@eli-schwartz
Copy link

eli-schwartz commented Jul 31, 2022

We can always use meson setup build if that's the recommended way to install from source.

This will work fine except for the fact that Meson does not (yet) have dist-info generation, so you can install SciPy, but you cannot make importlib-metadata et al. aware that it is installed. (This will be added to Meson after a) the python 3.11 release with stdlib import tomllib, b) consensus is reached on how best to support older versions of python, as Meson is historically dependency-free.)

A technically gross hack is to mkdir the intended dist-info directory, and copy the PKG-INFO file in the sdist over (rename to METADATA to taste). You're "supposed" to have a WHEEL file too, but that may not actually be checked. egg-info didn't have them anyway. :D

@rgommers
Copy link
Contributor

I don't think --config-settings is a real pip flag.

It is, it's new in pip 22.1: pypa/pip#11059. Docs: https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-config-settings

There's this issue: https://github.com/FFY00/meson-python/issues/54. That may explain why it does not work.

This will work fine except for the fact that Meson does not (yet) have dist-info generation, so you can install SciPy, but you cannot make importlib-metadata et al. aware that it is installed

There's also a difference in default build options (like meson setup will build in debug mode by default, so binaries will end up being 3x larger).

I think this is the one working way to work around the problem right now:

$ meson setup build -Dblas=openblas -Dlapack=openblas --prefix=$PWD/build-install -Ddebug=false -Doptimization=2
$ python -m build --wheel -Cbuilddir=build --no-isolation --skip-dependency-check .
$ pip install dist/scipy*.whl

That should pick up the desired BLAS/LAPACK version, and build with the same debug and optimization settings as well as dependencies when simply doing pip install --no-isolation ..

I did test this and it works for me, but it of course looks a bit shaky.

@rgommers
Copy link
Contributor

This is a meson-python issue (or missing feature). Trying to fix that in https://github.com/FFY00/meson-python/pull/122.

@adamjstewart
Copy link
Member Author

Thanks for all of the ideas and links! Since Spack users are very concerned about proper BLAS/LAPACK linking, I think the appropriate path forward is:

  1. Merge this PR as is (use pkg-config and hope for the best) but install scipy 1.8 by default instead of 1.9
  2. Once https://github.com/FFY00/meson-python/pull/122 is merged and released, add dependencies on pip 22.1+ and meson-python 0.8.2?+, use --config-settings, and bump the default from 1.8 to 1.9

Does this sound appropriate? I would rather avoid anything too hacky since we'll be ripping it out later. The alternative is just to wait for https://github.com/FFY00/meson-python/pull/122 to be merged and released before moving forward with this PR. Not sure how many Spack users are desperate to start using scipy 1.9.

@rgommers
Copy link
Contributor

There's a few other issues coming up for 1.9.0 that I'm not sure will affect users:

  • Building for PyPy does not yet work
  • The scipy/*.pxd headers seem to be missing in the wheel (somehow no one complained yet though, and the test suite doesn't fail - but it should affect numba and the scikit-learn build).

Does this sound appropriate? I would rather avoid anything too hacky since we'll be ripping it out later. The alternative is just to wait for FFY00/meson-python#122 to be merged and released before moving forward with this PR. Not sure how many Spack users are desperate to start using scipy 1.9.

I'd prefer either waiting for a week or so (I imagine that that's when the fixes land), or building 1.9.0 with numpy.distutils still, to avoid introducing new known problems for users.

@adamjstewart adamjstewart force-pushed the packages/py-scipy branch 2 times, most recently from 28a53f8 to 621859a Compare August 2, 2022 18:09
@adamjstewart
Copy link
Member Author

It seems like https://github.com/FFY00/meson-python/pull/122 isn't going to be merged any time soon. I'm starting to think that we should use:

$ meson setup build -Dblas=openblas -Dlapack=openblas --prefix=$PWD/build-install -Ddebug=false -Doptimization=2
$ python -m build --wheel -Cbuilddir=build --no-isolation --skip-dependency-check .
$ pip install dist/scipy*.whl

to build while we wait for that. Can steps 2 and 3 be replaced with pip install .? Or does that try to rebuild step 1 and render our BLAS/LAPACK settings meaningless?

@rgommers
Copy link
Contributor

Yes, I think that is reasonable, it's only a couple of lines more and it does the job.

Can steps 2 and 3 be replaced with pip install .? Or does that try to rebuild step 1 and render our BLAS/LAPACK settings meaningless?

No, I don't think so, that will ignore step 1. Also, pip seems to have an issue with reusing a build dir, so step 2 should be done with build.

@adamjstewart adamjstewart marked this pull request as ready for review August 24, 2022 00:43
@adamjstewart
Copy link
Member Author

adamjstewart commented Aug 24, 2022

P.S. This builds properly for me now. Just want to figure out the correct syntax for the conflict and this should be ready to merge assuming CI passes.

@adamjstewart
Copy link
Member Author

@spackbot run pipeline

@spackbot-app
Copy link

spackbot-app bot commented Aug 24, 2022

I had a problem triggering the pipeline.

@adamjstewart
Copy link
Member Author

Ping @scottwittenburg @zackgalbreath, we're currently unable to reproduce the pipeline issues so it's difficult to debug this.

@rgommers you mentioned that you were working on improving MKL support, any progress on this?

@rgommers
Copy link
Contributor

@rgommers you mentioned that you were working on improving MKL support, any progress on this?

No work on my Meson PR in the last few weeks, but the meson-python PR to pass command-line arguments through pip/build just landed, so once 0.11.0 is released that will be easier.

@adamjstewart
Copy link
Member Author

Other users are clamoring for newer scipy (#33929) so let me see if I can just add a conflict for MKL for the time being and we can figure it out in the future...

@scottwittenburg
Copy link
Contributor

we're currently unable to reproduce the pipeline issues so it's difficult to debug this

Sorry about that, #33953 should fix spack ci reproduce-build. With that change I was able to reproduce the py-scipy error:

  >> 54    scipy/meson.build:131:0: ERROR: Dependency "mkl-dynamic-lp64-seq" not found, tried pkgconfig

@adamjstewart
Copy link
Member Author

Tests are finally passing. I say we merge this and figure out MKL support in a follow-up PR.

@rgommers
Copy link
Contributor

That sounds good to me! Thanks for the persistence @adamjstewart & everyone who helped out!

@rscohn2
Copy link
Member

rscohn2 commented Nov 16, 2022

Tests are finally passing. I say we merge this and figure out MKL support in a follow-up PR.

I agree, too. Since @scottwittenburg says reproducing will work now, I should be able to track down the problem.

Copy link
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are finally passing. I say we merge this and figure out MKL support in a follow-up PR.

👍

@alalazo alalazo merged commit 2f057d7 into spack:develop Nov 19, 2022
@adamjstewart adamjstewart deleted the packages/py-scipy branch November 19, 2022 15:04
amd-toolchain-support pushed a commit to amd-toolchain-support/spack that referenced this pull request Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants