MAINT: Allow scipy to be compiled in cython3 #18242
Conversation
|
Except replacing relative imports with absolute one, the rest of PR is just annotating functions as |
|
I can confirm that this builds (and I was going completely the wrong direction in trying to debug this!). |
| @@ -1 +1 @@ | |||
| from .linalg cimport cython_blas, cython_lapack | |||
| from scipy.linalg cimport cython_blas, cython_lapack | |||
There was a problem hiding this comment.
The stated preference in #17234 was to keep this relative, to avoid that this ever picks up some other installed scipy. I understand that this isn't possible (or at least bugfree) with 0.29.x, but wouldn't want to mix these changes.
There was a problem hiding this comment.
Let me just say that that's not critical - whatever works here to get SciPy to build. We can deal with the rest later.
There was a problem hiding this comment.
It's probably still better to have a single commit with the absolute import changes to allow rolling it back independently at some point. Although there will likely be a couple of conflicts by then, but I'd guess that it would still help.
|
The build succeeds but I am getting this error on Ubuntu 22.04 with Python 3.11:
|
- Updates SciPy to v1.10.1
- This fixes a reported bug for a customer
- Removes `setup.py` dependency as per the linter's suggestion
- Doing so caused a handful of new dependencies, as we were previously
relying on PyPi to fill in some gaps
- Moves `meson-python` dependency to host section
- Addresses build failure found in upstream [Issue #17234](scipy/scipy#17234)
- Patch file found here:
- scipy/scipy#18242
- https://patch-diff.githubusercontent.com/raw/scipy/scipy/pull/18242.patch
- Updates SciPy to v1.10.1 - This fixes a reported bug for a customer Eric, Ari, and I attempted to move away from using `setup.py` but as Marco stated in the last version bump, `mesonpy` doesn't have great support for non-Linux platforms. Ari recently added some in-house support for other platforms, but we're still at a loss. Here are some notes on what we attempted/how far we got: - Swapped the `setup.py` line in `build.sh` for: `$PYTHON setup.py install --single-version-externally-managed --record=record.txt` - Added `- meson-python 0.12.1` to the `host` section of `meta.yaml` - Found a build failure known to upstream [Issue #17234](scipy/scipy#17234) - Attempted to patch the build with the file found here: - scipy/scipy#18242 - https://patch-diff.githubusercontent.com/raw/scipy/scipy/pull/18242.patch At the end of all this, the build fails on trying to build a Windows executable: ``` Sanity testing C compiler: $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-cc Is cross compiler: False. Sanity check compiler command line: $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-cc sanitycheckc.c -o sanitycheckc.exe -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O3 -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/scipy-1.10.1 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem $PREFIX/include -D_FILE_OFFSET_BITS=64 -Wl,-O2 -Wl,--sort-common -Wl,--as-needed -Wl,-z,relro -Wl,-z,now -Wl,-rpath,$PREFIX/lib -Wl,-rpath-link,$PREFIX/lib -L$PREFIX/lib -shared Sanity check compile stdout: ----- Sanity check compile stderr: ----- Running test binary command: $SRC_DIR/builddir/meson-private/sanitycheckc.exe meson.build:1:0: ERROR: Executables created by c compiler $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-cc are not runnable. ``` So given the urgency of the customer request, we decided to revert our work and ignore the linter, sticking with the `setup.py` solution.
- Updates SciPy to v1.10.1 - This fixes a reported bug for a customer - Adds linter skipping for known issues (see below) Eric, Ari, and I attempted to move away from using `setup.py` but as Marco stated in the last version bump, `mesonpy` doesn't have great support for non-Linux platforms. Ari recently added some in-house support for other platforms, but we're still at a loss. Here are some notes on what we attempted/how far we got: - Swapped the `setup.py` line in `build.sh` for: `$PYTHON setup.py 8000 install --single-version-externally-managed --record=record.txt` - Added `- meson-python 0.12.1` to the `host` section of `meta.yaml` - Found a build failure known to upstream [Issue #17234](scipy/scipy#17234) - Attempted to patch the build with the file found here: - scipy/scipy#18242 - https://patch-diff.githubusercontent.com/raw/scipy/scipy/pull/18242.patch At the end of all this, the build fails on trying to build a Windows executable: ``` Sanity testing C compiler: $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-cc Is cross compiler: False. Sanity check compiler command line: $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-cc sanitycheckc.c -o sanitycheckc.exe -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O3 -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/scipy-1.10.1 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem $PREFIX/include -D_FILE_OFFSET_BITS=64 -Wl,-O2 -Wl,--sort-common -Wl,--as-needed -Wl,-z,relro -Wl,-z,now -Wl,-rpath,$PREFIX/lib -Wl,-rpath-link,$PREFIX/lib -L$PREFIX/lib -shared Sanity check compile stdout: ----- Sanity check compile stderr: ----- Running test binary command: $SRC_DIR/builddir/meson-private/sanitycheckc.exe meson.build:1:0: ERROR: Executables created by c compiler $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-cc are not runnable. ``` So given the urgency of the customer request, we decided to revert our work and ignore the linter, sticking with the `setup.py` solution.
I will try to have a look on that. |
|
with cpython master + cython master (3.0.0b2-36-g781b087c9) + this branch I'm now getting failures that look like: |
|
cython/cython#5361 fixes the above build error. |
|
Thanks for working on this! If there's parts that can be merged separately so the rest is easier to work on, we can do that. |
@matusvalo After looking into this a bit, I found the offending code to be introduced in cython/cython#5040. This seems to be a bug fix for C++ public functions but SciPy didn't fail before since the function was a C function and it erroneously got declared as one in the generated C++ file (up until Cython 3.0.0). Which is why the symbol was found and everything just worked. Now that Does Cython allow overriding this behavior i.e. explicitly declaring a Cython function as an |
|
Found this comment. Just need to change this line: scipy/scipy/special/meson.build Line 374 in 34b9f13 to: cpp_args: [cython_cpp_args, '-DBOOST_MATH_STANDALONE=1', '-D__PYX_EXTERN_C=extern "C"'], |
Added to this PR |
|
We need to change |
|
|
When running test suite, it freezes on following test: |
|
BTW. Now it seems that test suite fails only with following two tests: |
OK I can replicate the issue with following simple example: cimport numpy as cnp
import cython
ctypedef fused np_numeric_t:
cnp.int8_t
cnp.int16_t
cnp.int32_t
cnp.int64_t
cnp.uint8_t
cnp.uint16_t
cnp.uint32_t
cnp.uint64_t
cnp.float32_t
cnp.float64_t
cnp.longdouble_t
cnp.complex64_t
cnp.complex128_t
@cython.initializedcheck(False)
def bandwidth_c(np_numeric_t[:, ::1]A):
returnWhen compiled following code with cython 0.29.X: When compiled following code with Cython 3: |
|
I have raised a question about the issue in the Cython devel mailing list: https://mail.python.org/pipermail/cython-devel/2023-April/005422.html |
|
Thanks @matusvalo. I think it's fine to change these tests to (perhaps temporarily) accept also |
Just repeating what I posted on the mailing list:
(I think the fix would need to go here cython/cython@805d7f3#r110191078, but I haven't yet investigated properly) |
|
Thank you @da-woods. In order to track here the issue I have created issue in Cython - cython/cython#5401 |
@rgommers I am not 100% sure whether we can merge this PR. This PR still requires unmerged branch in Cython in order to work properly cython/cython#5386. If someone from Cython core devs can accept it and merge it, then I think we can proceed putting this PR to master. |
|
I'm now seeing Which I think is related to python/cpython#103809 but I think this is something that needs to be fixed upstream because they used a c++ keyword as a variable name which then fails when included into a c++ file. |
This header is internal and should not be used hence I think they can be reluctant changing it. I will create issue in Cython first, |
|
This header is internal and should not be used hence I think they can be reluctant changing it.
They should still change it, if only to be more future proof for themselves. Private functions do not necessarily stay private and internal forever.
|
|
@rgommers all dependencies were merged to master in Cython. Now scipy can be build with Cython master and the test suite passes. I am marking this PR as ready for review. |
There was a problem hiding this comment.
Very nice work getting this to pass @matusvalo!
This looks good to go. Let me just add one instance of each of the build warnings that are visible in the CI log for completeness:
The keyword 'nogil' should appear at the end of the function signature line. Placing it before 'except' or 'noexcept' will be disallowed in a future version of Cython.
The 'DEF' statement is deprecated and will be removed in a future Cython version. Consider using global variables, constants, and in-place literals instead. See https://github.com/cython/cython/issues/4310
Each of the above appears many times, and isn't fixable or best ignored right now.
And then there's one of this, which I think is fixable but that's not critical so we can leave it alone too:
Only extern functions can throw C++ exceptions.
Just to be sure, does that all sound right?
This can be easily fixed. I think I have somewhere branch with the fix. I can fix it in separate PR.
This I don't know to be honest. I know only basics from C++ unfortunately. |
PR created: #18418 |
|
Thanks, that all sounds good. In it goes! |
Partially revert "MAINT: Allow scipy to be compiled in cython3 (scipy#18242)" See commit a181fd5.
* Partially revert "MAINT: Allow scipy to be compiled in cython3 (scipy#18242)" See commit a181fd5. * Revert "BLD: copy `cython_optimize.pxd` to build dir (scipy#18810)" This reverts commit 3c89445.
Reference issue
Fixes #17234
What does this implement/fix?
This PR allows Scipy to be compiled by cython3.
This version is just PoC and should not be merged. Feel free to close this PR when is not needed anymore.Requirements
Additional information
Part of the PR is coming from #18224
This PR has following predecessors/requirements:
_cythonized_array_utils.pxd#18362ValueErrorinstead ofTypeError. cython/cython#5401