Support for pybind11 smart_holder

[kliegeois]

Hello,

For one of the projects for which I am using Binder, we need to use the smart_holder branch of pybindd11 ( https://github.com/pybind/pybind11/tree/smart_holder ). The differences, in term of pybindd11 code associated to the interface, are very minimal .

Would it be fine if I create during the next weeks a PR to binder to add an option to generate pybindd11 codes that use the smart_holder branch? Could this extra feature be accepted?

Thanks!

[lyskov]

I am open to the idea assuming that switching logic could be implemented neatly so it does not make code much harder to maintain. Do you already know what kind of changes in generated code this command-line will trigger? If so, could you please outline them here so it is possible to look at the concrete example? Thanks.

[kliegeois]

Thanks @lyskov for your comments!

(This issue pybind/pybind11#2839 is a good extra context of why it could be useful)

Up to now, I have been using the conservative mode of the smart holder branch.

So the code:

#include <pybind11/pybind11.h>

struct Foo {};

PYBIND11_MODULE(example_bindings, m) {
    namespace py = pybind11;
    py::class_<Foo>(m, "Foo");
}

would now be replaced by:

#include <pybind11/smart_holder.h>

struct Foo {};

PYBIND11_SMART_HOLDER_TYPE_CASTERS(Foo)

PYBIND11_MODULE(example_bindings, m) {
    namespace py = pybind11;
    py::classh<Foo>(m, "Foo");
}

In my previous successful tests, I have been using py::classh and PYBIND11_SMART_HOLDER_TYPE_CASTERS only for the classes for which it was useful.

Concretely, for binder, I was thinking of adding a configuration option that can be enabled per class like this:

+use_smart_holder_for_class Foo

If the option is set, then we would add PYBIND11_SMART_HOLDER_TYPE_CASTERS and use py::classh for that class.
And if there is at least one use_smart_holder_for_class, we would use #include <pybind11/smart_holder.h> instead of #include <pybind11/pybind11.h>.

What do you think of this?

[lyskov]

@kliegeois this looks quite reasonable to me, pull-request implementing this will be welcome! Could you please also include test in your PR? If so please take a look at https://github.com/RosettaCommons/binder/blob/master/test/T81.custom_trampoline_with_args.config test on how to specify specific config directive for particular test. Thanks,

Also, thank you for mentioning pybind/pybind11#2839 issue! I noticed similar issue in my project and having general solution for them would great!

[kliegeois]

Thanks for your comment @lyskov !
I will start working on this and will ping you on the PR once it is ready for review.

[kliegeois]

I am closing this issue as #269 has been merged. Thanks @lyskov !