Skip to content

Fix GIL being re-enabled by C++ extensions#8059

Merged
stefanv merged 8 commits intoscikit-image:mainfrom
ngoldbaum:fix-gil
Feb 25, 2026
Merged

Fix GIL being re-enabled by C++ extensions#8059
stefanv merged 8 commits intoscikit-image:mainfrom
ngoldbaum:fix-gil

Conversation

@ngoldbaum
Copy link
Copy Markdown
Contributor

@ngoldbaum ngoldbaum commented Feb 18, 2026

Towards fixing #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 #7678.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 18, 2026

📝 Walkthrough

Walkthrough

These 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

Cohort / File(s) Summary
CI/CD Workflow
/.github/workflows/test-linux.yaml
Added "3.14t" to the Python matrix for test_skimage_linux; removed the test_skimage_linux_free_threaded job and its steps; updated report-failures job dependencies to drop the removed job.
Build Configuration
src/skimage/meson.build
Renamed cython_argscython_codegen_args; updated cython_gen and C++ codegen paths to use the renamed variable and consolidated related argument handling.
Test Infrastructure
tests/conftest.py
Added FREE_THREADED_BUILD and GIL_ENABLED_AT_START flags; conditionally initialize GIL state; added pytest_terminal_summary hook to warn and abort if the GIL was re-enabled during the test run.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main objective of the PR: fixing GIL re-enablement by C++ extensions.
Description check ✅ Passed The pull request description clearly explains the issue (GIL being re-enabled on free-threaded builds), the root cause in the meson configuration, and all the changes made to address it.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Feb 18, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🔴 Critical

Stale needs reference to removed job will break the entire workflow.

test_skimage_linux_free_threaded was removed in this PR but is still listed in the needs of the report-failures job. GitHub Actions validates all needs references at parse time and will reject the workflow with a validation error, preventing every job — including the primary test_skimage_linux matrix — from running on main pushes.

🐛 Proposed fix
-    needs: [test_skimage_linux, test_skimage_linux_free_threaded]
+    needs: [test_skimage_linux]

@ngoldbaum
Copy link
Copy Markdown
Contributor Author

FWIW, here's a passing CI run from my fork: https://github.com/ngoldbaum/scikit-image/actions/runs/22157729121

Copy link
Copy Markdown
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Ready to merge if the CI is happy too. Thanks!

@lagru lagru added 🤖 type: Infrastructure CI, packaging, tools and automation and removed 🩹 type: Bug fix Fixes unexpected or incorrect behavior labels Feb 19, 2026
@stefanv
Copy link
Copy Markdown
Member

stefanv commented Feb 25, 2026

Thank you, @ngoldbaum!

@stefanv stefanv merged commit ea7afcb into scikit-image:main Feb 25, 2026
23 of 25 checks passed
@stefanv stefanv added this to the 0.27 milestone Feb 25, 2026
matusvalo pushed a commit to matusvalo/scikit-image that referenced this pull request Mar 4, 2026
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.
matthew-brett added a commit that referenced this pull request Apr 10, 2026
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖 type: Infrastructure CI, packaging, tools and automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants