Factory class doesn't allow derived classes to be created

[hughed2]

I'm trying to bind a factory class which takes in a constructor function and can later use that function to create an instance of the desired object. This should be working with inherited classes as well. In C++ it works fine, but in Python it looks like I'm losing the derived type. The derived class is created, deleted, and then an instance of the base class is returned.

Here are example bindings, keeping it as close to our original code as possible.
example.cpp

And then some sample python showing where it doesn't work as I expect
import example

def create_dog():
  class Dog(example.Animal):
    def _init__(self):
      example.Animal.__init__(self)
    def speak(self):
      print "Woof!"
    def __del__(self):
      print "The puppy is moving to a farm upstate"
  return Dog

in_python = create_dog()()
in_python.speak()
# Expected output: Woof!
# Actual output: Woof!

example.set_factory(create_dog())
in_cpp = example.get_animal()
# Expected output: Making new animal from factory
# Actual output: Making new animal from factory
#                The puppy is moving to a farm upstate
in_cpp.speak()
# Expected output: Woof!
# Actual output: I am just a generic animal, I have no voice

[uwydoc]

i think the problem is the cast in line 17:

#16    std::shared_ptr<Animal> operator()() { py::object animal = obj();
#17                                           return animal.cast< std::shared_ptr<Animal> >();}

what you are actually doing is converting an object created in python-side to some type in C++ side, which i do not think works in this simple way. yes, pybind11 supports converting between python object and c++ object for function arguments and return values, but with some book keeping codes, especially with creating proxy python object and properly handles reference counting.

i think the cast operator does not necessarily takes care of reference counting of the original python object, nor does it handles inheritance smartly enough. thus in your use case, what the cast operator did is just make a base class instance and return it, and after the return statement, the python object animal just got destructed.

so, for your use case, i think the proper way is do not perform the cast and just return the py::object instance created by your factory method:

    py::object operator()() { return obj(); }

however, this way is not very type-safe though.

[hughed2]

Thanks, you got me on the right track. It would be nice to have the same kind of boost::python functionality that is able to deal with inheritance, but I can subclass the factory easily enough and bind that instead of the original one.

[hughed2]

I'm reopening this because I need to get that factory working on the C++ side. As it stands, I have two factories. If I am working in C++, I use an Animal factory which can be used in C++, or, through my bindings, in python. If I am working in python, I use a pybind11::object factory which can ONLY be used in python, because of the casting issue seen above.

So, to go back to my original question: can we get this working, preferably without a workaround like above?

[jagerman]

I may be missing something, but I think you can solve this by always creating the new instance through Python via its __class__ attribute, by doing the equivalent of newobj = obj.__class__() in C++: py::object newobj = obj.attr("__class__")();. If object was an unextended C++ object, that just creates another one; if it was extended, that creates the other one through the extending Python class.

Here's an example:

myclone.cpp:

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

namespace py = pybind11;

struct Foobar {
    Foobar() {}
    virtual ~Foobar() {}

    virtual void speak() { std::cout << "I'm mute :(\n"; }
};

struct FoobarTrampoline : Foobar {
    using Foobar::Foobar;
    void speak() override { PYBIND11_OVERLOAD(void, Foobar, speak, ); }
};

PYBIND11_MODULE(myclone, m) {

    py::class_<Foobar, FoobarTrampoline>(m, "Foobar")
        .def(py::init<>())
        .def("speak", &Foobar::speak)
        ;

    m.def("clone_foo", [](py::object foobar) {
        return foobar.attr("__class__")();
    });

    m.def("make_it_speak", [](Foobar &f) { f.speak(); });
}

pyclone.py:

from myclone import Foobar, clone_foo, make_it_speak

class MyFoobar(Foobar):
    def __init__(self, *args, **kwargs):
        Foobar.__init__(self, *args, **kwargs)

    def speak(self):
        print("Woof!")

a = MyFoobar()
b = Foobar()
c = clone_foo(a)
d = clone_foo(b)

for i in [a, b, c, d]:
    print(type(i))
    i.speak()
    make_it_speak(i)

Output:

c++ -std=c++14 -Os -I/usr/include/python3.6m -I. myclone.cpp -shared -fPIC -o myclone.cpython-36m-x86_64-linux-gnu.so
jagerman@betwixt:~/src/pybind11/scratch⦗upstream✻$⦘$ python3 pyclone.py 
<class '__main__.MyFoobar'>
Woof!
Woof!
<class 'myclone.Foobar'>
I'm mute :(
I'm mute :(
<class '__main__.MyFoobar'>
Woof!
Woof!
<class 'myclone.Foobar'>
I'm mute :(
I'm mute :(

[hughed2]

I had that working on python previously, but the issue is doing the same in C++. I've tried dealing with a corresponding factory method in C++ itself rather than just in python, and that hasn't been working. I can write up a better example later you can actually run, but in the interest of time here's a quick example

//Create a MyFoobar object foobar_from_python
Foobar foobar_in_cpp = foobar_from_python.cast<Foobar>();
foobar_from_python.attr("speak")(); // Outputs "Woof!"
foobar_in_cpp.speak(); // Outputs "I'm mute :(" even though it's copied from a MyFoobar

A very similar problem, but one that can't be solved quite so easily. If I try going a bit fancier (like your suggestion, calling the python __class__ attribute), I still end up with a pybind11::object on the C++ side, which similarly becomes an unextended Foobar once it gets cast.

To complicate things further, these factories contain functions that return the desired class, and those functions don't have to be the default constructor. Those functions could mess around with member parameters before returning the object, for example. So it'd be best if I stick with the original object output by the factory (cast into the appropriate type) rather than falling back on trying to use the default constructor.

[jagerman]

I still end up with a pybind11::object on the C++ side, which similarly becomes an unextended Foobar once it gets cast.

Try casting it to a reference or pointer rather than a value:

auto &foobar_in_cpp = foobar_from_python.cast<Foobar &>();

and if you need to combine it with a factory, do the factory call via Python side (via invoking __class__, as in the examples above) then cast that resulting new py::object to a pointer or lvalue (rather than value): you'll get a reference to the actual C++ object stored inside the new Python object, which will actually be a FoobarTrampoline instance with the overridden virtual methods that forward into Python calls.

[hughed2]

Those were some of my first thoughts, and what I'm seeing is

auto foobar_in_cpp = foobar_from_python.cast< Foobar *>();
foobar_in_cpp->speak();

leads to a segfault. foobar_in_cpp is non-negative, and no pybind11::cast_error is thrown, but it looks like it's not properly casting.

auto foobar_in_cpp = foobar_from_python.cast< std::shared_ptr< Foobar > >();
foobar_in_cpp->speak();

leads to "I'm mute"

auto &foobar_in_cpp = foobar_from_python.cast<Foobar &>();
foobar_in_cpp.speak();

similarly segfaults.

[jagerman]

I think the segfault is coming from something else; I don't see any segfaults if I amend my example to m.def("make_it_speak", [](py::object o) { o.cast<Foobar &>().speak(); });

[hughed2]

The issue isn't when I'm trying to run something in the pybind11 module, it's when I'm trying to send it back to C++.

For example

m.def("clone_foo", [](py::object o) {
          Foobar *foo_ptr = o.cast<Foobar *>();
          foo_ptr->speak();
          return foo_ptr;
}

works, it gives the expected Woof! output. However, if I later (in C++, outside of any pybind11 modules) call

//object foo_from_python is a MyFoobar
Foobar *foo_ptr = foo_from_python.attr("clone_foo").cast<Foobar *>();
foo_ptr->speak();

It'll first produce the Woof! from the clone_foo call, but then I get a segfault (or, with some changes to the code, it won't segfault but will give me an I'm mute message).

[jagerman]

Foobar *foo_ptr = foo_from_python.attr("clone_foo").cast<Foobar *>();

That needs to be:

/*...*/ .attr("clone_foo")().cast<Foobar *>();

otherwise you're trying to cast the "clone_foo" attribute itself (rather than the result of calling it) to a Foobar pointer.

[hughed2]

It was correct in my actual code, there was just an editing error in my previous comment.

[EricCousineau-TRI]

I have not had time to fully process the entirety of this issue, but it looks like this is an aliasing / truncation issue due to the Python interpreter losing references on Pythonic portion of the instance, and leaving the C++ portion out in the cold all alone.

Can you look at this PR and let me know if that actually might solve your issue? (The caveat is having to be beholden to a specific type of holder)
#1146
Specifically, this portion of the documentation highlights this edge case that you may be running into:
https://github.com/EricCousineau-TRI/pybind11/blob/feature/py_move_shared-wip/docs/advanced/classes.rst#virtual-inheritance-and-lifetime

I ask because ultimately our project may want something similar; a factory method that can be called on the C++ side, generating Python-derived objects that were registered with the factory.

All that being said, have y'all already discussed the workaround of having a Python-specific base class / proxy, which stores its own py::object reference to the effective base class? (This was suggested by someone else.)
This gets hairy with (a) opaque reference cycles between pybind11 holders and Python references, and (b) becomes crappy when you want to reference state in the base class (if your base is not a pure virtual class); however, this could ultimately be a first pass to more C++-friendly keep alive semantics.

[EricCousineau-TRI]

Just to check, can I ask why you closed this? Did you come to a resolution in your code, or is this already covered by above issue?

[pvallet]

I know this is kind of an old issue, but I've encountered the same situation, so I think it could be useful to share how I handled it there.
The problem in

std::shared_ptr<Animal> createAnimal(py::object class) {
  py::object animal = class();
  return animal.cast< std::shared_ptr<Animal> >();
}

is that the shared pointer seems to only be a soft reference to the object animal. I haven't dug in pybind11 code, but from my tests the shared pointer python type is only kept until animal gets deleted.
The workaround that I'm using to circumvent this problem is to store the python object on the heap and then handle its lifetime with shared pointers:

std::shared_ptr<Animal> createAnimal(py::object class) {
  py::object* animal = new py::object(class());
  return std::shared_ptr<Animal>(animal->cast< Animal* >());
}

The trick here is to cast to a raw pointer instead of a shared pointer, because otherwise, the python reference that animal holds would be added to the generated shared pointer, leading to animal never getting deleted.

[EricCousineau-TRI]

Er, I've since forgotten the intricacies on this one, but I do think this might be a form of object slicing happening here:
#1774 (comment)

@pvallet In your case, it works b/c you've allocated py::object on the heap, so it's refcount doesn't go to zero. In effect, though, you're leaking this object, but TBH it's probably not that big of a deal.

[pvallet]

How is it leaking? The destructor of animal gets called when the shared pointer gets deleted, reaching a ref count of zero. Am I missing something?

[EricCousineau-TRI]

I think it's this line:

py::object* animal = new py::object(class());

I don't see delete animal being called anywhere, which is (a) why animal doesn't get sliced and (b) why animal doesn't get double-deleted by the pybind11 holder.

[pvallet]

delete gets called by the shared pointer destructor, which takes ownership of the raw pointer.
https://stackoverflow.com/questions/4665266/creating-shared-ptr-from-raw-pointer

[EricCousineau-TRI]

Er, true for shared_ptr<Animal> as a standalone, but unfortunately I don't think that's the relevant case.

The more nuanced part is py::object* and the shared_ptr<Animal> it contains as it's holder.
If ya haven't already seen:
https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html

When you call new py::object(cls()), two things happen:

• cls() invokes py::function, calls whatever method passed in, returns the py::object. In that py::object, there's some pybind11 magic that stores the type-erased holder, which in this case is shared_ptr<Animal>.
• For new py::object(o), it creates py::object on the heap (as ya pointed out), increases the refcount, but then never gets deleted, thus the contained holder is never deallocated.

For reference:

pybind11/include/pybind11/pybind11.h

Lines 1328 to 1380 in d96c345

	/// Initialize holder object, variant 1: object derives from enable_shared_from_this
	template <typename T>
	static void init_holder(detail::instance *inst, detail::value_and_holder &v_h,
	const holder_type * /* unused */, const std::enable_shared_from_this<T> * /* dummy */) {
	try {
	auto sh = std::dynamic_pointer_cast<typename holder_type::element_type>(
	v_h.value_ptr<type>()->shared_from_this());
	if (sh) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(sh));
	v_h.set_holder_constructed();
	}
	} catch (const std::bad_weak_ptr &) {}
	if (!v_h.holder_constructed() && inst->owned) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());
	v_h.set_holder_constructed();
	}
	}
	static void init_holder_from_existing(const detail::value_and_holder &v_h,
	const holder_type *holder_ptr, std::true_type /*is_copy_constructible*/) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(*reinterpret_cast<const holder_type *>(holder_ptr));
	}
	static void init_holder_from_existing(const detail::value_and_holder &v_h,
	const holder_type *holder_ptr, std::false_type /*is_copy_constructible*/) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(*const_cast<holder_type *>(holder_ptr)));
	}
	/// Initialize holder object, variant 2: try to construct from existing holder object, if possible
	static void init_holder(detail::instance *inst, detail::value_and_holder &v_h,
	const holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this<T>) */) {
	if (holder_ptr) {
	init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible<holder_type>());
	v_h.set_holder_constructed();
	} else if (inst->owned || detail::always_construct_holder<holder_type>::value) {
	new (std::addressof(v_h.holder<holder_type>())) holder_type(v_h.value_ptr<type>());
	v_h.set_holder_constructed();
	}
	}
	/// Performs instance initialization including constructing a holder and registering the known
	/// instance. Should be called as soon as the `type` value_ptr is set for an instance. Takes an
	/// optional pointer to an existing holder to use; if not specified and the instance is
	/// `.owned`, a new holder will be constructed to manage the value pointer.
	static void init_instance(detail::instance *inst, const void *holder_ptr) {
	auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
	if (!v_h.instance_registered()) {
	register_instance(inst, v_h.value_ptr(), v_h.type);
	v_h.set_instance_registered();
	}
	init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
	}

pybind11/include/pybind11/pytypes.h

Lines 220 to 240 in d96c345

	/** \rst
	Holds a reference to a Python object (with reference counting)
	Like `handle`, the `object` class is a thin wrapper around an arbitrary Python
	object (i.e. a ``PyObject *`` in Python's C API). In contrast to `handle`, it
	optionally increases the object's reference count upon construction, and it
	*always* decreases the reference count when the `object` instance goes out of
	scope and is destructed. When using `object` instances consistently, it is much
	easier to get reference counting right at the first attempt.
	\endrst */
	class object : public handle {
	public:
	object() = default;
	PYBIND11_DEPRECATED("Use reinterpret_borrow<object>() or reinterpret_steal<object>()")
	object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
	/// Copy constructor; always increases the reference count
	object(const object &o) : handle(o) { inc_ref(); }
	/// Move constructor; steals the object from ``other`` and preserves its reference count
	object(object &&other) noexcept { m_ptr = other.m_ptr; other.m_ptr = nullptr; }
	/// Destructor; automatically calls `handle::dec_ref()`
	~object() { dec_ref(); }

[pvallet]

Ok, I understand, thank you for the explanations. Basically, I was testing that the py::object pointer was deleted by casting it again to its underlying C++ type. I didn't realize that only the underlying type and not the python container was deleted. A bit counter intuitive to me since virtual calls to normal functions are resolved but not to destructors, it seems. I guess I'll have to dig into pybind11 code base to understand this issue better.
I'll try to work out another workaround to handle that python object lifetime, which should be doable since it should only be owned by my C++ code.

[EricCousineau-TRI]

Sounds good! And TBH, your current workaround is probably more or less what you may want.

I have a solution in our little fork of pybind, but it's a tad bit messy, and doesn't do well in Python 3.8 ;)
https://github.com/RobotLocomotion/pybind11/blob/drake/README_DRAKE.md

[pvallet]

Ok, I got it to work using aliasing constructors, thanks to this github issue that was related #1389

std::shared_ptr<Animal> createAnimal(py::object pyClass) {
  std::shared_ptr<py::object> pyObject = std::make_shared<py::object>(pyClass());
  return std::shared_ptr<Animal>(pyObject, pyObject->cast<Animal*>());
}

Basically, the python object holder is kept alive for as long as its contained object is alive. Unless I'm missing something, this should be memory safe. It just feels like the cast operator should be able to handle that itself.

[EricCousineau-TRI]

Ooh, nice!

I was typing out that I thought there'd be some weird double-delete w/ a reference cycle, but reading through the SO link and the issue discussion, but nah, it looks pretty solid!

(Still trying to think of edge cases, but so far have come up with none!)

[EricCousineau-TRI]

Note: This is most likely a duplicate of #1333.