-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
BUG: cythonization / compliation failure with development branch of cython #17234
Comments
You can probably fix this issue in Scipy by changing the definition of
(This should be compatible with the latest 0.29.x release too) In this case though I think Cython should update its PyCapsule definitions though. I'll consider that some more and do it if necessary. So perhaps wait for the Cython to update if it isn't urgent scipy works with the development branch. We expect a small amount of breakage from this change though. But your specific case probably shouldn't be one. |
Right - having investigated a bit more: The capsule destructor function is called in
Therefore the function Therefore I think this is expected. |
With this patch: $ git diff
diff --git a/scipy/_lib/_ccallback_c.pyx b/scipy/_lib/_ccallback_c.pyx
index 0704acc62..93a7ec73c 100644
--- a/scipy/_lib/_ccallback_c.pyx
+++ b/scipy/_lib/_ccallback_c.pyx
@@ -14,7 +14,7 @@ from .ccallback cimport (ccallback_t, ccallback_prepare, ccallback_release, CCAL
# PyCapsule helpers
#
-cdef void raw_capsule_destructor(object capsule):
+cdef void raw_capsule_destructor(object capsule) noexcept:
cdef char *name
name = PyCapsule_GetName(capsule)
free(name) Cythonization fails for 3.0.0a11 that the function definition is invalid cython and with cython master it fails with things that look like
|
Yeah - it doesn't look like
This looks like a different issue. I suspect it comes from cython/cython@0c8dea1 but I'll need to look into it further. |
The issue is that the
Yes I confirm that this commit is source of issue. |
I had another thought about the issue. If I am not wrong, the current behaviour is correct. E.g. in python following structure:
you are not able to import module
|
I don't have a lot of time to think about this this week unfortunately. I think @matusvalo is broadly right that ideally there wouldn't be a file with the same name as a package folder. I believe these declarations are for users of Scipy rather than just Scipy itself so if we're proposing the fix is in Scipy then it probably does need to be backwards compatible and I haven't had a chance to look into that. We could probably do a workaround in Cython if necessary (prefer |
Cython 3.0 beta 1 landed - and we haven't solved this issue yet, hence our pre-release CI job is failing (as reported in gh-18062). So I guess it's time to patch things up. @tirthasheshpatel I have a memory of browsing a conversation last week where you were digging into this. But I can't find it - can you point it out, or am I imagining things? |
@rgommers I think you are remembering #17360. The issue there was that a lot of files were being opened and it exceeded I tried it locally but now I can other failures:
|
Would this be the remedy?
|
This looks to be to do with the new treatment of I think the error is probably a mistake but it needs a bit of investigation. |
This operation does not require the GIL. We can avoid the Cython issue (whether or not it is a Cython bug) by replacing the line with
and include There are two changes in that suggestion: replace Edit: In fact, I suspect there is a 10 year old mistake in this function. This relevant commit is 728b802, and the commit message is "BUG: special: deal with loss of precision in binom(n, k) for |k| >> |n| ", so I think the line
|
I did that and got a lot of
|
If this is going to be only valid for |
Just a note: Cython 3 has |
This chips away at the issues listed in scipygh-17234 for Cython 3.0b1
xref scipygh-17234 for the remaining issues
xref scipygh-17234 for the remaining issues
This chips away at the issues listed in scipygh-17234 for Cython 3.0b1
This chips away at the issues listed in scipygh-17234 for Cython 3.0b1
I had a look at this one - that fix doesn't actually seem to work in any form. It triggers one of these errors:
or
I've tried leaving out a Here is a branch if anyone feels like digging in more: https://github.com/scipy/scipy/compare/main...rgommers:scipy:cy30b1-fixes?expand=1. Using relative cimport's with |
xref scipygh-17234 for the remaining issues [skip actions] [skip cirrus] [skip circle]
Looking in your example you are having this:
Which version of cython are you using? If cython 0.29.X the example won't work. |
Is it really? Normally, once |
…psule_Destructor" function type to document explicitly that it must not emit exceptions. See scipy/scipy#17234
With the current master branch of cython
I am still seeing failures. Applying diff --git a/scipy/_lib/_ccallback_c.pyx b/scipy/_lib/_ccallback_c.pyx
index 5208a15c2..42ef8ab55 100644
--- a/scipy/_lib/_ccallback_c.pyx
+++ b/scipy/_lib/_ccallback_c.pyx
@@ -6,7 +6,7 @@ from cpython.long cimport PyLong_AsVoidPtr
from libc.stdlib cimport free
from libc.string cimport strdup
-from .ccallback cimport (ccallback_t, ccallback_prepare, ccallback_release, CCALLBACK_DEFAULTS,
+from scipy._lib.ccallback cimport (ccallback_t, ccallback_prepare, ccallback_release, CCALLBACK_DEFAULTS,
ccallback_signature_t)
diff --git a/scipy/linalg.pxd b/scipy/linalg.pxd
index 1f656b870..c7c49f9ad 100644
--- a/scipy/linalg.pxd
+++ b/scipy/linalg.pxd
@@ -1 +1 @@
-from .linalg cimport cython_blas, cython_lapack
+from scipy.linalg cimport cython_blas, cython_lapack
diff --git a/scipy/optimize.pxd b/scipy/optimize.pxd
index 2402eeb02..51342b990 100644
--- a/scipy/optimize.pxd
+++ b/scipy/optimize.pxd
@@ -1 +1 @@
-from .optimize cimport cython_optimize
+from scipy.optimize cimport cython_optimize
diff --git a/scipy/optimize/cython_optimize.pxd b/scipy/optimize/cython_optimize.pxd
index d5a0bdd75..d35f8da68 100644
--- a/scipy/optimize/cython_optimize.pxd
+++ b/scipy/optimize/cython_optimize.pxd
@@ -7,5 +7,5 @@
# support. Changing it causes an ABI forward-compatibility break
# (gh-11793), so we currently leave it as is (no further cimport
# statements should be used in this file).
-from .cython_optimize._zeros cimport (
+from scipy.optimize.cython_optimize._zeros cimport (
brentq, brenth, ridder, bisect, zeros_full_output)
diff --git a/scipy/special.pxd b/scipy/special.pxd
index 62cb82807..1daa9fb37 100644
--- a/scipy/special.pxd
+++ b/scipy/special.pxd
@@ -1 +1 @@
-from .special cimport cython_special
+from scipy.special cimport cython_special
diff --git a/scipy/special/_legacy.pxd b/scipy/special/_legacy.pxd
index a97f56dcd..7697f9041 100644
--- a/scipy/special/_legacy.pxd
+++ b/scipy/special/_legacy.pxd
@@ -11,7 +11,7 @@ from libc.math cimport isnan, isinf, NAN
from . cimport sf_error
from ._ellip_harm cimport ellip_harmonic
-from .sph_harm cimport sph_harmonic
+from scipy.special.sph_harm cimport sph_harmonic
from ._cephes cimport (bdtrc, bdtr, bdtri, expn, nbdtrc,
nbdtr, nbdtri, pdtri, kn, yn,
smirnov, smirnovi, smirnovc, smirnovci, smirnovp)
diff --git a/scipy/stats/_unuran/unuran_wrapper.pyx b/scipy/stats/_unuran/unuran_wrapper.pyx
index 5d4d8ebaa..0ddafadc2 100644
--- a/scipy/stats/_unuran/unuran_wrapper.pyx
+++ b/scipy/stats/_unuran/unuran_wrapper.pyx
@@ -8,7 +8,7 @@ from numpy.random cimport bitgen_t
from scipy._lib.ccallback cimport ccallback_t
from scipy._lib.messagestream cimport MessageStream
-from .unuran cimport *
+from scipy.stats._unuran.unuran cimport *
import warnings
import threading
import functools
you
|
…psule_Destructor" function type to document explicitly that it must not emit exceptions. See scipy/scipy#17234
These errors are caused by new See http://docs.cython.org/en/latest/src/userguide/migrating_to_cy30.html#exception-values-and-noexcept I am playing with building scipy with cython3 so I can provide PR for that. |
I have created PoC pull request showing compilable version of scipy using cython3 - #18242 |
- 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 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.
So it seems both are broken - relative imports are clearly safer in principle, but in practice they're used less because historically they've been less robust. It may seem unlikely, but it's currently broken in both |
Cython `v3.0.0` was recently released (https://github.com/cython/cython/releases/tag/3.0.0) and is used in newly built docker images. This causes a compilation issue since 3.0.0 expects function definitions to be explicitly declared with the `noexcept` annotation. This change should be backwards compatible to `v0.29.*`. For more details see the discussion here: scipy/scipy#17234 (comment). Change-Id: Ic252ddfb4262a3b0fffe93c5ca4b9729bf167e05
Cython `v3.0.0` was recently released (https://github.com/cython/cython/releases/tag/3.0.0) and is used in newly built docker images. This causes a compilation issue since 3.0.0 expects function definitions to be explicitly declared with the `noexcept` annotation. This change should be backwards compatible to `v0.29.*`. For more details see the discussion here: scipy/scipy#17234 (comment). Change-Id: Ic252ddfb4262a3b0fffe93c5ca4b9729bf167e05
Cython `v3.0.0` was recently released (https://github.com/cython/cython/releases/tag/3.0.0) and is used in newly built docker images. This causes a compilation issue since 3.0.0 expects function definitions to be explicitly declared with the `noexcept` annotation. This change should be backwards compatible to `v0.29.*`. For more details see the discussion here: scipy/scipy#17234 (comment). Change-Id: Ic252ddfb4262a3b0fffe93c5ca4b9729bf167e05
Describe your issue.
cython commit cython/cython@77918c5 in cython/cython#4670 broke scipy installation. This is currently only on cython's master branch and not in an alpha release yet.
I'm not sure if this is best fixed on the scipy or cython side.
I reported this via back-channels to @rgommers.
Reproducing Code Example
with the development version of cython installed.
The text was updated successfully, but these errors were encountered: