Keyword arguments mismatch with C++ function arguments

[uentity]

I have a C++ function with the following signature:

spv_float well_path_ident(
	ulong nx, ulong ny, spv_float coord, spv_float zcorn,
	spv_float well_info, bool include_well_nodes, const char* strat_traits = "online_tops",
	const ulong min_split_threshold = 0, spv_ulong hit_idx = nullptr
);

Here ulong is std::size_t, and spv_float, spv_ulong are std::shared_ptr pointers to my custom array type which is inherited from pybind11::array_t. I've written custom converters for it that are very similar to Eigen binding code. Converters are working as expected in my tests.

Next, I export the function above with the following snippet:

m.def("well_path_ident", &well_path_ident,
	"nx"_a, "ny"_a, "coord"_a, "zcorn"_a, "well_info"_a, "include_well_nodes"_a = true,
	"mesh_traits"_a = "online_tops", "min_split_threshold"_a = 0, "hit_idx"_a = nullptr
);

Everything compiles just fine! pybind11 correctly counts arguments of well_path_ident -- if I make a mistake (for ex. forget one argument) it will notify me with an error at compile time.

But after module is loaded in Python here's what I get with help(my_module) command:

well_path_ident(...)
        well_path_ident(*args, **kwargs)
        Overloaded function.

        1. well_path_ident(nx: int, ny: int, numpy.ndarray[float64], numpy.ndarray[float64], numpy.ndarray[float64], coord: bool, zcorn: unicode, well_info: int, numpy.ndarray[uint64]) -> numpy.ndarray[float64]

(I have one more overload of this function, but it has the same issue).

As you can see, keyword arguments "skip" numpy.ndarray[float64] (spv_float) for some reason, and starting from "coord" they bind to wrong C++ function arguments.

I feel that this is related in some way to custom type conversion code, but what really can cause such issue?
Btw, when I try to call this function from Python, I get an exception:

RuntimeError: Unknown internal error occurred

And actual C++ function isn't called, I can't get into it in debugger.

[uentity]

If I leave only one overload and remove all keyword arguments like this:

m.def("well_path_ident", &well_path_ident);

then function called properly without exceptions and all arguments are passed correctly.

[jagerman]

The issue is most likely stemming from inheriting from array_t while simultaneously using a std::shared_ptr around it. array_t (and object and all the other Python type wrappers) are already a "shared pointer" of a sort (though they are sharable references to Python-side numpy arrays rather than C++ types).

You probably want to use composition (i.e. use a local array_trather than inherit from it) instead.

I'll add a static_assert to catch this in the future.

[jagerman]

(I also couldn't reproduce the skipping-kwargs issue with regular array_t arguments, so I suspect it's the same issue).

[uentity]

I understand your point about all wrapped Python types being already reference counted via object. And I think I can switch from inheritance to composition and probably have this issue solved.

But still, if used properly, inheritance from object combined with std::shared_ptr holder is not such a nasty bad unacceptable thing. If it is, then it should be prohibited completely. If not, at least it shouldn't break pybind11 internals. IMHO.

[jagerman]

Yes, you're definitely right. That's what #786 will fix (by making it a compilation failure).

[jagerman]

Thanks for the report. This should cause a compilation failure now rather than an internal error. I've filed #787 separately to track properly supporting holders around converting types.

[jagerman]

I've reproduced the issue with this test case; investigating it now:

#include <pybind11/pybind11.h>

struct mytype {};
using mysp = std::shared_ptr<mytype>;

NAMESPACE_BEGIN(pybind11)
NAMESPACE_BEGIN(detail)

template <> struct type_caster<mysp> {
public:
	bool load(handle src, bool convert) { return false; }
	static handle cast(const mysp&, return_value_policy, handle) { return handle(); }
    operator mysp() { return value; }
	template <typename> using cast_op_type = mysp;
    static PYBIND11_DESCR name() { return _("foo"); }

private:
    mysp value;
};

NAMESPACE_END(detail)
NAMESPACE_END(pybind11)

namespace py = pybind11;
PYBIND11_PLUGIN(example)
{
   py::module m("example");
   m.def("f", [](int, mysp, int) {}, py::arg("a"), py::arg("b"), py::arg("c") = 2);
   return m.ptr();
}

[jagerman]

... which gives the rather interesting (and wrong) docstring:

python3 -c 'import example; print(example.f.__doc__);'
f(a: int, foo, b: int) -> c: None=2

[uentity]

Yes, that's exactly what I've got.

[jagerman]

Aha, there's a bug in the Eigen type caster with named arguments (which you ended up copying).

The

static PYBIND11_DESCR name() { return _("foo"); }

should be

static PYBIND11_DESCR name() { return type_descr(_("foo")); }

[jagerman]

The test suite code that I wrote even has some of the same errors in it that I never noticed (the 'arg0: ' should be on the argument, not the return type!).

[jagerman]

The second bug (fixed in PR #791) affects the doc string in eigen (and if you make the same change, as I mentioned above, should fix it in your class as well). I made sure that it doesn't actually affect the passing of named arguments themselves: it just affects the way they are displayed in the error message/docstring.

That PR (and equivalent change for you) should resolve the docstring; I'm hoping that it plus the change to make the cast_op_type a copy (rather than reference or pointer) will resolve the unknown internal error, but please let me know if it doesn't.

You aren't hitting the added static asserts because you're adding a type caster for a shared pointer, rather than the pointed-at type of the shared_ptr (which is what I had assumed from your initial report), and so you shouldn't hit that code path at all. On the plus side, the change to composition probably isn't necessary: your type caster isn't for a type that inherits from object, it's a shared_ptr, so I think you'll actually be fine with the current approach. (That said, if you've made substantial progress with composition, I think it's probably a worthwhile change for maintainability).

[uentity]

TLDR: did that (uentity/pybind11@c5d1198) but issue is not resolved.

Docstring is fixed now. If I pass all arguments (including ones with default values specified), then function get called without errors. If I omit at least one argument from the end of args list, then I hit RuntimeError: Unknown internal error occurred.

Maybe default value for an array casing the issue? My C++ function accepts std::shared_ptr< myarray> = nullptr last argument. From docstring I see that nullptr transformed to none on Python side. How can I correctly pass nullptr to C++ function from Python if I have custom type caster for std::shared_ptr< myarray>.

As of composition code - I've made ~60% of work, but took a pause until you can find out the solution :). And I was thinking the same way -- if I wrap shared_ptr, then I can leave inheritance. My other array traits are also inherited from containers (so I can for ex. easily import constructors), so for better consistency and less chance of errors I'd better leave current approach as is.

[jagerman]

I'm all for tracking this down, but I'm out of ideas.

I don't think the nullptr could cause the internal error issue. While it will convert the value to None, the worst that happens in that case is you get load called with src = none(), which will most likely end up being passed to Array::ensure which will convert it into a 0-d array containing a NaN value. Probably not what you want (i.e. you should check src.is_none() in load(), and if so, set the nullptr yourself), but also not causing a problem.

If you can narrow this down into a relatively small, self-contained test case, I'll investigate some more, but right now it's a bit too much.

[uentity]

@jagerman, I've nailed down the issue. It indeed was related to code path assigning default nullptr value (which becomes None in Python) to array shared_ptr argument.

Exception in Python fires when calling array_t::ensure(src) when src is actually None. Is it an error or not is up to you to decide. I've added check for None to my type caster and that solved the issue.

I'll prepare test case tomorrow and post here.

[jagerman]

What's the T on your array_t<T>?

[uentity]

Ahh error was a little bit deeper than I described.

The main cause of the issue was not in array_t::ensure(src) call with src = None (which actually succeeded), but in the following cast to my custom array type, derived from array_t, which invoked array_t constructor again with object.m_ptr = 0 (casted from None) and this time array_t throws error_already_set() (because it assumes that if m_ptr is not initialized, then something bad happened).

So, the issue caused by the following line:

Array aref = Array::ensure(src);

where Array is the type, derived from array_t, so array_t constructor is called twice here, and src is None.

You was right about tricky maintaining of non-compositional code.

[jagerman]

So I am right to conclude that pybind is okay here, it was just an error handling issue? (Nevertheless, we did find and fix two other bugs in the process, so it was all worthwhile).

[uentity]

@jagerman pybind11 is okay here, but maybe you'll consider to replace

    array_t(const object &o) : array(raw_array_t(o.ptr()), stolen_t{}) {
        if (!m_ptr) throw error_already_set();
    }

with

    array_t(const object &o) : array(raw_array_t(o.ptr()), stolen_t{}) {
        if (!m_ptr && PyErr_Occurred()) throw error_already_set();
    }

?

You may argue that array with m_ptr = 0 is almost useless and even dangerous thing, but:

1. Such arrays already appear when initialized from None and at least such array can be checked immediately (if(!array) ...).
2. I think that error_already_set should be thrown only in case of Python exception really happened. Otherwise it leads to strange confusing RuntimeError: Unknown internal error occurred.

If array_t constructor have had such a check (if Python exception really happened) then this overall issue #785 wouldn't appear.

[jagerman]

Unknown internal error is indeed a problem. However, the issue does not appear to be initializing with none (because none is not m_ptr = 0; rather it has m_ptr set to the PyNone object). If I do that, it works for a double array (I get a 0-d array containing NaN) and gives me a TypeError: int() argument must be a string, a bytes-like object or a number, not 'NoneType' for an int array.

I can trigger it with an explicit empty object (which is different from None!), like this:

py::array_t<double>(py::object())

and this really does indicate a failure of the caller: we're being asked to convert a generic object into a specific type (array_t<T>) when the object isn't actually referencing an object at all.

That said, we need to fix the error message to something informative.

[jagerman]

#794 should address this (by giving a more informative error message).

[uentity]

@jagerman what Python version are you using? I'm on 2.7.x (still) and I can tell for sure that array_t::ensure(src) with src being None gives me an array with m_ptr = 0.

Here's the test code that I inherited from your example.

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

namespace py = pybind11;
using namespace pybind11::literals;

using ulong = std::size_t;
using Array = py::array_t< unsigned long, py::array::forcecast >;

struct mytype {
	Array val_;

	mytype(const Array& src) : val_(src) {}

	mytype& operator()(const Array& src) {
		val_ = src;
		return *this;
	}
};
using mysp = std::shared_ptr<mytype>;

NAMESPACE_BEGIN(pybind11)
NAMESPACE_BEGIN(detail)

template <> struct type_caster<mysp> {
public:
	bool load(handle src, bool convert) {
		bool need_copy = !isinstance< Array >(src);

		if(!need_copy) {
			Array aref = reinterpret_borrow< Array >(src);
			if(aref && aref.writeable()) {
				value = std::make_shared< mytype >(std::move(aref));
				return true;
			}
		}

		//if (!convert) return false;

		Array aref = Array::ensure(src);
		if(aref.ptr() == 0)
			std::cout << "OMG! I'm null, don't try to dereference me!" << std::endl;
		if(!aref) return false;
		value = std::make_shared< mytype >(std::move(aref));
		return true;
	}

	static handle cast(const mysp&, return_value_policy, handle) {
		return handle();
	}

	operator mysp() { return value; }
	template <typename> using cast_op_type = mysp;
	static PYBIND11_DESCR name() { return type_descr(_("foo")); }

private:
	mysp value;
};

NAMESPACE_END(detail)
NAMESPACE_END(pybind11)

mysp f(
	ulong nx, ulong ny, mysp coord, mysp zcorn,
	mysp well_info, bool include_well_nodes = true, const char* strat_traits = "online_tops",
	const ulong min_split_threshold = 0, mysp hit_idx = nullptr
) {
	return mysp();
}

PYBIND11_PLUGIN(example)
{
	py::module m("example");
	m.def("f", &f,
		"nx"_a, "ny"_a, "coord"_a, "zcorn"_a, "well_info"_a, "include_well_nodes"_a = true,
		"mesh_traits"_a = "online_tops", "min_split_threshold"_a = 0, "hit_idx"_a = nullptr);

	//m.def("f", [](int, int, mysp) {}, py::arg("a"), py::arg("c") = 2, py::arg("b") = nullptr);
	return m.ptr();
}

When I call f() like this: f(3, 4, None, None, None) I get "OMG!..." printed 4 times.

[jagerman]

Yes, you get a nullptr back in that example, but the python error will also be set properly. If you change Array to array_t<double> you won't get the "OMG".

[uentity]

OK, I think we nailed this issue to the bones, so it's time to just close it :)
Thank you very much for your help!