BLD: Refactor BLAS/LAPACK wrapper infrastructure#20232
Conversation
h-vetinari
left a comment
There was a problem hiding this comment.
Thanks for breaking this out! :)
I'm not familiar with the code in the *.pyf.src files, so it would be good if someone else could check that (though it looks sensible to me). Other than that, I'm happy with this, aside from some minor nits.
0fd1b3b to
c9e6989
Compare
|
I'd like to branch for SciPy |
|
@tylerjereddy Personally, I think this is ready to go, though it would be nice to have one more reviewer. |
|
@thalassemia, could you please rebase on main? I don't want to squash this PR as the commits are very distinct, but I also don't want to add spurious merge commits. |
Generates incorrect `_cpointer` attrubute. See comment on scipygh-19816. [skip circle]
fd164e6 to
12ac2c4
Compare
|
I'll try to have a look at this tomorrow. |
There was a problem hiding this comment.
Thank you for breaking this out of #19816!
The changes LGTM (modulo those in *.pyf.src, which I don't feel qualified to review).
@rgommers This PR is a consequence of a review I had in the other PR (building upon yet another previous review); here we refactor the existing infrastructure based on a setup that allows easy extension for the 13.3+ enablement, without duplicating the things found in _wrappers_common.py.
That said, this is still +/- the first complete iteration that @thalassemia and I came up with, so it's possible you'd like to change some things.
|
I can't spare time from work too much these days. Hence did not look very deep into things. However I can't see how they can cause any issues either hence looks good to me. With little time I have, testing ARPACK/PROPACK rewrite anyways, so soon we can remove these dummy wrappers completely. But probably will happen in 1.14 the earliest. |
rgommers
left a comment
There was a problem hiding this comment.
This looks quite good, thanks a lot @thalassemia. And thanks for the reviews @h-vetinari.
I agree that this looks ready to go. I did some local testing, including with building scikit-learn on top (because our Cython API is very poorly tested by our own test suite). All looked good, not a single build warning or test failure. And changing to C wrappers will likely have multiple benefits down the line.
| 'cython_lapack_signatures.txt', | ||
| ], | ||
| # Set PYTHONPATH to import from _build_utils/_wrappers_common.py | ||
| env: {'PYTHONPATH': meson.global_source_root() / 'scipy/_build_utils'}, |
There was a problem hiding this comment.
This I wasn't 100% sure about; should be fine but in case it gives an issue later we can avoid the problem by importing _wrappers_common with importlib like so: https://github.com/numpy/numpy/blob/64676baf423c3e1c53b37ae413c3ca6aaecc642a/numpy/_core/code_generators/genapi.py#L27-L32
This was introduced last month in scipygh-20232; I commented on it there about it maybe being able to cause an issue later. And it just did, see scipygh-20433. Upgrading Meson in an existing build environment is technically unsupported by Meson, but has always "just worked". Now we hit a corner case on Windows, because when we set an environment variable in a `meson.build` file, a pickled version of Meson is called and this is less robust to upgrading Meson in the build env. The fix is a bit more verbose, but should be reliable - the `importlib` importing is directly copied from how it's done in NumPy's code generation scripts.
This was introduced last month in scipygh-20232; I commented on it there about it maybe being able to cause an issue later. And it just did, see scipygh-20433. Upgrading Meson in an existing build environment is technically unsupported by Meson, but has always "just worked". Now we hit a corner case on Windows, because when we set an environment variable in a `meson.build` file, a pickled version of Meson is called and this is less robust to upgrading Meson in the build env. The fix is a bit more verbose, but should be reliable - the `importlib` importing is directly copied from how it's done in NumPy's code generation scripts.
Reference issue
Based on the discussion in gh-19816.
What does this implement/fix?
I refactored
scipy/linalg/_generate_pyx.pyfor better readability and flow. To minimize code duplication in gh-19816, I moved the code shared between this file and that PR into a separate file (scipy/_build_utils/_wrappers_common.py) for both to import.Before this PR,
_generate_pyx.pygenerated Fortran subroutine wrappers over complex-valued BLAS/LAPACK functions (see gh-19855). This PR cuts that logic (and Fortran) out by adding equivalent C wrappers toscipy/_build_utils/src/wrap_g77_abi.candwrap_dummy_g77.abi.c.An additional motivation for these hard-coded C wrappers comes from
scipy.linalg._fblasand_flapack, which both currently rely on F2PY-generated Fortran subroutine wrappers. Specifically, in generating these wrappers, F2PY also adds some incorrect code that will cause problems down the line.To avoid this, I refactored the complex-valued functions in
scipy/linalg/fblas_l1.pyf.srcinto subroutines. This stops F2PY from generating both the problematic code and the Fortran wrappers. I replaced these wrappers with my newly added C wrappers by editing thefortrannamestatements for the relevant functions.While I was at it, I added
intent(c)statements to the other functions infblas_l1.pyf.srcandflapack_other.pyf.src, modifyingcallstatementandcallprotoargumentappropriately. This eliminated Fortran from the code generated by F2PY, leaving pure C.