[python] Add pybind11 header. #1519

jmirabel · 2021-08-31T11:55:37Z

Follows discussion #1518

I think the minimum C++ standard of my solution is 14. Not sure it works with 11.

@wxmerkt I would be happy to see your solution.

jcarpent

Thanks @jmirabel for providing this.
I made some comments that are easy to handle.

bindings/python/pybind11.hpp

jcarpent · 2021-08-31T12:23:09Z

bindings/python/pybind11.hpp

+template <typename T>
+struct convert_type {
+ typedef T type;
+ static inline auto _to(T t) { return t; }


inline is not needed here.

bindings/python/pybind11.hpp

unittest/python_pybind11/CMakeLists.txt

jcarpent

@jmirabel
Could you also add Doxygen info?

bindings/python/pybind11.hpp

jmirabel · 2021-08-31T12:34:18Z

I'll wait to see @wxmerkt proposal before addressing the requested changes.

Could you also add Doxygen info?

You mean documenting functions in Doxygen format ?

jcarpent · 2021-08-31T12:38:42Z

You mean documenting functions in Doxygen format ?

Yes, exactly.

wxmerkt · 2021-09-01T23:05:08Z

Apologies for the late response to this PR - this week is particularly busy and I wanted to experiment with combining the nice C++->Python caster with an implicit typecaster I've been using(*) - this would avoid having to wrap function calls. I might get to it tomorrow evening or Friday afternoon - however, please don't consider my slow response blocking. Would that be alright or would you like to move ahead before? In general, this PR is very nice and clean, particularly the re-use when going from C++ back to Python.

(*) This raises the question if an implicit type_caster is even desired since it will lead to temporaries when passing objects from Python to C++. It does make it easier to deal with Pinocchio objects for people working on modules with Pybind11 though (no wrapped calls).

jmirabel · 2021-09-02T07:06:31Z

Before going further, I would prefer to see what you have. Just the C++ file without the whole project is fine. I think it is enough for me to grasp what you do exactly.

jcarpent · 2021-09-08T15:49:45Z

@wxmerkt @jmirabel any update on this PR?

wxmerkt · 2021-09-09T00:06:52Z

My apologies, I went quiet on this one as I didn't find time to explore this further (it's ICRA season).

What I've been doing creates a type_caster inside pybind11's detail namespace - this way we don't need to wrap the functions and as soon as the header is included we can bind methods that take pinocchio types as arguments using pybind11. I realised I never finished the C++-back-to-Python part; and directly plugging in Joseph's solution for this part does not appear to work, but I haven't had time to explore this further. I also haven't checked if the load method of the type_caster results in copies.

The code snippet for Python-to-C++ without wrapping the functions is:

#define BOOST_PYTHON_TO_PYBIND11_TYPE_CASTER(native_type, boost_python_name)           \
    namespace pybind11                                                                 \
    {                                                                                  \
    namespace detail                                                                   \
    {                                                                                  \
    template <>                                                                        \
    struct type_caster<native_type>                                                    \
    {                                                                                  \
    public:                                                                            \
        PYBIND11_TYPE_CASTER(native_type, boost_python_name);                          \
        /* Python -> C++ */                                                            \
        bool load(handle src, bool)                                                    \
        {                                                                              \
            PyObject* source = src.ptr();                                              \
            value = boost::python::extract<native_type>(source);                       \
            return !PyErr_Occurred();                                                  \
        }                                                                              \
        /* C++ -> Python */                                                            \
        static handle cast(native_type src, return_value_policy policy, handle parent) \
        {                                                                              \
            /* TODO */                                                                 \
            throw std::runtime_error("C++ to Python not yet implemented!");            \
            return PyLong_FromLong(1);                                                 \
        }                                                                              \
    };                                                                                 \
    } /* namespace detail */                                                           \
    } /* namespace pybind11 */

BOOST_PYTHON_TO_PYBIND11_TYPE_CASTER(pinocchio::Model, _("pinocchio.pinocchio_pywrap.Model"));
// etc

jmirabel · 2021-09-09T08:27:26Z

That's neat. I'll give a try to @wxmerkt and update my PR.

jcarpent · 2021-09-09T08:43:29Z

Thanks @jmirabel and @wxmerkt for your contributions.

jmirabel · 2021-09-13T13:07:44Z

I also haven't checked if the load method of the type_caster results in copies.

I had a look and it results in a copy in both directions. Given the definition of PYBIND11_TYPE_CASTER, I don't see any workaround.

I think it is fine to provide both options:

no copy with wrapper,
copy without wrapper (developer-friendly but likely less user friendly).

jcarpent · 2021-09-13T18:25:00Z

My apologies, I went quiet on this one as I didn't find time to explore this further (it's ICRA season).

What I've been doing creates a type_caster inside pybind11's detail namespace - this way we don't need to wrap the functions and as soon as the header is included we can bind methods that take pinocchio types as arguments using pybind11. I realised I never finished the C++-back-to-Python part; and directly plugging in Joseph's solution for this part does not appear to work, but I haven't had time to explore this further. I also haven't checked if the load method of the type_caster results in copies.

The code snippet for Python-to-C++ without wrapping the functions is:

#define BOOST_PYTHON_TO_PYBIND11_TYPE_CASTER(native_type, boost_python_name)           \
    namespace pybind11                                                                 \
    {                                                                                  \
    namespace detail                                                                   \
    {                                                                                  \
    template <>                                                                        \
    struct type_caster<native_type>                                                    \
    {                                                                                  \
    public:                                                                            \
        PYBIND11_TYPE_CASTER(native_type, boost_python_name);                          \
        /* Python -> C++ */                                                            \
        bool load(handle src, bool)                                                    \
        {                                                                              \
            PyObject* source = src.ptr();                                              \
            value = boost::python::extract<native_type>(source);                       \
            return !PyErr_Occurred();                                                  \
        }                                                                              \
        /* C++ -> Python */                                                            \
        static handle cast(native_type src, return_value_policy policy, handle parent) \
        {                                                                              \
            /* TODO */                                                                 \
            throw std::runtime_error("C++ to Python not yet implemented!");            \
            return PyLong_FromLong(1);                                                 \
        }                                                                              \
    };                                                                                 \
    } /* namespace detail */                                                           \
    } /* namespace pybind11 */

BOOST_PYTHON_TO_PYBIND11_TYPE_CASTER(pinocchio::Model, _("pinocchio.pinocchio_pywrap.Model"));
// etc

It should be easy to implement the C++ -> Python. @wxmerkt Do you need it?

jcarpent · 2021-10-01T15:40:24Z

@jmirabel Could you fix the CI issues? I would suggest using a CMake external project to find PyBind11 instead of relying on the one installed on the system.

jmirabel · 2021-10-06T06:28:18Z

It's in my todo list.

I would suggest using a CMake external project to find PyBind11

You mean let CMake download the sources at configure time ?

jcarpent · 2021-10-06T07:43:17Z

Yes, this is what I mean.

jmirabel · 2021-10-13T06:49:14Z

The error on Windows looks unrelated to this PR:

pinocchio/math/fwd.hpp(69,5): error : expected unqualified-id [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(57,68): message : expanded from macro 'PINOCCHIO_OVERLOAD_MATH_BINARY_OPERATOR' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(69,5): error : expected ')' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(57,68): message : expanded from macro 'PINOCCHIO_OVERLOAD_MATH_BINARY_OPERATOR' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(69,45): message : to match this '(' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(69,45): error : expected ')' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(69,45): message : to match this '(' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(69,45): error : expected ')' [cpp2pybind11.vcxproj]
pinocchio/math/fwd.hpp(69,45): message : to match this '(' [cpp2pybind11.vcxproj]

Any hint of what's happening ?

jcarpent · 2021-10-13T07:33:35Z

It is weird that this is only occurring on this test. Could you check this is not an issue of include?

jmirabel · 2021-10-13T07:42:13Z

It happens from the pybind11 unit test but I don't get what I do wrong.

Could you check this is not an issue of include?

What do you mean ?

jcarpent · 2021-10-13T07:44:15Z

I think on Windows they might me issue with comma parsing in Macros.
You can check in the macro.hpp file of Pinocchio to check that.

jmirabel · 2021-10-13T08:01:57Z

The issue is with min and max. The expansion of PINOCCHIO_OVERLOAD_MATH_BINARY_OPERATOR(min) fails at (expanded):

    template<typename T1, typename T2> \
    inline typename internal::return_type_min<T1,T2>::type min(const T1 & a, const T2 & b) \
    { return internal::call_min<T1,T2>::run(a,b); }

Precisely, it isn't happy about the first const after (. For some reason, I think it parses min as a macro call.

jmirabel · 2021-10-14T12:34:07Z

CI is now all green except from codacy which shows what appears like irrelevant issues.

[python] Add pybind11 header.

f326fc1

jcarpent requested changes Aug 31, 2021

View reviewed changes

jcarpent reviewed Aug 31, 2021

View reviewed changes

bindings/python/pybind11.hpp Show resolved Hide resolved

jcarpent requested a review from wxmerkt August 31, 2021 14:19

[python] Update pybind11 headers.

7e06d46

traversaro mentioned this pull request Oct 9, 2021

Scripting interface to simulation workflow gazebosim/gz-sim#789

Closed

jmirabel mentioned this pull request Oct 12, 2021

[QUESTION] Working with a C++ library that has boost::python bindings pybind/pybind11#2987

Open

jmirabel force-pushed the pybind11 branch from d7b8aa1 to 2413ab9 Compare October 13, 2021 08:13

jmirabel added 3 commits October 13, 2021 11:16

[pybind11] Let CMake download pybind11

23c4160

[pybind11] Add pybind11-all.hpp

2720c6a

[CMake] Use NOMINMAX compilation flag on Windows.

ce31b40

jmirabel force-pushed the pybind11 branch from 2413ab9 to ce31b40 Compare October 13, 2021 09:21

jcarpent merged commit 364f573 into stack-of-tasks:devel Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[python] Add pybind11 header. #1519

[python] Add pybind11 header. #1519

jmirabel commented Aug 31, 2021

jcarpent left a comment

jcarpent Aug 31, 2021

jcarpent left a comment

jmirabel commented Aug 31, 2021

jcarpent commented Aug 31, 2021

wxmerkt commented Sep 1, 2021 •

edited

Loading

jmirabel commented Sep 2, 2021

jcarpent commented Sep 8, 2021

wxmerkt commented Sep 9, 2021

jmirabel commented Sep 9, 2021

jcarpent commented Sep 9, 2021

jmirabel commented Sep 13, 2021

jcarpent commented Sep 13, 2021

jcarpent commented Oct 1, 2021

jmirabel commented Oct 6, 2021

jcarpent commented Oct 6, 2021

jmirabel commented Oct 13, 2021 •

edited

Loading

jcarpent commented Oct 13, 2021

jmirabel commented Oct 13, 2021

jcarpent commented Oct 13, 2021

jmirabel commented Oct 13, 2021

jmirabel commented Oct 14, 2021

[python] Add pybind11 header. #1519

[python] Add pybind11 header. #1519

Conversation

jmirabel commented Aug 31, 2021

jcarpent left a comment

Choose a reason for hiding this comment

jcarpent Aug 31, 2021

Choose a reason for hiding this comment

jcarpent left a comment

Choose a reason for hiding this comment

jmirabel commented Aug 31, 2021

jcarpent commented Aug 31, 2021

wxmerkt commented Sep 1, 2021 • edited Loading

jmirabel commented Sep 2, 2021

jcarpent commented Sep 8, 2021

wxmerkt commented Sep 9, 2021

jmirabel commented Sep 9, 2021

jcarpent commented Sep 9, 2021

jmirabel commented Sep 13, 2021

jcarpent commented Sep 13, 2021

jcarpent commented Oct 1, 2021

jmirabel commented Oct 6, 2021

jcarpent commented Oct 6, 2021

jmirabel commented Oct 13, 2021 • edited Loading

jcarpent commented Oct 13, 2021

jmirabel commented Oct 13, 2021

jcarpent commented Oct 13, 2021

jmirabel commented Oct 13, 2021

jmirabel commented Oct 14, 2021

wxmerkt commented Sep 1, 2021 •

edited

Loading

jmirabel commented Oct 13, 2021 •

edited

Loading