diff --git a/CHANGELOG.md b/CHANGELOG.md index 8627cad..a911646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,39 @@ against, and semantic-release manages only the **patch** component. ## [Unreleased] +### Added + +- **nanobind 3 is supported**, and the build requirement widens from + `nanobind>=2.0,<3` to `>=2.0,<4` + ([#85](https://github.com/stillwater-sc/mtl5-python/issues/85)). The cap added + when nanobind 3.0.0 broke every wheel build was a hold, not a fix; this is the + port. **Both majors are built and tested** — 1374 passed, 3 skipped under each + of nanobind 2.15.0 and 3.0.0. + + nanobind 3 removed the free `nb::detail::keep_alive(nurse, patient)`, routing + the same operation through a backend slot that must be handed the extension's + context pointer. `mtl5_ndarray.cpp` now spells it once, in `keep_view_alive()`, + which selects the form on `NB_VERSION_MAJOR` — so the bound moved rather than + jumped, and a source build against either major produces the same behaviour. + + The obvious simplification is wrong, and there is now a test saying so. + Replacing these calls with the declarative `nb::keep_alive<0, 1>` annotation + would compile and pass the old suite, but the annotation applies to **every** + return of a `.def`, and `__getitem__`, `reshape` and `ravel` each return a + view on one path and a copy (or, for a fully-integer index, a scalar) on + another. Pinning a large source array to an independent copy of itself is a + memory-retention bug, so the pin has to stay per-path. + + Two gaps in the lifetime tests are closed as part of this, both of which a + broken keep-alive would previously have survived: + - `test_a_view_of_an_OWNING_array_keeps_it_alive` — the existing test derives + its views from `asarray()`, where the view *also* carries the NumPy owner + object, so the buffer survives on that reference alone. A `copy()` owns its + memory with no second owner, which is the configuration where dropping the + parent is a genuine use-after-free. + - `test_a_copy_does_not_pin_its_source` — asserts the aliasing/copying split + directly, via refcount: a view increfs its parent, a copy does not. + ### Fixed - **nanobind capped below 3.0** (`nanobind>=2.0,<3`), which unbreaks every wheel diff --git a/pyproject.toml b/pyproject.toml index f30b540..4e1a0ca 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,16 +25,15 @@ # one, is what a wheel build actually resolves, and unbounded it resolved # nanobind 3.0.0 the day that released. # -# nanobind 3 is not a drop-in: it moved the internals to a backend-slot dispatch -# model, so the free function nb::detail::keep_alive(nurse, patient) that -# mtl5_ndarray.cpp uses to tie a view's lifetime to its owner is gone, replaced -# by NB_CALL(keep_alive_py)(NB_CTX, ...) which needs a context not available at -# those call sites. Lifting this cap is a real port, not a version edit; until -# someone does it, pin the major so a nanobind release cannot retroactively fail -# every build the way one already has. +# The bound is now <4, i.e. nanobind 2 AND 3 are supported and tested. It was +# <3 while mtl5_ndarray.cpp still called the free nb::detail::keep_alive that +# nanobind 3 removed; keep_view_alive() there now spells that operation for +# both majors, so the cap moved rather than disappeared. Keep an upper bound: +# nanobind 3.0.0 broke every build the day it released precisely because there +# was none. requires = [ "scikit-build-core>=0.10", - "nanobind>=2.0,<3", + "nanobind>=2.0,<4", "numpy>=1.24", "packaging>=22", "typing_extensions>=4.0; python_version < '3.11'", diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index 131c468..d23c273 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -6,7 +6,7 @@ find_package(Python REQUIRED COMPONENTS Interpreter Development.Module) # find_package almost always wins under `pip install .`, because pyproject.toml's # build requirement puts a nanobind in the build overlay. The FetchContent tag # below is the fallback for a bare CMake build with no nanobind installed -- and -# it is kept in step with the `nanobind>=2.0,<3` bound in pyproject.toml on +# it is kept in step with the `nanobind>=2.0,<4` bound in pyproject.toml on # purpose. When the two drifted (fallback v2.4.0, requirement unbounded), a local # CMake build and a CI wheel build compiled against different major versions of # nanobind, so a source change could pass locally and fail every CI job. Move @@ -17,7 +17,7 @@ if(NOT nanobind_FOUND) FetchContent_Declare( nanobind GIT_REPOSITORY https://github.com/wjakob/nanobind.git - GIT_TAG v2.15.0 + GIT_TAG v3.0.0 GIT_SHALLOW TRUE ) FetchContent_MakeAvailable(nanobind) diff --git a/python/src/mtl5_ndarray.cpp b/python/src/mtl5_ndarray.cpp index 540bdfc..e0dc99e 100644 --- a/python/src/mtl5_ndarray.cpp +++ b/python/src/mtl5_ndarray.cpp @@ -30,9 +30,16 @@ // builds the view through ndarray's (pointer, shape, strides) constructor, // which is the same primitive `slice` is built on. // -// Lifetime: every method returning a view uses nb::keep_alive<0, 1>, so the -// array it borrows from cannot be collected first. `asarray` additionally holds -// the NumPy object, which is what keeps the underlying buffer alive. +// Lifetime: a method that returns a view keeps the array it borrows from alive, +// so the parent cannot be collected first. `asarray` and the `as_ndarray` +// converters do that with nb::keep_alive<0, 1>, the declarative form. +// +// `__getitem__`, `reshape` and `ravel` cannot: each returns a view on one path +// and a copy (or, for a fully-integer index, a scalar) on another, while the +// annotation applies to every return of a `.def`. Pinning a large parent array +// to an independent copy of itself is a memory-retention bug, so those three +// call `keep_view_alive` on exactly the aliasing paths. See its comment below -- +// that split is the reason the port to nanobind 3 was not a one-line rename. #include "mtl5_types.hpp" @@ -58,6 +65,32 @@ namespace { namespace ma = mtl::array; +/// Tie `patient`'s lifetime to `nurse`: the patient cannot be collected while +/// the nurse is alive. Used where we hand back a VIEW that aliases another +/// array's memory, so the buffer cannot be freed out from under it. +/// +/// Spelled here rather than at the call sites because nanobind moved it. 2.x +/// exposed a free `nb::detail::keep_alive(nurse, patient)`; 3.x routes the same +/// operation through a backend slot that has to be handed this extension's +/// context pointer. `NB_CTX` resolves to that pointer in extension code (it is +/// only unavailable inside nanobind's own build, under NB_BUILD), so the 3.x +/// form is usable directly -- it just cannot be spelled the old way. +/// +/// Both branches are still nanobind-internal API. The alternative is the public +/// `nb::keep_alive` annotation, which does NOT fit here: it +/// applies unconditionally to every return of a `.def`, and all three callers +/// below return a view on one path and a copy (or a scalar) on another. Pinning +/// the parent to a copy would be a silent memory-retention regression -- a +/// large source array held alive by an independent copy of it -- so the +/// conditional, per-path call is the behaviour to preserve, not an accident. +inline void keep_view_alive(nb::handle nurse, nb::handle patient) { +#if defined(NB_VERSION_MAJOR) && NB_VERSION_MAJOR >= 3 + NB_CALL(keep_alive_py)(NB_CTX, nurse.ptr(), patient.ptr()); +#else + nb::detail::keep_alive(nurse.ptr(), patient.ptr()); +#endif +} + /// An ndarray plus, when it borrows NumPy memory, the object owning it. /// /// `owner` defaults to a NULL handle rather than `nb::none()`. That matters: @@ -337,7 +370,7 @@ void register_ndarray(nb::module_& m) { } NDArrayView out(ma::ndarray(ptr, sh, st), v.owner); nb::object o = nb::cast(std::move(out)); - nb::detail::keep_alive(o.ptr(), self.ptr()); + keep_view_alive(o, self); return o; }; switch (kept) { @@ -364,7 +397,7 @@ void register_ndarray(nb::module_& m) { NDArrayView out( ma::ndarray(const_cast(v.arr.data()), sh), v.owner); nb::object o = nb::cast(std::move(out)); - nb::detail::keep_alive(o.ptr(), self.ptr()); + keep_view_alive(o, self); return o; } // Not contiguous in its own order: pack into logical order first. @@ -403,7 +436,7 @@ void register_ndarray(nb::module_& m) { NDArrayView out( ma::ndarray(const_cast(v.arr.data()), sh), v.owner); nb::object o = nb::cast(std::move(out)); - nb::detail::keep_alive(o.ptr(), self.ptr()); + keep_view_alive(o, self); return o; } ma::shape<1> sh; diff --git a/tests/test_ndarray.py b/tests/test_ndarray.py index 997eb60..f753880 100644 --- a/tests/test_ndarray.py +++ b/tests/test_ndarray.py @@ -548,6 +548,62 @@ def test_numpy_source_is_kept_alive(self): gc.collect() np.testing.assert_array_equal(x.to_numpy(), arange(2, 3)) + def test_a_view_of_an_OWNING_array_keeps_it_alive(self): + """The source owns its buffer, so only the keep-alive holds it. + + The asarray() test above cannot catch a broken keep-alive: a view + derived from an asarray() array also carries the NumPy owner object, so + the buffer survives on that reference alone. An array built by copy() + owns its memory and has no second owner — if the parent is collected + without each view pinning it, every read below is a use-after-free. + + This covers all three sites that pin manually (`__getitem__`, `reshape`, + `ravel`); `transpose` is covered by the asarray test and uses the + declarative nb::keep_alive<0, 1> instead. + """ + import gc + + base = arange(4, 6) + src = A.asarray(base).copy() + assert not src.is_view, "copy() must own its buffer for this to test anything" + + sliced = src[1] + raveled = src.ravel() + reshaped = src.reshape([6, 4]) + + del src + gc.collect() + + np.testing.assert_array_equal(sliced.to_numpy(), base[1]) + np.testing.assert_array_equal(raveled.to_numpy(), base.ravel()) + np.testing.assert_array_equal(reshaped.to_numpy(), base.reshape(6, 4)) + + def test_a_copy_does_not_pin_its_source(self): + """Only the aliasing paths pin the parent — the copying paths must not. + + `__getitem__`, `reshape` and `ravel` each return a view on one path and + a copy on another, and they pin per path rather than per method. The + declarative nb::keep_alive<0, 1> annotation cannot express that: it + applies to every return, so a large source array would be held alive by + an independent copy of itself. This is the test that fails if someone + "simplifies" the manual calls into the annotation. + + Refcount is the observable: pinning increfs the parent. + """ + import sys + + src = A.asarray(arange(4, 6)).copy() + before = sys.getrefcount(src) + view = src.ravel() # C-contiguous -> view + assert view.is_view + assert sys.getrefcount(src) == before + 1, "a view must pin its source" + + nc = A.asarray(arange(4, 6)).T # non-contiguous + before_nc = sys.getrefcount(nc) + copied = nc.ravel() # cannot alias -> copy + assert not copied.is_view + assert sys.getrefcount(nc) == before_nc, "a copy must NOT pin its source" + class TestPublicSurface: def test_array_submodule_is_exported(self):