COMP: Make ITKVtkGlue wrapping abi3-compatible - #6715
Conversation
dzenanz
left a comment
There was a problem hiding this comment.
Looks good on a glance. Matt and Brad should review this.
|
Force-push
Root cause
It failed on exactly one configuration because three things have to line up: an undefined CMake variable expands to empty without error, ELF shared objects tolerate undefined symbols, and static archives never resolve them at all. Only a shared macOS build links Verified by configuring |
This comment was marked as resolved.
This comment was marked as resolved.
c046803 to
d542b02
Compare
|
Two force-pushes just landed, split by concern: |
|
What are the implications of this for building against the Python Free Thread ABI. Currently Free Threads are not stable and there is expected to be a separate stable ABI for 3.15: |
|
I read this, and the requirements for |
|
I do think this is a mandatory step toward abi3t, but I am not an expert on those details. The primary motivator was to remove complexity in the cmake options, and remove unnecessary dependance on non-abi3 code. |
thewtex
left a comment
There was a problem hiding this comment.
I don't think we necessarily want to remove support for building without abi3. Linux distributions, etc. may want/need to not use abi3.
|
Yes, abi3t will need to be a separate build. |
I agree with this. For example for the condo-forge distribution they are still build per python distribution. It is unclear if the SWIG generated code utilizes the extra available interfaces or not in these configurations. |
d542b02 to
66a1cf1
Compare
|
@thewtex @blowekamp — thanks both; your comments changed the shape of this PR. It is now only the The ABI questions you raised are tracked separately in #6757 — free-threaded CPython, Because Limited API calls are also valid in a full-API build, one implementation serves both modes — the module does not become abi3-only, and the conda-forge: abi3 is compatible with per-version builds, and does not require them to collapseChecked against conda-forge's own recipe machinery rather than assumed. conda-forge supports python_version_independent: true # [is_abi3]
skip: True # [is_abi3 and not is_python_min]plus a The relevant point for ITK: this is opt-in. A feedstock that keeps its per-version matrix and simply compiles For reference, One correction to how this is sometimes stated. Version-specific builds are not needed "for
So free-threaded 3.13t/3.14t require a build that is neither That changes at 3.15, but not to a single build: PEP 803 recommends shipping both, tagged |
|
I still like this. I don't have time to carefully review. |
|
@thewtex would you be able to review this PR? |
|
I defer to VTK developers. |
|
@dzenanz @thewtex Do you know who uses this mechanism and could review? The purpose for addressing this conversion was to simplify the interplay between sabi vs non-sabi across Python builds. These changes represent using the stable-abi in such a way that it works both with and without the requirement being set. |
dzenanz
left a comment
There was a problem hiding this comment.
I think that Slicer is a prominent user of this. Another is probably ITK-SNAP. Maybe MITK too?
I am formally approving this, despite not being entirely certain that this works. It is not very likely we will get another review. I am relying on your testing.
Exchange pointers with VTK's Python layer through the `__this__` and `Addr=0x...` encodings using only Limited API calls, instead of through vtkPythonUtil, whose header chain accesses PyTypeObject members that Py_LIMITED_API hides. Dropping VTK::WrappingPythonCore from the wrapping link interface is required for correctness, not tidiness: that library is built against one libpython, so an extension linking it cannot be version-agnostic. Neither encoding is documented VTK API, so PythonVtkGlueABI3EncodingTest asserts both still hold and PythonVtkGlueRoundTripTest exercises the typemaps end to end. Closes: InsightSoftwareConsortium#6711
66a1cf1 to
37f05c6
Compare
VtkGlueRoundTripTest exercises the vtkImageData pair. The vtkImageImport and vtkImageExport accessors, non-float pixel types, 3D images, object identity across the address round trip, lifetime after the producing filter is released, and rejection of unconvertible input were untested. The test avoids numpy so it runs wherever ITKVtkGlue is wrapped, without requiring ITKBridgeNumPy.
|
@dzenanz — you said you liked this but didn't have time to review carefully, and @thewtex deferred to VTK developers. Rather than ask for more review time, I expanded the test coverage and validated the branch on two platforms so the evidence stands on its own. Every configuration passes, including a pre-PR baseline. Two OSes, two VTK minor versions, both wrapping modes.
The Linux rows are new — the original description was macOS-only. The branch was also rebased onto current New test: PythonVtkGlueTypemapCoverageTest
It deliberately avoids numpy, so it runs wherever ITKVtkGlue is wrapped without requiring Worth noting for context: Artifact audit, re-verified on the rebased branch
The A macOS detour worth recording, since it is not a defect in this PRThe three C++ tests initially segfaulted on macOS. They segfault identically on unpatched The cause is Slicer's VTK being a kit build ( So the macOS rows above use a purpose-built VTK 9.6.2: non-kit, rendering enabled, Python-wrapped for the same 3.13.9 interpreter ITK uses, same compiler and deployment target as ITK. Nothing to fix here; recording it so the next person testing VtkGlue against a Slicer VTK does not lose an evening to it. Separately, this surfaced #6846: ITKVtkGlue cannot configure against a rendering-free VTK because the |
|
CI doesn't do a Windows Python build. Let me do that locally. |
You need to explicitly request this; it is not enabled by default. And it requires a VTK build with rendering to be available. |
Makes
ITKVtkGlue's Python wrapping stable-ABI clean, so it can be built withITK_USE_PYTHON_LIMITED_API=ON— the configuration itsFATAL_ERRORpreviously rejected. Closes #6711.Scope is deliberately limited to
Modules/Bridge/VtkGlue/. No CMake option, Python discovery, or wrapping-macro behaviour changes here; whether ITK should default toabi3is separate work, tracked in #6757 along with free-threading andabi3t.Because Limited API calls are also valid in a full-API build, one implementation serves both modes — the module does not become abi3-only, and the
FATAL_ERRORis removed rather than inverted.Verified on macOS 26 / arm64 only. No Linux or Windows verification, and no Slicer test result.
The fix, and a defect in the approach #6711 proposed
VtkGlue.iexchanged pointers with VTK throughvtkPythonUtil, whose header chain reachesPyVTKObject.hand touchesPyTypeObjectmembers thatPy_LIMITED_APIhides. The typemaps now use Limited API calls only, parsing VTK's__this__encoding and reconstructing throughAddr=0x....VTK::WrappingPythonCoreis dropped from the wrapping link interface. This is a correctness requirement rather than tidying: that library is built against onelibpython, so an extension linking it cannot be version-agnostic.Defect in the sketch from #6711. The issue proposed
PyObject_CallFunction(cls, "s", addr), i.e.vtkImageData("Addr=0x…"). On VTK 9.6.2 this fails, but only forvtkImageDataandvtkPolyData, and only in the pointer-to-object direction. The cause is not ABI-related:vtkmodules/util/data_model.pyregisters@vtkImageData.overridewith a Python subclass whose__init__(self, **kwargs)is keyword-only, so the positional address string never reaches VTK's C-level reconstruction.vtkImageExport,vtkImageImport,vtkObjectandvtkPointshave no override and were unaffected.The fix is to call
__new__explicitly: it bypasses the override's__init__while still returning the enhancedImageDatatype, with pointer identity preserved.Neither
__this__norAddr=0x...is documented VTK API, soPythonVtkGlueABI3EncodingTestexists to fail loudly and specifically if VTK changes either one.PythonVtkGlueRoundTripTestexercises the typemaps end to end.Environments verified (and what was not)
Platform: macOS 26 / arm64, Apple clang via conda-forge toolchain.
VTK:
github.com/slicer/VTKat6181bb1223bbc499a340a1644f5356e7e152c318— the SHA Slicer'sSuperBuild/External_VTK.cmakepins for its 9.6 series — built with Qt6 6.9.1, full rendering, and Python wrapping.Both wrapping modes, built and tested separately
ITK_USE_PYTHON_LIMITED_API=ON=OFFFAILED:FAILED:ctest -R PythonPythonVtkGlueABI3EncodingTestPythonVtkGlueRoundTripTest_ITKVtkGluePython.abi3.so_ITKVtkGluePython.sopre-commit run --all-files: exit 0.Artifact audit (limited-API mode)
_ITKVtkGluePython.abi3.soundefinedPy*symbolsPyVTK*/vtkPythonUtilsymbolslibvtkWrappingPythonCorelinkedlibpythonlinkedMulti-interpreter. Extensions were built against pixi Python 3.13.9; the others are Homebrew builds, a different distribution. VTK's Python wrappers were built separately for cp311/cp312/cp313/cp314 against the same VTK C++ libraries, so each row pairs the same ITK
.abi3.sowith a different version-specific VTK.The 3.11 and 3.14 rows are the substantive ones: that
.abi3.sowas compiled against cp313 VTK headers yet drove full round-trips against cp311 and cp314 VTK modules. WhileVTK::WrappingPythonCorewas linked, those pairings were structurally impossible.Downstream
ITK_WRAP_PYTHON=OFF,Module_ITKVtkGlue=ON: 2730/2730 targets, 0 errors.ITK_WRAP_PYTHON=OFF).Not verified: Linux, Windows, any non-arm64 target, and any completed Slicer test suite.