From 4b3006ba9f97eed1008227f2aa08cb8dc94c9a78 Mon Sep 17 00:00:00 2001 From: Rick Poyner Date: Tue, 5 Nov 2024 11:22:55 -0500 Subject: [PATCH] [pydrake] Add function to make arbitrary ref cycles This patch adds internal::make_arbitrary_ref_cycle(), which will prove useful for awkward bindings like those involving multiple return values. While in the neighborhood, it also adds a few previously missing tests. --- bindings/pydrake/common/ref_cycle_pybind.cc | 68 ++++++++++++------- bindings/pydrake/common/ref_cycle_pybind.h | 9 +++ .../pydrake/common/test/ref_cycle_test.py | 18 ++++- .../common/test/ref_cycle_test_util_py.cc | 14 ++++ 4 files changed, 83 insertions(+), 26 deletions(-) diff --git a/bindings/pydrake/common/ref_cycle_pybind.cc b/bindings/pydrake/common/ref_cycle_pybind.cc index beb9d50c7acc..3186226b99ec 100644 --- a/bindings/pydrake/common/ref_cycle_pybind.cc +++ b/bindings/pydrake/common/ref_cycle_pybind.cc @@ -44,6 +44,32 @@ void make_ref_cycle(handle p0, handle p1) { make_link(p1, p0); } +void check_and_make_ref_cycle(size_t peer0, handle p0, size_t peer1, handle p1, + std::function not_gc_message_function) { + // Returns false if the handle's value is None. Throws if the handle's value + // is not of a garbage-collectable type. + auto check_handle = [&](size_t n, handle p) -> bool { + if (p.is_none()) { + return false; + } + // Among the reasons the following check may fail is that one of the + // participating pybind11::class_ types does not declare + // pybind11::dynamic_attr(). + if (!PyType_IS_GC(Py_TYPE(p.ptr()))) { + py::pybind11_fail(not_gc_message_function(n)); + } + return true; + }; + if (!check_handle(peer0, p0) || !check_handle(peer1, p1)) { + // At least one of the handles is None. We can't construct a ref-cycle, but + // neither should we complain. A None variable value could happen for any + // number of legitimate reasons, and does not mean that the ref_cycle call + // policy is defective. + return; + } + make_ref_cycle(p0, p1); +} + } // namespace void ref_cycle_impl( @@ -67,32 +93,24 @@ void ref_cycle_impl( handle p0 = get_arg(peer0); handle p1 = get_arg(peer1); - // Returns false if the handle's value is None. Throws if the handle's value - // is not of a garbage-collectable type. - auto check_handle = [&](size_t n, handle p) -> bool { - if (p.is_none()) { - return false; - } - // Among the reasons the following check may fail is that one of the - // participating pybind11::class_ types does not declare - // pybind11::dynamic_attr(). - if (!PyType_IS_GC(Py_TYPE(p.ptr()))) { - py::pybind11_fail(fmt::format( - "Could not activate ref_cycle: object type at index {} for " - "function '{}' is not tracked by garbage collection. Was the object " - "defined with `pybind11::class_<...>(... pybind11::dynamic_attr())`?", - n, call.func.name)); - } - return true; + auto not_gc_error = [&call](size_t n) -> std::string { + return fmt::format( + "Could not activate ref_cycle: object type at index {} for " + "function '{}' is not tracked by garbage collection.", + n, call.func.name); }; - if (!check_handle(peer0, p0) || !check_handle(peer1, p1)) { - // At least one of the handles is None. We can't construct a ref-cycle, but - // neither should we complain. A None variable value could happen for any - // number of legitimate reasons, and does not mean that the ref_cycle call - // policy is defective. - return; - } - make_ref_cycle(p0, p1); + check_and_make_ref_cycle(peer0, p0, peer1, p1, not_gc_error); +} + +void make_arbitrary_ref_cycle( + handle p0, handle p1, const std::string& location_hint) { + auto not_gc_error = [&location_hint](size_t n) -> std::string { + return fmt::format( + "Could not activate arbitrary ref_cycle: object type at argument {} " + "for binding at '{}' is not tracked by garbage collection.", + n, location_hint); + }; + check_and_make_ref_cycle(0, p0, 1, p1, not_gc_error); } } // namespace internal diff --git a/bindings/pydrake/common/ref_cycle_pybind.h b/bindings/pydrake/common/ref_cycle_pybind.h index 930f4056f284..d4ba44648ff9 100644 --- a/bindings/pydrake/common/ref_cycle_pybind.h +++ b/bindings/pydrake/common/ref_cycle_pybind.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "drake/bindings/pydrake/pydrake_pybind.h" namespace drake { @@ -51,6 +53,13 @@ struct ref_cycle {}; void ref_cycle_impl(size_t peer0, size_t peer1, const py::detail::function_call& call, py::handle ret); +/* This function constructs a reference cycle from arbitrary handles. It may be + needed in special cases where the ordinary call-policy annotations won't work. + The `location_hint` will appear in any exception messages; it should help + developers locate where and why this function was called. */ +void make_arbitrary_ref_cycle( + py::handle p0, py::handle p1, const std::string& location_hint); + } // namespace internal } // namespace pydrake } // namespace drake diff --git a/bindings/pydrake/common/test/ref_cycle_test.py b/bindings/pydrake/common/test/ref_cycle_test.py index a075800d7913..987a6a7c0537 100644 --- a/bindings/pydrake/common/test/ref_cycle_test.py +++ b/bindings/pydrake/common/test/ref_cycle_test.py @@ -7,7 +7,8 @@ import weakref from pydrake.common.ref_cycle_test_util import ( - NotDynamic, IsDynamic, invalid_arg_index, free_function, ouroboros) + arbitrary_bad, arbitrary_ok, free_function, invalid_arg_index, IsDynamic, + NotDynamic, ouroboros) from pydrake.common.test_utilities.memory_test_util import actual_ref_count @@ -56,12 +57,27 @@ def test_ouroboros(self): self.assertEqual(len(dut._pydrake_internal_ref_cycle_peers), 1) self.check_is_collectable_cycle(returned, dut) + def test_arbitrary_ok(self): + got = arbitrary_ok() + self.assertTrue(hasattr(got, '_pydrake_internal_ref_cycle_peers')) + + def test_arbitrary_bad(self): + with self.assertRaisesRegex(RuntimeError, ".*from arbitrary_bad.*"): + arbitrary_bad() + def test_free_function(self): p0 = IsDynamic() p1 = IsDynamic() free_function(p0, p1) self.check_is_collectable_cycle(p0, p1) + def test_init_cycle(self): + # Cover the case where index 1 refers to the `self` of a py::init<>() + # binding. + other = IsDynamic() + dut = IsDynamic(other) + self.check_is_collectable_cycle(dut, other) + def test_not_dynamic_add(self): dut = NotDynamic() peer = IsDynamic() diff --git a/bindings/pydrake/common/test/ref_cycle_test_util_py.cc b/bindings/pydrake/common/test/ref_cycle_test_util_py.cc index 4591ba81a0ff..09e3d5ad6f6f 100644 --- a/bindings/pydrake/common/test/ref_cycle_test_util_py.cc +++ b/bindings/pydrake/common/test/ref_cycle_test_util_py.cc @@ -22,6 +22,7 @@ class TestDummyBase { TestDummyBase(const TestDummyBase&) = default; ~TestDummyBase() = default; + explicit TestDummyBase(IsDynamic*) {} void AddNot(NotDynamic*) {} NotDynamic* ReturnNot() { return new NotDynamic(); } NotDynamic* ReturnNullNot() { return nullptr; } @@ -50,6 +51,7 @@ PYBIND11_MODULE(ref_cycle_test_util, m) { using Class = IsDynamic; py::class_(m, "IsDynamic", py::dynamic_attr()) .def(py::init<>()) + .def(py::init(), py::arg("thing"), ref_cycle<1, 2>()) .def("AddNot", &Class::AddNot) .def("AddNotCycle", &Class::AddNot, ref_cycle<1, 2>()) .def("ReturnNot", &Class::ReturnNot) @@ -71,6 +73,18 @@ PYBIND11_MODULE(ref_cycle_test_util, m) { // Returns its argument and creates a self-cycle. m.def( "ouroboros", [](IsDynamic* x) { return x; }, ref_cycle<0, 1>()); + m.def("arbitrary_ok", []() { + auto d1 = py::cast(new IsDynamic); + auto d2 = py::cast(new IsDynamic); + internal::make_arbitrary_ref_cycle(d1, d2, "from arbitrary_ok"); + return d1; + }); + m.def("arbitrary_bad", []() { + auto dyn = py::cast(new IsDynamic); + auto bad = py::cast(new NotDynamic); + internal::make_arbitrary_ref_cycle(dyn, bad, "from arbitrary_bad"); + return dyn; + }); } } // namespace pydrake