Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is clone pattern supported with classes overloaded in python? #1049

Open
yesint opened this issue Aug 31, 2017 · 21 comments
Open

Is clone pattern supported with classes overloaded in python? #1049

yesint opened this issue Aug 31, 2017 · 21 comments
Labels
docs Docs or GitHub info

Comments

@yesint
Copy link
Contributor

yesint commented Aug 31, 2017

In C++ clone pattern is used in many APIs:

class A {
public:
   A();
   A(const A& other)
   virtual void func()=0;
   virtual A* clone() const { return new A(*this);  }
}

class A_trampoline: public A {
 // Usual things for pure virtual func()
}

// function dependent on clone functionality
void work_on_many_instances(const shared_ptr<A>& inst){
    std::vector<shared_ptr<A>> tasks;
    for(int i=0; i<100; ++i) tasks.push_back( shared_ptr<A>(inst->clone()) );
    // Do something with clonned instances
}

Suppose one extends class A in python using standard trampoline facilities and exposes it as usual:

py::class_<A,A_trampoline,std::shared_ptr<A>>(m, "A") 
....
;

// Accepts PyObject with class derived in python from A
m.def("work_on_many_instances", [](const py::object& o){
        auto p = o.cast<shared_ptr<A>>();
        work_on_many_instances(p); // UPS! clone will try to create A, not the derived class...
   });

When clone() is called inside work_on_many_instances() it creates an instance of A, which is an abstract class and, of course, fails. Unless I'm missing something obvious I can't figure out how to clone an object of derived class correctly. As far as I understand the trampoline should make a deep copy of existing python derived class instance on C++ side somehow but I have absolutely no idea how. Is it possible at all?

@YannickJadoul
Copy link
Collaborator

Isn't the whole point of a virtual clone() just to avoid this, i.e. to have a polymorphic copy-constructor that you can override in the derived classes?

Why don't you just add virtual A_trampoline *clone() const { return new A_trampoline(*this); } to the trampoline class?

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

Yes, this is what I did initially. Unfortunately it doesn't work:

what(): Tried to call pure virtual function "A::func"

That is what I can't understand, actually.

@YannickJadoul
Copy link
Collaborator

When exactly does that happen? You have implemented a non-pure A_trampoline::func, right?

@YannickJadoul
Copy link
Collaborator

Wait, I'm now wondering if o.cast<shared_ptr<A>>(); is the right thing to do. Since there will be a shared_ptr<A_trampoline> inside, no?

@dean0x7d
Copy link
Member

@yesint Can you post a complete code example (both C++ and Python sides) which reproduces the error that you're seeing?

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

@dean0x7d Here is minimal example to reproduce:


//-----------------------------
#include <pybind11/pybind11.h>

using namespace std;
namespace py = pybind11;

class A {
public:
   A(){}
   A(const A& other){}
   virtual void func()=0;
   virtual A* clone() const =0;
};

class A_trampoline: public A {
    using A::A;

    void func() override {
        PYBIND11_OVERLOAD_PURE(
            void, /* Return type */
            A,      /* Parent class */
            func,          /* Name of function in C++ (must match Python name) */
                          /* Argument(s) */
        );
    }

    virtual A_trampoline* clone() const {
        return new A_trampoline(*this);
    }
};

// function dependent on clone functionality
void work_on_many_instances(const shared_ptr<A>& inst){
    std::vector<shared_ptr<A>> tasks;
    for(int i=0; i<100; ++i) tasks.push_back( shared_ptr<A>(inst->clone()) );
    for(int i=0; i<100; ++i) tasks[i]->func(); // Call python overload for all instances
}


PYBIND11_MODULE(module1, m) {

    py::class_<A,A_trampoline,std::shared_ptr<A>>(m, "A")
            .def(py::init<>())
            .def("func",&A::func)
    ;

    // Accepts PyObject with class derived in python from A
    m.def("work_on_many_instances", [](const py::object& o){
            auto p = o.cast<shared_ptr<A>>();  //shared_ptr<A_trampoline> gives the same error
            work_on_many_instances(p); // UPS! clone will try to create A, not the derived class...
       });
}

And the python code to test:

from module1 import *

class B(A):
    def func(self):
        print "hi!"
        
inst = B()
work_on_many_instances(inst)

The error:

$ python test_1.py 
Traceback (most recent call last):
  File "test_1.py", line 8, in <module>
    work_on_many_instances(inst)
RuntimeError: Tried to call pure virtual function "A::func"

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

Wait, I'm now wondering if o.cast<shared_ptr>(); is the right thing to do. Since there will be a shared_ptr<A_trampoline> inside, no?

Unless I call clone() this cast gives me pointer to correct object, which is able to call python-derived func():

m.def("work_on_many_instances", [](const py::object& o){
            auto p = o.cast<shared_ptr<A>>();
            p->func();           
       });

// prints: hi! when called from python

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

Correct me if I'm wrong (I don't know internals of pybind11) but it seems that when clone() is called only C++ A_trampoline object is created but not the B instance on the python side. As a result the trampoline redirects the virtual call to nothing ending up in calling pure-virtual A::func(). So the idea as far as I understand is to create B instance in python interpreter as usual and access it from A_trampoline::clone instead of allocating A_trampoline at C++ side. But I have no idea how to implement this.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Aug 31, 2017

Oh, yes, of course! You are right; I'd failed to notice that complexity!
So the A_trampoline::func() does get called, but there's nothing it can do because it's indeed not a B...

One option is then of course to override the clone() in the same way on the Python side, but that somehow does not feel very Pythonic?

Another option is to use the type of the actual Python object like this:

    virtual A_trampoline* clone() const {
	auto this_object = py::cast(this);
        return py::cast<A_trampoline*>(this_object.get_type()(this_object).release());
    }

and then add

            .def(py::init_alias<const A_trampoline &>())

The downside of this approach is that when you implement __init__ on the Python derived class, that it should also have a 'copy constructor overload' that's cascaded back to A.

EDIT:
Either way, I've just tested the latter approach, and that does seem to work for me.

@YannickJadoul
Copy link
Collaborator

Wait, actually, this might be nastier than it seemed at first. Because ... where is the actual Python object now stored (including its attributes)? I guess this either means something's leaking, or that call to func() is unsafe?

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

Well, this black magic works :) At least the program now behaves as expected.
However I don't quit understand the point with init:

from module1 import *

class B(A):
    
    def __init__(self):
        A.__init__(self)
        self.x = 1
        
    def func(self):
        print "hi!",self.x
        
inst = B()
work_on_many_instances(inst)

Gives:

$ python test_1.py 
Traceback (most recent call last):
  File "test_1.py", line 12, in <module>
    work_on_many_instances(inst)
TypeError: __init__() takes exactly 1 argument (2 given)

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Aug 31, 2017

Yeah, there's the thing with this_object.get_type()(this_object), which is a call to that constructor of B, but with the object as argument.

Another option would to try and see if you can replace this copy-constructor thing with Python's copy.copy() or copy.deepcopy() ?

Edit: just doing this works, btw:

     def __init__(self, *args):
        A.__init__(self, *args)

@YannickJadoul
Copy link
Collaborator

But also, I'm still not convinced that this py::cast<A_trampoline*>(<...>.release()); is ok. Maybe @dean0x7d has a better idea of what would be happening inside pybind11, and why this approach magically doesn't loose the Python object's attributes?

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

Well, it seems that the attributes are not copied and have to be assigned in python-side "copy-constructor":

def __init__(self,other=None):
        if other:
            A.__init__(self,other)
            self.x = other.x # Without this x is not cloned
        else:
            A.__init__(self)
            self.x = 0

I'm not familiar with deepcopy() enough to figure out how to use it in __init__ but in the case of deepcopy what will happen with C++ object base?

@yesint
Copy link
Contributor Author

yesint commented Aug 31, 2017

Btw, both copy() and deepcopy() fails on B:


inst = B()
copy.deepcopy(inst)

> Traceback (most recent call last):
  File "test_1.py", line 21, in <module>
    copy.deepcopy(inst)
  File "/usr/lib/python2.7/copy.py", line 190, in deepcopy
    y = _reconstruct(x, rv, 1, memo)
  File "/usr/lib/python2.7/copy.py", line 329, in _reconstruct
    y = callable(*args)
  File "/usr/lib/python2.7/copy_reg.py", line 93, in __newobj__
    return cls.__new__(cls, *args)
TypeError: pybind11_object.__new__(B) is not safe, use object.__new__()

@YannickJadoul
Copy link
Collaborator

I'm not familiar with deepcopy() enough to figure out how to use it in __init__ but in the case of deepcopy what will happen with C++ object base?

I don't really know either, but you can implement those by using __copy__ and __deepcopy__. However, then you're probably +- back to an alternative version of clone().

Btw, both copy() and deepcopy() fails on B:

I do not really know what that error message is about. But the memory allocation and initialization of pybind11 is not that easy, so I guess the integration with actual Python objects is not that clear-cut :-/

@dean0x7d
Copy link
Member

dean0x7d commented Sep 1, 2017

@yesint
Correct me if I'm wrong (I don't know internals of pybind11) but it seems that when clone() is called only C++ A_trampoline object is created but not the B instance on the python side. As a result the trampoline redirects the virtual call to nothing ending up in calling pure-virtual A::func(). So the idea as far as I understand is to create B instance in python interpreter as usual and access it from A_trampoline::clone instead of allocating A_trampoline at C++ side.

Yes, that's exactly it.

@YannickJadoul
return py::cast<A_trampoline*>(this_object.get_type()(this_object).release());

One issue here is that the reference of the newly created object is leaked following .release(), i.e. the clone will live forever. Another problem is that obj.cast<T*> returns a non-owning pointer, so you're not allowed to delete it, but T* clone() expects exactly that.

Given the requirement to keep the Python state alive, I think the best solution here is to make clone() return a std::shared_ptr instead of a raw pointer, and then stash the Python state using std::shared_ptr's aliasing constructor. See the example below:

#include <pybind11/pybind11.h>
namespace py = pybind11;

class A {
public:
    A() = default;
    A(const A&) {}
    virtual ~A() = default;

    virtual void func() = 0;
    virtual std::shared_ptr<A> clone() const = 0;
};

class A_trampoline: public A {
public:
    using A::A;
    A_trampoline(const A& a) : A(a) {}

    void func() override { PYBIND11_OVERLOAD_PURE(void, A, func,); }

    std::shared_ptr<A> clone() const override {
        auto self = py::cast(this);
        auto cloned = self.attr("clone")();

        auto keep_python_state_alive = std::make_shared<py::object>(cloned);
        auto ptr = cloned.cast<A_trampoline*>();

        // aliasing shared_ptr: points to `A_trampoline* ptr` but refcounts the Python object
        return std::shared_ptr<A>(keep_python_state_alive, ptr);
    }
};

PYBIND11_MODULE(example, m) {
    py::class_<A, A_trampoline, std::shared_ptr<A>>(m, "A")
        .def(py::init<>())
        .def(py::init<const A&>())
        .def("func", &A::func);

    m.def("call", [](const std::shared_ptr<A>& inst){
        inst->func();
        inst->clone()->func();
    });
}
import example as m


class B(m.A):
    def __init__(self):
        m.A.__init__(self)
        self.x = 0

    def func(self):
        print("hi!", self.x)

    def clone(self):
        print("clone")
        # create a new object without initializing it
        cloned = B.__new__(B)
        # clone C++ state
        m.A.__init__(cloned, self)
        # clone Python state
        cloned.__dict__.update(self.__dict__)
        return cloned


inst = B()
inst.x = 1
m.call(inst)

@yesint
Copy link
Contributor Author

yesint commented Sep 2, 2017

Wow! That is indeed much more involved that I thought. Thank you very much, @dean0x7d !
Probably it worth adding this example somewhere to documentation?

@YannickJadoul
Copy link
Collaborator

Nice, I had not idea about the aliasing constructor of an std::shared_ptr.

Now I'm just wondering, if you want to make this transparent to the Python side, could you maybe still move the

cloned = B.__new__(B)
# clone C++ state
m.A.__init__(cloned, self)
# clone Python state
cloned.__dict__.update(self.__dict__)

part into the A_trampoline::clone() implementation?

@dean0x7d
Copy link
Member

dean0x7d commented Sep 6, 2017

@YannickJadoul Yes, that part could definitely be moved into C++. At that point, I guess pybind11 might be able to offer some utility to make that whole procedure easier and it might also be interesting to consider plugging into Python's __copy__ and __deepcopy__.

I think this should be documented but not exactly in the complicated form above. This needs to cook a bit more, to be easier to use, but I don't think I'll have time to develop it much more. If anyone would like to look into this and refine it further, feel free.

@franzpoeschel
Copy link
Contributor

I am currently trying to implement a very similar workflow, and I've found a solution that nearly does what is described in this issue (with a unique pointer instead of a shared pointer), except that the reference counting somehow goes wrong and I don't understand why.

(If I should rather create a new issue, do tell me, but I feel like this belongs here)

The basic idea is the same as above, I have a virtual cloneable C++ class and its Python trampoline (full code at the end):

struct virtual_base
{
    virtual void call() = 0;
    virtual std::unique_ptr<virtual_base> clone() const = 0;
};

struct py_virtual_base
    : virtual_base
{
    void call() override;
    std::unique_ptr<virtual_base> clone() const override;
};

And then define a child class on the Python side:

class child_virtual(po.virtual_base):
    def __init__(self):
        super().__init__()
    
    def call(self):
        print("Called child class")

Now, as far as my understanding of the pybind11 object model goes, this will create a Python object that owns an instance of py_virtual_base, creating a one-to-one mapping between Python and C++, where Python is the master.
Cloning that C++ object naively will then lose the relation to the Python object.
So, what I did instead, was to optionally store the handle to the Python object in the trampoline class. If the handle is not there, then the current C++ object is the one owned directly by the original Python object, otherwise the Python object is behind the handle:

    struct OriginalInstance
    {
        py::handle pythonObject;

        ~OriginalInstance()
        {
            pythonObject.dec_ref();
        }
    };
    /*
     * If the shared pointer is empty, this object is the original object owned
     * by Python and the Python handle can be acquired by:
     * py::cast(static_cast<ChildPy const *>(this))
     *
     * Copied instances will refer to the Python object handle via this member.
     * By only storing this member in copied instances, but not in the original
     * instance, we avoid a memory cycle and ensure clean destruction.
     */
    std::shared_ptr<OriginalInstance> m_originalInstance;

This design allows using unique pointers and still avoids memory loops. The Python object will wait for the C++ objects to deallocate.
However, for some reason the reference counting goes wrong. Here is the full code of the C++ minimal example:

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>
#include <pybind11/stl_bind.h>

#include <iostream>
#include <memory>

namespace py = pybind11;

/*
 * py_virtual_base is the C++ representation for objects created in Python.
 * One challenge about these classes is that they cannot be easily copied or
 * moved in memory, as the clone will lose the relation to the Python object.
 * This class has a clone_impl() method that child classes can use for cloning
 * the object and at the same time storing a reference to the original Python
 * object.
 * The template parameters ChildCpp and ChildPy implement a CRT-like pattern,
 * split into a C++ class and a Python trampoline class as documented here:
 * https://pybind11.readthedocs.io/en/stable/advanced/classes.html?highlight=trampoline#overriding-virtual-functions-in-python
 *
 * A typical child instantiation would look like:
 * struct ChildPy : ChildCpp, ClonableTrampoline<ChildCpp, ChildPy>;
 */
template <typename ChildCpp, typename ChildPy>
struct ClonableTrampoline
{
    struct OriginalInstance
    {
        py::handle pythonObject;

        ~OriginalInstance()
        {
            pythonObject.dec_ref();
        }
    };
    /*
     * If the shared pointer is empty, this object is the original object owned
     * by Python and the Python handle can be acquired by:
     * py::cast(static_cast<ChildPy const *>(this))
     *
     * Copied instances will refer to the Python object handle via this member.
     * By only storing this member in copied instances, but not in the original
     * instance, we avoid a memory cycle and ensure clean destruction.
     */
    std::shared_ptr<OriginalInstance> m_originalInstance;

    [[nodiscard]] py::handle get_python_handle() const
    {
        if (m_originalInstance)
        {
            std::cout << "Refcount "
                      << m_originalInstance->pythonObject.ref_count()
                      << std::endl;
            return m_originalInstance->pythonObject;
        }
        else
        {
            auto self = static_cast<ChildPy const *>(this);
            return py::cast(self);
        }
    }

    template <typename Res, typename... Args>
    Res call_virtual(std::string const &nameOfPythonMethod, Args &&...args)
    {
        py::gil_scoped_acquire gil;
        auto ptr = get_python_handle().template cast<ChildCpp *>();
        auto fun = py::get_override(ptr, nameOfPythonMethod.c_str());
        if (!fun)
        {
            throw std::runtime_error(
                "Virtual method not found. Did you define '" +
                nameOfPythonMethod + "' as method in Python?");
        }
        auto res = fun(std::forward<Args>(args)...);
        return py::detail::cast_safe<Res>(std::move(res));
    }

    [[nodiscard]] std::unique_ptr<ChildCpp> clone_impl() const
    {
        auto self = static_cast<ChildPy const *>(this);
        if (m_originalInstance)
        {
            return std::make_unique<ChildPy>(*self);
        }
        else
        {
            OriginalInstance oi;
            oi.pythonObject = py::cast(self);
            // no idea why we would need this twice, but we do
            oi.pythonObject.inc_ref();
            oi.pythonObject.inc_ref();
            auto res = std::make_unique<ChildPy>(*self);
            res->m_originalInstance =
                std::make_shared<OriginalInstance>(std::move(oi));
            return res;
        }
    }
};

struct virtual_base
{
    virtual void call() = 0;
    virtual std::unique_ptr<virtual_base> clone() const = 0;
};

struct py_virtual_base
    : virtual_base
    , ClonableTrampoline<virtual_base, py_virtual_base>
{
    void call() override
    {
        call_virtual<void>("call");
    }
    std::unique_ptr<virtual_base> clone() const override
    {
        return clone_impl();
    }
};

void call_from_base(virtual_base &obj)
{
    obj.call();
}

PYBIND11_MODULE(python_override, m)
{
    py::class_<virtual_base, py_virtual_base>(m, "virtual_base")
        .def(py::init<>())
        .def("call", &virtual_base::call)
        .def("clone", &virtual_base::clone);
    m.def("call_from_base", &call_from_base);
}

CMake:

cmake_minimum_required(VERSION 3.12.0)

project(pybind11_test)
find_package(pybind11 2.9.1 REQUIRED)

pybind11_add_module(python_override main.cpp)

And using this in Python:

#!/usr/bin/env python3

import build.python_override as po

class child_virtual(po.virtual_base):
    def __init__(self):
        super().__init__()
    
    def call(self):
        print("Called child class")

def main():
    obj = child_virtual()
    obj.call()
    po.call_from_base(obj)

    obj_cloned = obj.clone()
    print("Will delete object now")
    del obj
    print("Deleted object now")
    obj_cloned.call()
    po.call_from_base(obj_cloned)

main()

Special note goes to the repeated inc_ref():

            // no idea why we would need this twice, but we do
            oi.pythonObject.inc_ref();
            oi.pythonObject.inc_ref();

The output of this is:

> ./main.py
Called child class
Called child class
Will delete object now
Deleted object now
Refcount 1
Called child class
Refcount 1
Called child class

However, when increasing the refcount only once:

> ./main.py
Called child class
Called child class
Will delete object now
Deleted object now
Refcount 0
Segmentation fault (core dumped)

moratom pushed a commit to luxonis/depthai-python that referenced this issue Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Docs or GitHub info
Projects
None yet
Development

No branches or pull requests

5 participants