Eventual collisions between opaque STL containers

[molpopgen]

After some chatting on gitter, @jagerman suggested that I raise an issue here, mostly so that it can be kept track of.

This is related to #439, but has more to do with what happens in the long run when more and more packages are developed using pybind11.

Currently, opaque STL containers are most often generated with a combo of the PYBIND11_MAKE_OPAQUE macro and a call to py::bind_vector.

If two different modules by different developers generate the same opaque containers, an import error occurs if both modules are used together. One simple solution is to wrap the binding in try/catch blocks as shown here: https://github.com/molpopgen/opaque_test. (That example uses ccpimport to compile the code.)

However, the try/catch solution is imperfect: the name seen on the Python side depends on the import order. Further, there is the deeper problem that py::bind_vector may optionally be invoked with py::buffer_protocol() and that the class type returned may be further augmented via .defs in the usual way. The possible result then is that both the name and features/behavior depend on import order, which seems less than ideal.

The gitter discussion ended with the agreement that a solution is needed and that it is not clear what that solution should look like.

[jagerman]

Brainstorming some possible solutions to this:

• we make bind_vector etc. a special class that can't be extended. If something else calls bind_vector with the same type, we simply make the two types aliases of each other.
• continue to allow bind_vector to be extended, make them aliases when multiply-registered, and have any extensions apply on top of the existing extensions. There's still a potential for name clashes (e.g. two different classes extend with different "foo()" methods, in which case overload ordering applies and is going to be applied by module load order).
• we add support for module-localized class_es, perhaps (borrowing for C++) a py::hidden_visibility() attribute. When a C++ -> Python type lookup results in such a type, we'd have to perform some additional lookup in an internal-to-the-module version of internals.

Any thoughts? (the question is wide open, not aimed at @molpopgen in particular)

[molpopgen]

we make bind_vector etc. a special class that can't be extended. If something else calls bind_vector with the same type, we simply make the two types aliases of each other.

I think that this could make sense for POD types, but perhaps less so for custom structs. I'm getting a lot of use from this feature for the case of an opaque vector of a C++ class that cannot be "dtyped" where I .def functions to return buffer_protocol-ed containers of data.

continue to allow bind_vector to be extended, make them aliases when multiply-registered, and have any extensions apply on top of the existing extensions. There's still a potential for name clashes (e.g. two different classes extend with different "foo()" methods, in which case overload ordering applies and is going to be applied by module load order).

No specific thoughts on this one for now.

we add support for module-localized class_es, perhaps (borrowing for C++) a py::hidden_visibility() attribute. When a C++ -> Python type lookup results in such a type, we'd have to perform some additional lookup in an internal-to-the-module version of internals.

This seems sensible, but I have no idea how intrusive a change this. Any idea how much the additional lookup would "cost" at run time?

[jagerman]

Any idea how much the additional lookup would "cost" at run time?

Probably not that much -- a second unordered_map lookup if the first type lookup gives back a special this-type-is-local-visibility value, and some logic to do that other lookup and apply the results. It's quite possible that I'm overlooking something else here, though, that could lead to it being much more intrusive.

[jagerman]

#949 will fix this. It essentially does the third approach I suggested above, (now called py::module_local) and applies this automatically to stl_bind.h bindings when those bindings are applied to a non-custom type (for custom types like std::vector<MyClass> the default is module-local only if MyClass is itself registered module-local).

[molpopgen]

Cool! Is there a unit test file showing that one can sent an opaque thing from module A to module B without a copy?

[jagerman]

Opaque types will never be copied (assuming you take them by pointer or reference); making them opaque is essentially just turning them into ordinary classes that go through the generic type caster. If they are local, they can't cross the module barrier; if global they can.

So in that respect, the v4.append in the tests added in #949 is essentially doing a cross-module global test, and the tests that raise exceptions in test_local_bindings() are essentially testing a cross-module failure due to local types that can't cross the barrier.

[molpopgen]

Hi, Sorry--I'm unable to follow what you've written. Am I able to define the same opaque type in two different modules, import both modules, and pass the type from one module into the next? I've read through the updates to the docs in the PR, but it'd be nice to have a plainer-English discussion of what is going on. Looking at the unit tests isn't clarifying things for me.

On Thu, Aug 3, 2017 at 5:42 PM Jason Rhinelander ***@***.***> wrote: Opaque types will never be copied (assuming you take them by pointer or reference); making them opaque is essentially just turning them into ordinary classes that go through the generic type caster. If they are local, they can't cross the module barrier; if global they can. So in that respect, the v4.append in the tests added in #949 <#949> is essentially doing a cross-module global test, and the tests that raise exceptions in test_local_bindings() are essentially testing a cross-module failure due to local types that can't cross the barrier. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGHnH3y2I8n8OFjPo9un_y72S2Evar5qks5sUmaigaJpZM4ODqxx> .

-- Kevin Thornton Associate Professor Ecology and Evolutionary Biology UC Irvine http://www.molpopgen.org http://github.com/ThorntonLab http://github.com/molpopgen

[jagerman]

No, that won't work, but that's a limitation that applies to ordinary types, too (e.g. py::class_<MyCppType>), but if two modules each try to bind MyType, they are defining distinct Python classes (e.g. m1.MyType and m2.MyType), and there's no one-to-one mapping between a C++ type MyCppType and a python type. The tradeoff is either making the binding global--in which case it needs to be bound in one place but can be used in multiple places--or making them local--in which case it can only be used within the shared object in which it is bound.

So the short answer is that if you want a type to cross the module boundary, it needs to be globally defined in just one module; another module depending on the bindings could do a py::module::import("othermodule"); in its binding code to make sure the other (global) bindings are available.

For basic types like std::vector<int> that are made opaque and bound with bind_vector(), however, the default will be local, which means that the type is available only in the module shared object in which it is defined. You can get around this explicitly by adding a py::module_local(false) argument to the bind_vector()--but in that case you can potentially run into conflicts with some other external module that later defines its own (global) binding on that vector type.

[molpopgen]

Ok, so if two modules declare opaque vector<int>, and both do so locally, what happens when the type from module A gets passed to a function in module B with const vector<int> & in its signature? Still trying to wrap my head around this....

On Fri, Aug 4, 2017, 7:53 AM Jason Rhinelander ***@***.***> wrote: Closed #919 <#919> via #949 <#949>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGHnHy2_p8srTuhy-bmOfZxvrdiuEmX0ks5sUy8KgaJpZM4ODqxx> .

-- Kevin Thornton Associate Professor Ecology and Evolutionary Biology UC Irvine http://www.molpopgen.org http://github.com/ThorntonLab http://github.com/molpopgen

[jagerman]

Ok, so if two modules declare opaque vector, and both do so locally, what happens when the type from module A gets passed to a function in module B with const vector & in its signature?

You'll get a type error: the pybind code in B will see as one of the arguments some custom type (e.g. A.intvector) that it doesn't know anything about, since that type is only registered within A.

[molpopgen]

Ok. The proposed changes seem like a regression to me, and raise more future problems. Really, this appears to just hides the issue that I raised. Modules based on this will have to come with caveats I'm their documentations: "If you pass this to another module, you may need to know if that module is implemented using pybind11. If so, please copy the data to a list first before passing it on". That's somewhat in jest, but these changes strongly suggest against using opaque containers of POD, IMO.

On Fri, Aug 4, 2017, 8:46 AM Jason Rhinelander ***@***.***> wrote: Ok, so if two modules declare opaque vector, and both do so locally, what happens when the type from module A gets passed to a function in module B with const vector & in its signature? You'll get a type error: the pybind code in B will see as one of the arguments some custom type (e.g. A.intvector) that it doesn't know anything about, since that type is only registered within A. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGHnH_9KqLdQQkAZPNFtpsc_xPCO3uAuks5sUzqdgaJpZM4ODqxx> .

-- Kevin Thornton Associate Professor Ecology and Evolutionary Biology UC Irvine http://www.molpopgen.org http://github.com/ThorntonLab http://github.com/molpopgen

[molpopgen]

Thought about it more on my way into work. I'll pull the changes associated with the PR and play around next week. Thanks.

On Fri, Aug 4, 2017 at 8:53 AM Kevin Thornton ***@***.***> wrote: Ok. The proposed changes seem like a regression to me, and raise more future problems. Really, this appears to just hides the issue that I raised. Modules based on this will have to come with caveats I'm their documentations: "If you pass this to another module, you may need to know if that module is implemented using pybind11. If so, please copy the data to a list first before passing it on". That's somewhat in jest, but these changes strongly suggest against using opaque containers of POD, IMO. On Fri, Aug 4, 2017, 8:46 AM Jason Rhinelander ***@***.***> wrote: > Ok, so if two modules declare opaque vector, and both do so locally, what > happens when the type from module A gets passed to a function in module B > with const vector & in its signature? > > You'll get a type error: the pybind code in B will see as one of the > arguments some custom type (e.g. A.intvector) that it doesn't know > anything about, since that type is only registered within A. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#919 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGHnH_9KqLdQQkAZPNFtpsc_xPCO3uAuks5sUzqdgaJpZM4ODqxx> > . > -- Kevin Thornton Associate Professor Ecology and Evolutionary Biology UC Irvine http://www.molpopgen.org http://github.com/ThorntonLab http://github.com/molpopgen

-- Kevin Thornton Associate Professor Ecology and Evolutionary Biology UC Irvine http://www.molpopgen.org http://github.com/ThorntonLab http://github.com/molpopgen

[jagerman]

Modules based on this will have to come with caveats I'm their documentations: "If you pass this to another module, you may need to know if that module is implemented using pybind11. If so, please copy the data to a list first before passing it on".

I do understand the limitation. It's a bit outside the scope of #949, but it's not something impossible. I think it might be possible to get it to work, perhaps by making bind_vector (and similar) create a global-scope base type that the (local) bound type inherits from. (But I may be overlooking something that would make that approach not workable).

[dean0x7d]

Modules based on this will have to come with caveats I'm their documentations: "If you pass this to another module, you may need to know if that module is implemented using pybind11. If so, please copy the data to a list first before passing it on". That's somewhat in jest, but these changes strongly suggest against using opaque containers of POD, IMO.

I'm not sure if I'm completely following, but I think the approach in #997 should address this concern. E.g. one module can bind a local opaque std::vector<int> using py:bind_vector. That opaque type will always be readable from other modules which use the completely generic vector caster found in <pybind11/stl.h>.

This doesn't really do anything for the case of two modules which have different local opaque bindings of the same vector -- those aren't compatible, but that's the point of the local bindings.

[molpopgen]

This doesn't really do anything for the case of two modules which have different local opaque bindings of the same vector -- those aren't compatible, but that's the point of the local bindings.

I haven't had time to play yet, but this is what I'm worried about. If two different modules define vector as opaque (and local), but add no further .def, my understanding is that they are incompatible, and cannot simply be treated as const std::vector & when passed to/from each module. Is this correct?

[jagerman]

If two different modules define vector as opaque (and local), but add no further .def, my understanding is that they are incompatible, and cannot simply be treated as const std::vector & when passed to/from each module. Is this correct?

Yes. (The opacity doesn't really matter; this hinges entirely on the module-local status).

I believe what you're looking for (and the reason I reopened the issue) is to have this working:
v1.cpp:

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

namespace py = pybind11;
PYBIND11_MAKE_OPAQUE(std::vector<int>)
PYBIND11_MODULE(v1, m) {
    py::bind_vector<std::vector<int>>(m, "IntVec1");
    m.def("sequence", [](int n) {
        std::vector<int> v;
        for (int i = 1; i <= n; i++) v.push_back(i);
        return v;
    });
    m.def("sum", [](const std::vector<int> &v) {
        int s = 0; for (auto i : v) s += i; return s;
    });
}

v2.cpp:

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

namespace py = pybind11;
PYBIND11_MAKE_OPAQUE(std::vector<int>)
PYBIND11_MODULE(v2, m) {
    py::bind_vector<std::vector<int>>(m, "IntVec2");
    m.def("squares", [](int n) {
        std::vector<int> v;
        for (int i = 1; i <= n; i++) v.push_back(i*i);
        return v;
    });
    m.def("sum", [](const std::vector<int> &v) {
        int s = 0; for (auto i : v) s += i; return s;
    });
}

v.py:

import v1, v2

a = v1.sequence(5)
b = v2.squares(5)
print(a)
print(b)
print(v1.sum(a), v2.sum(b))  # All good (same module types)
print(v1.sum(b), v2.sum(a))  # Failure (incompatible types)

which currently gives:

IntVec1[1, 2, 3, 4, 5]
IntVec2[1, 4, 9, 16, 25]
15 55
Traceback (most recent call last):
  File "v.py", line 8, in <module>
    print(v1.sum(b), v2.sum(a))  # Failure (incompatible types)
TypeError: sum(): incompatible function arguments. The following argument types are supported:
    1. (arg0: v1.IntVec1) -> int

Invoked with: IntVec2[1, 4, 9, 16, 25]

In principle, it seems feasible to make this work. Essentially the locality and incompatibility is only relevant on the Python side (i.e. during type caster casting). During argument loading, on the other hand, all that matters is that we can extract the local std::vector<int> & out of the Python instance. It ought to be possible to make this work by making module-local types only local to cast but keep them global with respect to loading, which I think would accomplish what you're looking for.

[molpopgen]

@jagerman Yes, that would be ideal.

[dean0x7d]

That wouldn't be too hard to pull off for a solitary type like std::vector<int>, but inheritance makes things much tougher. One module would have no information about the type hierarchy of the py::module_local types of another module. One possibility would be to smuggle this type information between modules by attaching it as a capsule to the Python type object (all Python types have a __dict__).

[molpopgen]

That wouldn't be too hard to pull off for a solitary type like std::vector, but inheritance makes things much tougher. One module would have no information about the type hierarchy of the py::module_local types of another module. One possibility would be to smuggle this type information between modules by attaching it as a capsule to the Python type object (all Python types have a dict).

Yes, inheritance should be harder, but that is ok in many cases. Since a class publicly inheriting from std::vector et al is making an error, classes using std containers should be expected to do so via private inheritance, and thus the buffer protocol and opaque stuff will be more "manual" for those types.

Except for Eigen/Armadillo types, I'm guessing it'll be fairly rare for two different modules to wrap the same underlying non-std C++ types.

My specific case where I ran into issues was in developing two different python packages based on two different C++ packages or genetics research. They are designed so that data can be shuttled to/from via some simple std containers, and it also makes sense for each Python package to provide opaque access w/buffer protocols available for those container types.

[jagerman]

Another possibility would be to smuggle the type loader out (rather than the type information in) by stashing a function pointer to the type loader somewhere associated with the instance for generic types; the caster could then call that (instead of its local type loader).

[jagerman]

#1007 should solve this: it essentially makes py::module_local only apply to the C++ -> Python conversion, while the Python -> C++ conversion will try to load from all local, foreign-local, and global type casters associated with the C++ type.