Mega-commit for cherry-picking ease. Fix joint C++/python object handling, ensure __str__ takes no parameters, test to_string methods

Author: jmalkin

CMakeLists.txt

@@ -74,7 +74,7 @@ execute_process(
 list(APPEND CMAKE_PREFIX_PATH "${NB_DIR}")
 find_package(nanobind CONFIG REQUIRED)
 
-nanobind_add_module(python MODULE LTO STABLE_ABI NB_STATIC)
+nanobind_add_module(python MODULE LTO NOMINSIZE STABLE_ABI NB_STATIC)
 
 set_target_properties(python PROPERTIES
   PREFIX ""

include/kernel_function.hpp

@@ -20,7 +20,8 @@
 #include <nanobind/nanobind.h>
 #include <nanobind/trampoline.h>
 #include <nanobind/ndarray.h>
-#include <nanobind/stl/shared_ptr.h>
+#include <nanobind/intrusive/counter.h>
+#include <nanobind/intrusive/ref.h>
 #include <nanobind/stl/vector.h>
 
 #include <numpy/arrayobject.h>
@@ -37,7 +38,7 @@ namespace datasketches {
  *        which native Python kernels ultimately inherit. The actual
  *        kernels implement KernelFunction, as shown in KernelFunction.py
  */
-struct kernel_function {
+struct kernel_function : public nb::intrusive_base {
   virtual double operator()(nb::handle& a, nb::handle& b) const = 0;
   virtual ~kernel_function() = default;
 };
@@ -71,7 +72,7 @@ struct KernelFunction : public kernel_function {
  * never need to use this directly.
  */
 struct kernel_function_holder {
-  explicit kernel_function_holder(std::shared_ptr<kernel_function> kernel) : _kernel(kernel) {}
+  explicit kernel_function_holder(kernel_function* kernel) : _kernel(kernel) {}
   kernel_function_holder(const kernel_function_holder& other) : _kernel(other._kernel) {}
   kernel_function_holder(kernel_function_holder&& other) : _kernel(std::move(other._kernel)) {}
   kernel_function_holder& operator=(const kernel_function_holder& other) { _kernel = other._kernel; return *this; }
@@ -99,7 +100,7 @@ struct kernel_function_holder {
   }
 
   private:
-    std::shared_ptr<kernel_function> _kernel;
+    nb::ref<kernel_function> _kernel;
 };
 
 }

include/tuple_policy.hpp

@@ -20,6 +20,8 @@
 #include <memory>
 #include <nanobind/nanobind.h>
 #include <nanobind/trampoline.h>
+#include <nanobind/intrusive/counter.h>
+#include <nanobind/intrusive/ref.h>
 
 #ifndef _TUPLE_POLICY_HPP_
 #define _TUPLE_POLICY_HPP_
@@ -33,10 +35,10 @@ namespace datasketches {
  *        which native Python policies ultimately inherit. The actual
  *        policies implement TuplePolicy, as shown in TuplePolicy.py
  */
-struct tuple_policy {
-  virtual nb::object create_summary() = 0;
-  virtual nb::object update_summary(nb::object& summary, const nb::object& update) = 0;
-  virtual nb::object operator()(nb::object& summary, const nb::object& update) = 0;
+struct tuple_policy : public nb::intrusive_base {
+  virtual nb::object create_summary() const = 0;
+  virtual nb::object update_summary(nb::object& summary, const nb::object& update) const = 0;
+  virtual nb::object operator()(nb::object& summary, const nb::object& update) const = 0;
   virtual ~tuple_policy() = default;
 };
 
@@ -53,7 +55,7 @@ struct TuplePolicy : public tuple_policy {
    * 
    * @return nb::object representing a new summary
    */
-  nb::object create_summary() override {
+  nb::object create_summary() const override {
     NB_OVERRIDE_PURE(
       create_summary,      // Name of function in C++ (must match Python name)
                            // Argument(s) -- if any
@@ -67,7 +69,7 @@ struct TuplePolicy : public tuple_policy {
    * @param update The new value with which to update the summary
    * @return nb::object The updated summary
    */
-  nb::object update_summary(nb::object& summary, const nb::object& update) override {
+  nb::object update_summary(nb::object& summary, const nb::object& update) const override {
     NB_OVERRIDE_PURE(
       update_summary,      // Name of function in C++ (must match Python name)
       summary, update      // Arguments
@@ -81,7 +83,7 @@ struct TuplePolicy : public tuple_policy {
    * @param update An update to apply to the current summary
    * @return nb::object The potentially modified summary
    */
-  nb::object operator()(nb::object& summary, const nb::object& update) override {
+  nb::object operator()(nb::object& summary, const nb::object& update) const override {
     NB_OVERRIDE_PURE_NAME(
       "__call__",          // Name of function in python
       operator(),          // Name of function in C++
@@ -96,7 +98,7 @@ struct TuplePolicy : public tuple_policy {
  * never need to use this directly.
  */
 struct tuple_policy_holder {
-  explicit tuple_policy_holder(std::shared_ptr<tuple_policy> policy) : _policy(policy) {}
+  explicit tuple_policy_holder(tuple_policy* policy) : _policy(policy) {}
   tuple_policy_holder(const tuple_policy_holder& other) : _policy(other._policy) {}
   tuple_policy_holder(tuple_policy_holder&& other) : _policy(std::move(other._policy)) {}
   tuple_policy_holder& operator=(const tuple_policy_holder& other) { _policy = other._policy; return *this; }
@@ -113,7 +115,7 @@ struct tuple_policy_holder {
   }
 
   private:
-    std::shared_ptr<tuple_policy> _policy;
+    nb::ref<tuple_policy> _policy;
 };
 
 /* A degenerate policy used to enable Jaccard Similarity on tuple sketches,

src/count_wrapper.cpp

@@ -50,7 +50,7 @@ void bind_count_min_sketch(nb::module_ &m, const char* name) {
                 "with 95% confidence, frequency estimates satisfy the 'relative_error' guarantee. "
                 "Returns the number of hash functions that are required in order to achieve the specified "
                 "confidence of the sketch. confidence = 1 - delta, with delta denoting the sketch failure probability.")
-    .def("__str__", &count_min_sketch<W>::to_string,
+    .def("__str__", [](const count_min_sketch<W>& sk) { return sk.to_string(); },
          "Produces a string summary of the sketch")
     .def("to_string", &count_min_sketch<W>::to_string,
          "Produces a string summary of the sketch")

src/cpc_wrapper.cpp

@@ -39,7 +39,7 @@ void init_cpc(nb::module_ &m) {
          ":type seed: int, optional"
     )
     .def("__copy__", [](const cpc_sketch& sk){ return cpc_sketch(sk); })
-    .def("__str__", &cpc_sketch::to_string,
+    .def("__str__", [](const cpc_sketch& sk) { return sk.to_string(); },
          "Produces a string summary of the sketch")
     .def("to_string", &cpc_sketch::to_string,
          "Produces a string summary of the sketch")

src/datasketches.cpp

@@ -18,6 +18,11 @@
  */
 
 #include <nanobind/nanobind.h>
+#include <nanobind/intrusive/counter.h>
+
+// needed for sketches such as Density and Tuple which rely
+// on a joint C++/Python object
+#include <nanobind/intrusive/counter.inl>
 
 namespace nb = nanobind;
 
@@ -41,6 +46,18 @@ void init_kolmogorov_smirnov(nb::module_& m);
 void init_serde(nb::module_& m);
 
 NB_MODULE(_datasketches, m) {
+  // needed in conjunction with the counter.inl include above
+  nb::intrusive_init(
+    [](PyObject *o) noexcept {
+        nb::gil_scoped_acquire guard;
+        Py_INCREF(o);
+    },
+    [](PyObject *o) noexcept {
+        nb::gil_scoped_acquire guard;
+        Py_DECREF(o);
+    }
+  );
+
   init_hll(m);
   init_kll(m);
   init_fi(m);

src/density_wrapper.cpp

@@ -20,7 +20,8 @@
 #include <vector>
 #include <memory>
 #include <nanobind/nanobind.h>
-#include <nanobind/stl/shared_ptr.h>
+#include <nanobind/intrusive/counter.h>
+#include <nanobind/stl/string.h>
 #include <nanobind/stl/vector.h>
 #include <nanobind/make_iterator.h>
 
@@ -38,7 +39,7 @@ void bind_density_sketch(nb::module_ &m, const char* name) {
   using namespace datasketches;
 
   nb::class_<density_sketch<T, K>>(m, name)
-    .def("__init__", [](density_sketch<T, K>* sk, uint16_t k, uint32_t dim, std::shared_ptr<kernel_function> kernel)
+    .def("__init__", [](density_sketch<T, K>* sk, uint16_t k, uint32_t dim, kernel_function* kernel)
         { K holder(kernel);
           new (sk) density_sketch<T, K>(k, dim, holder);
         },
@@ -67,7 +68,7 @@ void bind_density_sketch(nb::module_ &m, const char* name) {
         "Returns True if the sketch is in estimation mode, otherwise False")
     .def("get_estimate", &density_sketch<T, K>::get_estimate, nb::arg("point"),
         "Returns an approximate density at the given point")
-    .def("__str__", &density_sketch<T, K>::to_string, nb::arg("print_levels")=false, nb::arg("print_items")=false,
+    .def("__str__", [](const density_sketch<T, K>& sk) { return sk.to_string(); },
         "Produces a string summary of the sketch")
     .def("to_string", &density_sketch<T, K>::to_string, nb::arg("print_levels")=false, nb::arg("print_items")=false,
         "Produces a string summary of the sketch")
@@ -87,7 +88,7 @@ void bind_density_sketch(nb::module_ &m, const char* name) {
     )
     .def_static(
         "deserialize",
-        [](const nb::bytes& bytes, std::shared_ptr<kernel_function> kernel) {
+          [](const nb::bytes& bytes, kernel_function* kernel) {
           K holder(kernel);
           return density_sketch<T, K>::deserialize(bytes.c_str(), bytes.size(), holder);
         },
@@ -108,6 +109,8 @@ void init_density(nb::module_ &m) {
 
   // generic kernel function
   nb::class_<kernel_function, KernelFunction>(m, "KernelFunction",
+      nb::intrusive_ptr<kernel_function>(
+      [](kernel_function *kf, PyObject *po) noexcept { kf->set_self_py(po); }),
      "A generic base class from which user-defined kernels must inherit.")
     .def(nb::init())
     .def("__call__", &kernel_function::operator(), nb::arg("a"), nb::arg("b"),

src/ebpps_wrapper.cpp

@@ -39,7 +39,7 @@ void bind_ebpps_sketch(nb::module_ &m, const char* name) {
          ":param k: Maximum number of samples in the sketch\n:type k: int\n"
          )
     .def("__copy__", [](const ebpps_sketch<T>& sk){ return ebpps_sketch<T>(sk); })
-    .def("__str__", &ebpps_sketch<T>::to_string,
+    .def("__str__", [](const ebpps_sketch<T>& sk) { return sk.to_string(); },
          "Produces a string summary of the sketch")
     .def("to_string",
          [](const ebpps_sketch<T>& sk, bool print_items) {

src/fi_wrapper.cpp

@@ -52,7 +52,7 @@ void bind_fi_sketch(nb::module_ &m, const char* name) {
          ":type lg_max_k: int\n"
          )
     .def("__copy__", [](const frequent_items_sketch<T, W, H, E>& sk){ return frequent_items_sketch<T,W,H,E>(sk); })
-    .def("__str__", &frequent_items_sketch<T, W, H, E>::to_string, nb::arg("print_items")=false,
+    .def("__str__", [](const frequent_items_sketch<T, W, H, E>& sk) { return sk.to_string(); },
          "Produces a string summary of the sketch")
     .def("to_string", &frequent_items_sketch<T, W, H, E>::to_string, nb::arg("print_items")=false,
          "Produces a string summary of the sketch")

src/hll_wrapper.cpp

@@ -44,7 +44,7 @@ void init_hll(nb::module_ &m) {
          "HLL_8) at the cost of much higher initial memory use. Default (and recommended) is False.\n"
          ":type start_full_size: bool"
      )
-    .def("__str__", (std::string (hll_sketch::*)(bool,bool,bool,bool) const) &hll_sketch::to_string,
+    .def("__str__", [](const hll_sketch& sk) { return sk.to_string(); },
          "Produces a string summary of the sketch")
     .def("to_string", (std::string (hll_sketch::*)(bool,bool,bool,bool) const) &hll_sketch::to_string,
          nb::arg("summary")=true, nb::arg("detail")=false, nb::arg("aux_detail")=false, nb::arg("all")=false,