ENH: Add ITK_PYTHON_RELEASE_GIL option and SWIG -threads flag#6022
Conversation
|
| Filename | Overview |
|---|---|
| Wrapping/WrappingOptions.cmake | Adds cmake_dependent_option ITK_PYTHON_RELEASE_GIL (default ON when ITK_WRAP_PYTHON) — correct usage and placement. |
| Wrapping/Generators/Python/CMakeLists.txt | Conditionally appends -threads to SWIG command based on ITK_PYTHON_RELEASE_GIL; correct scoping with unset cleanup. |
| Wrapping/Generators/Python/Tests/CMakeLists.txt | PythonGILReleaseTest registered unconditionally — will fail when ITK_PYTHON_RELEASE_GIL=OFF. |
| Wrapping/Generators/Python/Tests/test_gil_release.py | GIL release test using timing heuristics; logic is reasonable but timing-based pass/fail can be flaky on constrained machines. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CMake Configure] --> B{ITK_WRAP_PYTHON?}
B -- No --> C[ITK_PYTHON_RELEASE_GIL forced OFF]
B -- Yes --> D{ITK_PYTHON_RELEASE_GIL
default ON}
D -- ON --> E[SWIG -threads flag added]
D -- OFF --> F[No -threads flag]
E --> G[Bindings release GIL
on each C++ call]
F --> H[Bindings hold GIL
during C++ calls]
G --> I{BUILD_TESTING?}
H --> I
I -- Yes --> J[PythonGILReleaseTest
registered unconditionally]
J --> K[Test asserts 2x speedup
across 4 threads]
K --> L{GIL released?}
L -- Yes --> M[PASS]
L -- No --> N[FAIL - even for valid OFF config]
Reviews (1): Last reviewed commit: "ENH: Add ITK_PYTHON_RELEASE_GIL option a..." | Re-trigger Greptile
|
The Python GIL may need to be required on call back such as in ITK commands, and I think ITK python has a filter callback to implement a filter in python too? |
- Added cmake option ITK_PYTHON_RELEASE_GIL (default ON) in WrappingOptions.cmake - Modified Python CMakeLists.txt to add -threads flag to SWIG when option is enabled - Created test_gil_release.py to verify GIL is released during ITK operations - Added test to CMakeLists.txt Co-authored-by: Matt McCormick <matt@fideus.io> Co-authored-by: dzenanz <1792121+dzenanz@users.noreply.github.com>
Detailed Review: GIL Release via SWIG -threadsSummaryTested across Python 3.10-3.14 with comprehensive gotcha tests. The PR's approach is fundamentally sound — Test Results (Python 3.10-3.14, all versions identical)
Known Gotchas Found in Research1. SWIG director methods don't auto-acquire GIL (CRITICAL) When C++ calls back into Python via a SWIG director (virtual method override in Python), the generated code does NOT acquire the GIL before calling the Python method. This causes segfaults. (swig/swig#3091) ITK mitigates this via manual Recommendation: Add a comment/warning in the PR description that any new Python callback code MUST use 2. C++ exceptions + thread state = potential crash When Tested: Exception propagation works correctly in current ITK builds (RuntimeError from file-not-found propagates cleanly). This appears fixed in modern SWIG versions. 3. Process exit race condition If the main thread calls Recommendation: Document that users should ensure all ITK operations complete before process exit. This is inherent to any GIL-releasing code, not specific to this PR. 4. SwigPyIterator::value() called without GIL The generated wrapper calls In practice this hasn't caused issues because ITK Python rarely uses SWIG iterator objects directly, but it's a latent bug. 5. @blowekamp's concern: Python filter callbacks As noted, What the PR Does Right
Suggestions
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Comprehensive GIL release safety test resultsCreated Results: ITK_PYTHON_RELEASE_GIL=ON
Results: ITK_PYTHON_RELEASE_GIL=OFF
Tests pass regardless of the setting because they test safety invariants (no crashes, no deadlocks, no data corruption) rather than concurrency speedup. Tests
The test file is ready to be included in this PR or submitted as a separate PR. |
2868e1c
into
InsightSoftwareConsortium:release-5.4
Backport from
mainfor ITK 5.4.