Fix GIL being re-enabled by C++ extensions#8059
Conversation
📝 WalkthroughWalkthroughThese changes update CI workflow and tests to include a free-threaded Python 3.14t variant, remove a separate free-threaded job, rename Meson cython build variable(s) for clarity, and add test-time detection that aborts the suite if the Python GIL is re-enabled at runtime. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-linux.yaml (1)
137-137:⚠️ Potential issue | 🔴 CriticalStale
needsreference to removed job will break the entire workflow.
test_skimage_linux_free_threadedwas removed in this PR but is still listed in theneedsof thereport-failuresjob. GitHub Actions validates allneedsreferences at parse time and will reject the workflow with a validation error, preventing every job — including the primarytest_skimage_linuxmatrix — from running onmainpushes.🐛 Proposed fix
- needs: [test_skimage_linux, test_skimage_linux_free_threaded] + needs: [test_skimage_linux]
|
FWIW, here's a passing CI run from my fork: https://github.com/ngoldbaum/scikit-image/actions/runs/22157729121 |
lagru
left a comment
There was a problem hiding this comment.
Makes sense to me. Ready to merge if the CI is happy too. Thanks!
|
Thank you, @ngoldbaum! |
Towards fixing scikit-image#7464 Currently, running the scikit-image tests fails on the free-threaded build because the GIL is enabled at runtime: ``` _______________________________________________ ERROR collecting build-install/usr/lib/python3.14t/site-packages/skimage/feature/censure.py ________________________________________________ build-install/usr/lib/python3.14t/site-packages/skimage/feature/censure.py:6: in <module> from ..morphology import octagon, star build-install/usr/lib/python3.14t/site-packages/skimage/morphology/__init__.py:28: in <module> from ._skeletonize import medial_axis, skeletonize, thin build-install/usr/lib/python3.14t/site-packages/skimage/morphology/_skeletonize.py:10: in <module> from ._skeletonize_lee_cy import _compute_thin_image E RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'skimage.morphology._skeletonize_lee_cy', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0. ``` This is happening because the `cython_args` aren't being passed to the C++ codegen function in the project's meson configuration. I changed the name of the variable to `cython_codegen_args` to make it a little clearer it has nothing to do with `cython_c_args` and `cython_cpp_args`, which are passed to e.g. clang and not Cython. I also made `cython_gen_cpp` use the same arguments as `cython_gen`. `c_undefined_ok` is unused, so I deleted it. Finally, I added a regression test for this in the project's `conftest.py`, following a similar check I added to numpy's `conftest.py` a while back. I also added a 3.14t testing job and deleted the 3.13t job and special testing logic. All of that is unnecessary now. I will follow up after this to make the 3.14t testing job use pytest-run-parallel, superseding scikit-image#7678.
* origin/main: (27 commits) Move `_shared` to `_skimage2` (#8102) Port `pad_footprint` & `mirror_footprint` to `skimage2` (#8094) Add rescaling API to skimage2 (#8075) Move `skimage2` implementation into `_skimage2` (#8093) Undo (double) mirroring in `ski2.morphology.dilation` (#8060) Grammar (#8091) Move `ensure_spacing` into `skimage2` (#8067) Ensure that `skimage2` does not eagerly import `skimage` (#8087) Use lazy-loader 0.5 (#8080) MAINT: make ellipse fitting forward compatible (#8054) Avoid deprecated assign to ndarray.shape (#8020) Avoid circular import in `feature/corner.py` (#8077) Turn off coderabbit auto-labeling & label checks (#8070) Fix GIL being re-enabled by C++ extensions (#8059) Fix conventions following docstub and misc. (#8055) Port grayscale morphology operators to skimage2 (#8046) Add revised `peak_local_max` to `skimage2` (#8039) Allow read-only arrays as input to remap (#7535) Add `prescale` parameter to "blob functions" (#7858) Try to make coderabbit respect exisiting type labels (#8042) ...
Towards fixing #7464
Currently, running the scikit-image tests fails on the free-threaded build because the GIL is enabled at runtime:
This is happening because the
cython_argsaren't being passed to the C++ codegen function in the project's meson configuration.I changed the name of the variable to
cython_codegen_argsto make it a little clearer it has nothing to do withcython_c_argsandcython_cpp_args, which are passed to e.g. clang and not Cython. I also madecython_gen_cppuse the same arguments ascython_gen.c_undefined_okis unused, so I deleted it.Finally, I added a regression test for this in the project's
conftest.py, following a similar check I added to numpy'sconftest.pya while back. I also added a 3.14t testing job and deleted the 3.13t job and special testing logic. All of that is unnecessary now.I will follow up after this to make the 3.14t testing job use pytest-run-parallel, superseding #7678.