Build the Python binding with nanobind + scikit-build-core and ship bundled wheels - #136
Open
grzanka wants to merge 5 commits into
Open
Build the Python binding with nanobind + scikit-build-core and ship bundled wheels#136grzanka wants to merge 5 commits into
grzanka wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
=======================================
Coverage 72.86% 72.86%
=======================================
Files 12 12
Lines 1640 1640
Branches 299 299
=======================================
Hits 1195 1195
Misses 445 445 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous ctypes Python wrapper with a compiled nanobind extension (libdedx._core) built via scikit-build-core, and adds wheel-building/publishing automation so pip install libdedx can work from prebuilt, self-contained wheels that statically link the C library.
Changes:
- Add a new
nanobindC++ extension implementing the workspace/config object model plus vectorized NumPy entry points and accessors, and reimplement the legacy high-level helpers on top of_core. - Switch Python packaging to a root-level
pyproject.tomlusingscikit-build-core+setuptools_scm, with CI for wheel/sdist builds and Trusted Publishing to TestPyPI/PyPI. - Adjust CMake install/build logic under
SKBUILDto avoid installing the C library artifacts into wheels; update tests/docs accordingly.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds DEDX_BUILD_PYTHON and SKBUILD-specific configuration (Python-only builds, version lockstep, skip packaging). |
src/CMakeLists.txt |
Skips dedx_shared and C-library install rules under SKBUILD for wheel builds. |
python/CMakeLists.txt |
Adds CMake build for the _core nanobind module linking the static dedx target. |
python/src/dedx_core.cpp |
Implements the new libdedx._core nanobind binding (Config/Workspace + module-level APIs). |
pyproject.toml |
Root-level scikit-build-core packaging config + cibuildwheel settings and metadata updates. |
.github/workflows/wheels.yml |
Adds multi-OS wheel builds, sdist build/check, and OIDC Trusted Publishing (TestPyPI then PyPI). |
.github/workflows/ci.yml |
Updates Python job to build/install the extension via pip install -e ".[dev]" and run tests/ruff. |
python/libdedx/__init__.py |
Re-exports _core APIs and reimplements legacy convenience helpers on top of _core. |
python/libdedx/_core.pyi |
Adds type stubs for the compiled _core module. |
python/tests/test_core.py |
Adds tests for the new low-level _core API surface. |
python/tests/test_libdedx.py |
Updates high-level smoke tests to use _core constants and adds version-string checks. |
python/tests/conftest.py |
Removes shared-library discovery/env-var setup (no longer needed with static linking). |
python/README.md |
Updates Python binding documentation for nanobind/scikit-build-core usage and workflow. |
python/libdedx/_api.py |
Removes the old ctypes-backed implementation. |
python/libdedx/__init__.pyi |
Removes stubs for the old ctypes-based surface (now typed via inline annotations / _core.pyi). |
python/pyproject.toml |
Removes the old Python-subdir setuptools-based packaging config. |
.gitignore |
Ignores new packaging artifacts (dist/, wheelhouse/, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Implements the binding-technology decision from #131: the Python binding is now a nanobind extension (`libdedx._core`) built by scikit-build-core that statically links the libdedx C library. Because libdedx embeds its data and needs no GSL, the result is a single self-contained extension and trivially self-contained wheels — `pip install libdedx` works with no local C build. Binding (`python/src/dedx_core.cpp`): - Workspace/Config object model (allocate -> load -> evaluate) with safe ownership of the C config's malloc'd element arrays. - Custom compounds (Z + atoms or mass-fractions, rho, per-element I-values). - stp, CSDA, inverse STP and inverse CSDA (ion_a auto-filled on load so the object model works without the C convenience wrappers). - Unit conversion (incl. keV/um), composition / I-value / density accessors, min/max energy, program/ion/material lists and names, Bragg provenance. - numpy in/out for the vectorised entry points. Build: - Top-level pyproject.toml uses build-backend scikit_build_core.build; the repo root is the CMake source tree and the extension is added under SKBUILD, which also skips the examples/tests and the C-library install/CPack rules so the wheel only contains the module. The version is taken from setuptools_scm and fed into the C library for lockstep versioning. - pyproject.toml moved to the repo root (was python/pyproject.toml) so the C sources are inside the build context, which is required for self-contained wheels via cibuildwheel and source installs. CI: - wheels.yml builds CPython 3.9-3.14 wheels with cibuildwheel (auto64, skip PyPy/musllinux) on Linux/Windows/macOS x86_64+arm64, smoke-tests each wheel, builds an sdist, and publishes to TestPyPI then PyPI via OIDC trusted publishing on v* tags. - ci.yml python_tests now builds the extension via pip and drops LIBDEDX_SO. Also corrects the license metadata to GPL-3.0-or-later (#115).
- The sdist job's wheel-from-sdist check passed the tarball to `build --wheel`,
which only accepts a directory ("is not a directory"). Use plain `build`,
which builds the sdist and then a wheel from that sdist, proving the sdist is
self-contained. Only the sdist is uploaded.
- Add a top-level `permissions: contents: read` block so the build jobs no
longer run with the default broad GITHUB_TOKEN scope (CodeQL alerts 10/11).
The publish jobs keep their explicit id-token: write.
… dest Copilot review feedback on the nanobind binding: - Config.set_elements_id() now drops the per-element arrays (atoms / mass_fraction / i_value) when the compound length changes, so stale buffers can no longer be shorter than elements_length and read out of bounds in dedx_load_config(). - check_length() now requires elements_id to be set first and enforces an exact length match for the dependent arrays, closing the "set before id" gap. - python/CMakeLists.txt lists RUNTIME alongside LIBRARY for the _core install so the destination is correct regardless of how a generator classifies the Python extension module. Adds a test covering the new ordering/length guards.
The `Wheels on macos-13` job sat queued for 25+ minutes (Intel macOS runners are being wound down and have very limited capacity), while `macos-14` (arm64) started immediately. Build both macOS architectures on the arm64 runner instead — arm64 natively and x86_64 via cross-compilation (cibuildwheel archs) — and drop macos-13 from the matrix. Intel-Mac wheels are still produced.
The PR ships a Python package, but the top-level docs were still C-only. Add: - README.md: a "Python binding" section (pip install + quick start). - index.rst (rendered in the Sphinx docs): a "Python binding" section covering the nanobind/scikit-build-core extension and the API surface. - CONTRIBUTING.md: developer notes on building/testing the binding, the C++ conventions for the binding source, dedx_config array ownership, version lockstep, and the inherited thread-safety limitation.
grzanka
force-pushed
the
claude/issue-131-C9Tuo
branch
from
June 16, 2026 14:30
f398e80 to
8094912
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #131. Implements the binding-technology decision: the Python binding is now a nanobind extension (
libdedx._core) built by scikit-build-core that statically links the libdedx C library. Since libdedx embeds its data and needs no GSL, the result is a single self-contained extension and trivially self-contained wheels —pip install libdedxworks with no local C build.What changed
Binding —
python/src/dedx_core.cppExposes the full C API the old ctypes wrapper omitted:
malloc'd element arrays (matchesdedx_free_config).stp, CSDA, inverse STP, inverse CSDA.ion_ais auto-filled onload()(the Cload_configdoesn't set it — only the convenience wrappers do), so the object model works out of the box.convert_units(incl. keV/µm, refactor: namespace public convert_units() as dedx_convert_units() #112), composition / I-value accessors (Expose nucleon number, atom mass, density and is-gas in the public header #119), min/max energy, program/ion/material lists & names, Bragg-fallback provenance (bragg_used, Make Bragg/stoichiometric fallback transparent (surface bragg_used) and add accuracy tests #111).nb::ndarraynumpy in/out for the vectorised entry points.get_version,get_stp,get_stp_table,get_default_table,get_csda_table) are reimplemented on top of_core, keeping the historical surface.Build
pyproject.toml→build-backend = "scikit_build_core.build"; build-requiresnanobind,scikit-build-core>0.10,setuptools-scm>=8; runtime depnumpy.SKBUILD, which also skips examples/tests and the C-library install/CPack rules so the wheel contains only the module (verified: wheel ships justlibdedx/{__init__.py,_core.so,_core.pyi,py.typed}).setuptools_scmresolves the version and feeds it into the C library (SKBUILD_PROJECT_VERSION_FULL), sodedx_get_version_string()and the wheel version match.GPL-3.0-or-later(bug: license metadata mismatch — Python binding declares LGPL-2.0 but library is GPL-3.0 #115) — matchesCOPYING/the source headers (was incorrectlyLGPL-2.0-or-later).Docs
README.md,index.rst(rendered in the Sphinx site), andCONTRIBUTING.mdnow document the Python binding (install + quick start, API surface, and the developer build/test workflow, array-ownership rule, version lockstep, and inherited thread-safety limitation).CI —
.github/workflows/wheels.ymlpermissions: contents: readblock; publish jobs opt intoid-token: write.v*tags (testpypi/pypienvironments).ci.ymlpython_testsnow builds the extension viapip install -e .[dev]and drops the oldLIBDEDX_SOflow.Open decisions (from the issue) — choices made
x86_64+arm64wheels (notuniversal2), both built on the arm64macos-14runner (arm64 natively, x86_64 cross-compiled). The Intelmacos-13runner is deliberately avoided — its capacity is scarce and jobs were queuing for 25+ minutes.Verification
pip installbuild the extension; Python tests pass, ruff clean.python -m buildbuilds an sdist and then a wheel from that sdist (self-contained); installing the wheel in a clean venv imports and computes with no source tree present.cmakeconfigure/build OK, 27/27 ctest pass.Acceptance criteria
pip installfrom a wheel works with no local C build (CI covers Linux/macOS/Windows × 3.9–3.14).python/tests/test_core.py.setuptools_scm.Refines #117 · relates to #112, #115, #119, #111.
https://claude.ai/code/session_01LH8M1RBJBdd3x69Fgu76No
Generated by Claude Code