Conversation
…n python3 rather than version specific library need to test wheel packaging and cross-platform
|
@aymanhab if you are looking for some additional tests/checks for the stable ABI stuff then it might be useful to know what I ended up doing for OPynSim, which has strict dependency/symbol/stableABI checks. I have some scripts that specifically check the binaries that pop out of the build right before they are packaged into wheels. E.g. anything with
The pertinent thing for stable ABI is to make sure the executable links to an unversioned UNIXes don't necessarily need to link to Python iirc. It just needs to ensured that they expose an appropriate There's also checks in there for macOS framework version and libstdc++ symbol versions. They're less about stable ABI and more about ensuring the binaries install on older versions of Linux etc. |
…ting setup.py for correct wheel name
|
Thanks for the pointers @adamkewley much appreciated 👍 The linking to python3 was done in CMake by linking to PYTHON3_STABLE_LIB and seems to work/link to the correct library on Windows, Mac, so all looking promising. Once I get ManyLinux to cooperate will have wheels on all platforms to test with and to run scripts similar to yours before packaging. |
…o other platforms and to facilitate testing now that we have one wheel per platform
Specify Python3 root directory for OpenSim configuration.
|
All platforms now build and publish the abi3 wheels. Tested on windows with 3.13 and 3.14 using the same wheel worked out of the box like a charm 😃 Any volunteers to give the wheels a spin on a version other than python 3.11 (used on ci) on Mac or linux? @nickbianco @AlbertoCasasOrtiz @carmichaelong or @adamkewley Thank you |
|
@aymanhab Ubuntu 24 seems fine for me. Detailed report below For Ubuntu 24, I tried this one-liner with for v in "3.10" "3.11" "3.12" "3.13" "3.14"; do mkdir -p $v && cd $v && uv venv --clear --python $v && uv pip install ../opensim-4.6-1-cp310-abi3-linux_x86_64.whl && uv run python -c "import opensim ; opensim.Model() ; print('$v works')" && cd - || break ; doneFirst run had an issue with openblas: Fixed by installing it system-wide (this is something almost any Linux user will automatically LLM/google and run): sudo apt-get install libopenblas0Re-running passed for all versions: |
|
@aymanhab for macOS, the wheel in CI doesn't look like a stableABI one? It's cp311-cp311, rather than cp311-abi3 (or even cp310-abi3, if we're going for 3.10). Just to give it a whack anyway, I renamed the file to For macOS, i ran this one-liner (after modding the filename): Output: Using CPython 3.11.15
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
Resolved 2 packages in 4ms
Installed 2 packages in 17ms
+ numpy==2.4.6
+ opensim==4.6 (from file:///Users/adam/Desktop/opensim-stableabi/opensim-4.6-1-cp311-abi3-macosx_15_0_universal2.whl)
3.11 works
~/Desktop/opensim-stableabi
Using CPython 3.12.13
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
Resolved 2 packages in 3ms
Installed 2 packages in 16ms
+ numpy==2.5.3
+ opensim==4.6 (from file:///Users/adam/Desktop/opensim-stableabi/opensim-4.6-1-cp311-abi3-macosx_15_0_universal2.whl)
3.12 works
~/Desktop/opensim-stableabi
Using CPython 3.13.14
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
Resolved 2 packages in 3ms
Installed 2 packages in 14ms
+ numpy==2.5.3
+ opensim==4.6 (from file:///Users/adam/Desktop/opensim-stableabi/opensim-4.6-1-cp311-abi3-macosx_15_0_universal2.whl)
3.13 works
~/Desktop/opensim-stableabi
Using CPython 3.14.0 interpreter at: /Library/Frameworks/Python.framework/Versions/3.14/bin/python3.14
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
Resolved 2 packages in 3ms
Installed 2 packages in 15ms
+ numpy==2.5.3
+ opensim==4.6 (from file:///Users/adam/Desktop/opensim-stableabi/opensim-4.6-1-cp311-abi3-macosx_15_0_universal2.whl)
(macOS CRASH: segfault)Segfault excerpt: Exception Type: EXC_BAD_ACCESS (SIGSEGV)
Exception Subtype: KERN_INVALID_ADDRESS at 0x0000000000000007
Exception Codes: 0x0000000000000001, 0x0000000000000007
Termination Reason: Namespace SIGNAL, Code 11, Segmentation fault: 11
Terminating Process: exc handler [68533]
VM Region Info: 0x7 is not in any region. Bytes before following region: 4297080825
REGION TYPE START - END [ VSIZE] PRT/MAX SHRMOD REGION DETAIL
UNUSED SPACE AT START
--->
__TEXT 100204000-100208000 [ 16K] r-x/r-x SM=COW /Library/Frameworks/Python.framework/Versions/3.14/Resources/Python.app/Contents/MacOS/Python
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 _simulation.so 0x10d129b88 <deduplicated_symbol> + 12
1 _simulation.so 0x10d129a24 SwigPyObject_Check(_object*) + 32
2 _simulation.so 0x10d129518 SWIG_Python_GetSwigThis(_object*) + 24
3 _simulation.so 0x10d154ed0 SWIG_Python_InitShadowInstance(_object*) + 84
4 _simulation.so 0x10ce19a44 Model_swiginit(_object*, _object*) + 28
5 Python 0x1014401a0 cfunction_call + 108
6 Python 0x101558678 _PyEval_EvalFrameDefault + 74220
7 Python 0x101543de0 _PyEval_Vector + 632
8 Python 0x1013c8688 _PyObject_Call_Prepend + 160
9 Python 0x1014876bc slot_tp_init + 860
10 Python 0x10147b308 type_call + 148
11 Python 0x101558678 _PyEval_EvalFrameDefault + 74220
12 Python 0x101543de0 _PyEval_Vector + 632
13 Python 0x101543ac4 PyEval_EvalCode + 160
14 Python 0x1016516f8 run_mod + 308
15 Python 0x1016532c8 _PyRun_SimpleStringFlagsWithName + 280
16 Python 0x101680dc0 Py_RunMain + 1272
17 Python 0x101682144 pymain_main + 444
18 Python 0x1016822c4 Py_BytesMain + 40
19 dyld 0x181df44e4 start + 6992 |
|
@adamkewley the wheel file I'm seeing attached to the CI runs is I had to modify the one-liner slightly, but everything works fine on my Mac: |
|
Awesome feedback @adamkewley and @nickbianco 🥇 will try to automate more testing in ci and investigate reported issue(s) |
|
@aymanhab I'd say that if we look hard enough there's certainly going to be end-users that refuse to move from 3.8, even, because (e.g.) they have some ancient ROS integration and don't want to upgrade. In my opinion, in the case of legacy users using beyond-EOL Pythons, they should be advised to pin to an earlier OpenSim or build the project from source against the Python they need. Expecting the OpenSim project to maintain and ship 3.8, 3.9, 3.10, abi3 wheels on 3 platforms (i.e. 12 builds) is quite a bit of work. Doing one stable-ABI build reduces maintenance work to a point where the wheels can be part of the main build and the special "wheels build" stuff can be dropped, meaning OpenSim can test+ship new Python wheels to end-users before a release, even. However, for legacy source builds against older Pythons, builders might need some kind of SABI-toggle in CMake. This is because systems like SWIG might disallow SABI against older Pythons, and because OS maintainers "know" the OS has particular version of Python and therefore would prefer to build an optimized Python-version-specific target. It might be that it's easiest to add that toggle now, because this PR now has a changeset that shows every singe location that needs to be controlled by the toggle. Otherwise, I'd PR the toggle separately when the need pops up (imo). For context, OPynSim provides a toggle via |
|
Great feedback @adamkewley Indeed I have updated the ci script to publish the abi wheel on all platforms including windows so that regular ci artifacts can be used without waiting for the wheels build (which may stay around for tagged versions+authentication on pypi when needed). |
|
@aymanhab maybe I'm misunderstanding your previous comment, but I assume that the wheels builds will eventually be updated to only build one wheel per platform via the stable ABI? I think I would prefer to have a separate workflow for building and publishing wheels to PyPI. |
|
@nickbianco Sorry for the confusion, the current plan is to include the stable abi wheels on all platforms with every ci build, then when we have a release/tag build and publish those stable abi wheels to PyPi using the separate build/publish workflows. And the build_wheels workflow will be updated accordingly. |
|
@aymanhab no worries and sounds good -- that was my understanding of the plan. |
…pensim-core into limited_python_310
… it off USE_PYTHON_STABLE_ABI option
|
This PR now builds the wheels cross platform and have a switch to toggle (default to use stable abi), wheels workflow updated as well. |
adamkewley
left a comment
adamkewley
left a comment
There was a problem hiding this comment.
@aymanhab seems good, the key take-home being that it works and will dramatically improve the build process, plus make each release of OpenSim usable for a longer time (with newer Pythons, etc.). Most of my comments are just about possible maintenance/cleanups to the build system, which may be non-obvious but may reduce surprises/headaches when reapproaching the build in a couple years' time.
@adamkewley reviewed 6 files and all commit messages, and made 11 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on aymanhab and nickbianco).
CMakeLists.txt line 438 at r1 (raw file):
set(required_python_version 3) set(Python3_ROOT_DIR "" CACHE PATH "Top level directory containing Python3. For conda environments, this can also be set to the top level directory for a specific environment.") find_package(Python3 3.10 REQUIRED COMPONENTS Interpreter Development.Module NumPy)
Just as a future cleanup, find_package has a Development.SABIModule COMPONENT that produces a Python::SABIModule target that can be linked to - might be easier than the hard-coded lib lookups etc.:
https://cmake.org/cmake/help/latest/module/FindPython.html
CMakeLists.txt line 446 at r1 (raw file):
message("Python3_Numpy_VERSION:${Python3_NumPy_VERSION}") get_filename_component(PY_LIB_DIR "${Python3_LIBRARIES}" DIRECTORY)
This path construction is because of the above. If SABIModule is defined, then you get things like Python::SABIModule and Python_SABI_LIBRARIES, which prevents having to mess around in downstream cmake scripts.
CMakeLists.txt line 451 at r1 (raw file):
set(PYTHON3_STABLE_LIB "${PY_LIB_DIR}/python3.lib") elseif(APPLE) set(PYTHON3_STABLE_LIB "${PY_LIB_DIR}/libpython3.so")
This cannot be valid: APPLE doesn't use .so files (.dylib)
.github/workflows/build_all_wheels.yml line 17 at r1 (raw file):
strategy: matrix: python-version: ['3.11']
This must've felt good ❤️ - less crap to build!
Bindings/Python/CMakeLists.txt line 22 at r1 (raw file):
# you must use Python 3.8 or higher. If sbi version needs to be changed # modify the value of the Py_LIMITED_API definition in this file. option(USE_PYTHON_STABLE_ABI
I'd consider calling this something like OPENSIM_USE_PYTHON_SABI - cmake cache variables are typically namespaced on a per-project basis (e.g. OPYN_USE_SABI, OSC_BUILD_DOCS, OPYN_BUILD_DOCS) - makes it possible to inline multiple projects into a megaproject with add_subdirectory.
Bindings/Python/CMakeLists.txt line 137 at r1 (raw file):
-Wno-deprecated-declarations -Wno-error=missing-field-initializers $<$<BOOL:${USE_PYTHON_STABLE_ABI}>:-DPy_LIMITED_API=${PY_LIMITED_API_HEX}>
I'd separate this from the flags that are messing around with warnings. E.g. have: codegen defs (optimizer flags like -O0, this flag), warning flags.
Bindings/Python/CMakeLists.txt line 177 at r1 (raw file):
set_target_properties(${_libname} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup") elseif(APPLE OR WIN32)
In modern CMake, esp. with Python::SABIModule, it can be preferable to link to targets, not libraries, because targets can do things like set include headers, include other libraries, etc. - iirc in this case the link targets might be:
if (USE_PYTHON_STABLE_ABI)
target_link_libraries(${_libname} PRIVATE Python::SABIModule)
else()
target_link_libraries(${_libname} PRIVATE Python::Module)
endif()Could be worth a try if also switching to the Python::SABIModule COMPONENT?
Bindings/Python/CMakeLists.txt line 199 at r1 (raw file):
else() # Defaults to .dylib; change to .so. set_target_properties(${_libname} PROPERTIES SUFFIX ".abi3.so")
This fixes the .so/.dylib thing I mentioned earlier: APPLE has no place near .so/.dylib. In fact, it's more accurate to say UNIX AND NOT APPLE or something so that APPLE doesn't hit this code.
Bindings/Python/setup.py line 40 at r1 (raw file):
version=__version__, options={"bdist_wheel": {"build_number": "1", "py_limited_api": "cp310"}},
With the CMake SABI toggle, I'd either clarify that this setup.py file is SABI-only, or do something like refactor it to setup.py.in and then use configure_file from cmake to generate a different setup.py in the build dir if toggling should also apply to this file.
Bindings/Python/CMakeLists.txt line 21 at r1 (raw file):
# Default is on and version 3.10 if you want to use the stable ABI, # you must use Python 3.8 or higher. If sbi version needs to be changed # modify the value of the Py_LIMITED_API definition in this file.
Modify the value of PY_LIMITED_API_HEX? - I know what you're getting at (it's used in Py_LIMITED_API later on), but I'm guessing the intent of the _HEX is to provide a nearby location devs can edit the hex?
|
Removed myself as a reviewer since @adamkewley's review seems to have covered everything. |
….SABIModule per feedback on PR
aymanhab
left a comment
aymanhab
left a comment
There was a problem hiding this comment.
Thanks for the great review @adamkewley 👍 I'll switch to SABI module and see if I can get it through the ci locally then on github. Switch done, all wheels build properly. Ready for another pass.
@aymanhab partially reviewed 2 files and made 9 comments.
Reviewable status: 3 of 6 files reviewed, 6 unresolved discussions (waiting on adamkewley).
CMakeLists.txt line 438 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
Just as a future cleanup,
find_packagehas aDevelopment.SABIModuleCOMPONENTthat produces aPython::SABIModuletarget that can be linked to - might be easier than the hard-coded lib lookups etc.:
switched to SABIModule
CMakeLists.txt line 446 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
This path construction is because of the above. If SABIModule is defined, then you get things like
Python::SABIModuleandPython_SABI_LIBRARIES, which prevents having to mess around in downstream cmake scripts.
Thanks much @adamkewley I'll give that a try 👍
CMakeLists.txt line 451 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
This cannot be valid: APPLE doesn't use
.sofiles (.dylib)
Done.
.github/workflows/build_all_wheels.yml line 17 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
This must've felt good ❤️ - less crap to build!
🎆
Bindings/Python/CMakeLists.txt line 21 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
Modify the value of
PY_LIMITED_API_HEX? - I know what you're getting at (it's used inPy_LIMITED_APIlater on), but I'm guessing the intent of the_HEXis to provide a nearby location devs can edit the hex?
Yes, intent is when we decide to upgrade to base 3.12 down the road then there's one place to modify. I'll add the namespace per your later comment but if you think this is confusing, I'm open to better names.
Bindings/Python/CMakeLists.txt line 22 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
I'd consider calling this something like
OPENSIM_USE_PYTHON_SABI- cmake cache variables are typically namespaced on a per-project basis (e.g.OPYN_USE_SABI,OSC_BUILD_DOCS,OPYN_BUILD_DOCS) - makes it possible to inline multiple projects into a megaproject withadd_subdirectory.
Agreed
Bindings/Python/CMakeLists.txt line 177 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
In modern CMake, esp. with
Python::SABIModule, it can be preferable to link to targets, not libraries, because targets can do things like set include headers, include other libraries, etc. - iirc in this case the link targets might be:if (USE_PYTHON_STABLE_ABI) target_link_libraries(${_libname} PRIVATE Python::SABIModule) else() target_link_libraries(${_libname} PRIVATE Python::Module) endif()Could be worth a try if also switching to the
Python::SABIModuleCOMPONENT?
Will try along with the change to SABIModule
Bindings/Python/setup.py line 40 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
With the CMake SABI toggle, I'd either clarify that this
setup.pyfile is SABI-only, or do something like refactor it tosetup.py.inand then useconfigure_filefrom cmake to generate a differentsetup.pyin the build dir if toggling should also apply to this file.
It feels a bit of an overkill to make setup.py.in at this point just for the potential of changing a variable, so I'll add documentation.
adamkewley
left a comment
adamkewley
left a comment
There was a problem hiding this comment.
@adamkewley reviewed 3 files and all commit messages, made 5 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on aymanhab).
CMakeLists.txt line 438 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
switched to SABIModule
@aymanhab the SABIModule should be switched based on whether it's an SABI-mode build, though! The targets should use+link to the Module for non-SABI and then SABI for SABI builds.
Bindings/Python/CMakeLists.txt line 137 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
I'd separate this from the flags that are messing around with warnings. E.g. have: codegen defs (optimizer flags like -O0, this flag), warning flags.
@aymanhab please move this away from warning flags: future developers shouldn't be looking for flags like this between -Wno-error=missing-field-initializers and -Wno-conversion-null.
Bindings/Python/CMakeLists.txt line 199 at r1 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
This fixes the
.so/.dylibthing I mentioned earlier: APPLE has no place near.so/.dylib. In fact, it's more accurate to sayUNIX AND NOT APPLEor something so that APPLE doesn't hit this code.
This still talks about replacing dylib with .so - it'd be broken on Apple
Additionally, abi.so is only valid for SABI builds, not versioned builds. I would strongly recommend ensuring OpenSim can still be built against specific Pythons if high-performance or legacy builds are desired long-term. This kind of stuff is what will break later on.
Bindings/Python/setup.py line 40 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
It feels a bit of an overkill to make setup.py.in at this point just for the potential of changing a variable, so I'll add documentation.
@aymanhab documentation isn't going to solve this problem. If you configure cmake with a different Python version, or non-SABI, then your py_limited_api number needs to also be updated (or replaced with a not-limited-api tag).
Case-in-point: this PR already has inconsistencies - even before future developers overlook this landmine comment in a few months' time:
- GH issue: Python 3.10
py_limited_api: Python 3.10- Documentation string: Python 3.11
- CI (+ CMake): Python 3.11
CMake is the single source of truth: whatever Python version was used during the configuration phase (setting Python3_VERSION_MINOR) will dictate the binary compatibility of the resulting wheel. So all compiler flags, configuration files, wheel metadata, etc. must derive from that source of truth in order to be valid. It also means that upgrading/changing Python only has to happen in one place (cmake configuration).
Otherwise, CI will produce wheel files that are labelled against the wrong version of Python. Right now, I can install this PR's wheels into Python 3.10, which may or may not explode at runtime.
Bindings/Python/CMakeLists.txt line 181 at r2 (raw file):
target_link_libraries(${_libname} PRIVATE Python3::SABIModule) else() target_link_libraries(${_libname} PRIVATE ${Python3_LIBRARIES})
You didn't fix the alternative: it should be that you link to Python3::SABIModule if you want an SABI build; otherwise, link to Python::Module. See:
…st use and expand comments/documentation
This branch has not been deployed
3 participants
Fixes issue #0
Brief summary of changes
This PR tells swig to build against the 3.10 python and use only the stable API for 3.10 so that the bindings can be used in any version 3.10+
Testing I've completed
Looking for feedback on...
CHANGELOG.md (choose one)
This change is