Replace SWIG bindings with Pybind11, and remove SWIG dependency #4949

rdeits · 2017-01-28T06:26:54Z

~~This isn't quite ready for review yet, but I want to see how it does on CI. This also includes the changes from #4944 which should come in first. I'll update this branch when that happens.~~

This replaces all of the pydrake bindings with identical bindings generated with Pybind11. With this change and #4944 we no longer have any dependencies on SWIG, so this also deletes the SWIG and swigmake externals.

This also:

changes the python binding build process, so there should no longer be any python build files in the source tree
removes all the #ifndef SWIG guards

Resolves #4486
Fixes #4454
Fixes #4243
Fixes #2646
Fixes #4469

Todo before merging:

Undo my evil hacks to the build system
Make sure there aren't any other methods that SWIG was generating that aren't present here
Discuss cleaning up the Python APIs

This change is

rdeits · 2017-01-28T06:27:54Z

cmake/config.cmake

+  # else()
+  #   find_package(Python 2.7 MODULE REQUIRED)
+  # endif()
+  find_package(pybind11 REQUIRED)


Todo: something that isn't this

rdeits · 2017-01-28T06:28:16Z

drake/CMakeLists.txt

@@ -61,7 +61,7 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=all -Werror=ignored-qualifiers -
 if(NOT CMAKE_CXX_FLAGS MATCHES "-fsanitize=") # sanitizers are extremely brittle without lazy linking
  if(APPLE)
    set(CMAKE_SHARED_LINKER_FLAGS "-Wl,-undefined -Wl,error ${CMAKE_SHARED_LINKER_FLAGS}")
-    set(CMAKE_MODULE_LINKER_FLAGS "-Wl,-undefined -Wl,error ${CMAKE_MODULE_LINKER_FLAGS}")
+    # set(CMAKE_MODULE_LINKER_FLAGS "-Wl,-undefined -Wl,error ${CMAKE_MODULE_LINKER_FLAGS}")


Todo: restore this flag in a way that doesn't break the python bindings

rdeits · 2017-02-01T03:41:05Z

Ok, modulo the clang issue, this is getting pretty close to ready. @patmarion what do I need to do to make sure all the director IK interfaces are working?

rdeits · 2017-02-01T17:55:57Z

More cmake issues here... I'm currently running into the fact that pybind11 ships its own FindPythonLibsNew.cmake, but that file won't be run if python has already been found. Not running it causes various subtle bugs like the python modules having the wrong file extension. I'm currently hacking around it by just calling find_package(pybind11) earlier in the drake build process, but that's not a good solution.

rdeits · 2017-02-01T21:11:56Z

@patmarion OK, pydrakeik tests are now working.

rdeits · 2017-02-01T23:52:56Z

Ok, this is good enough that I'd appreciate some feedback on it, and some help resolving the clang issues.

rdeits · 2017-02-02T20:34:27Z

The clang issues appear to be due to pybind/pybind11#639

rdeits · 2017-02-02T21:41:49Z

And that issue is fixed in pybind/pybind11#604

rdeits · 2017-02-08T20:36:35Z

Huge thanks to @mwoehlke-kitware for figuring out the automagic Eigen-Numpy conversions for custom scalar types. That's let me substantially simplify the python wrapper code, and will also help with #4771 . I've also switched over to our temporary pybind11 fork, which should (hopefully) resolve the clang issues.

rdeits · 2017-02-08T20:38:58Z

Review status: 0 of 47 files reviewed at latest revision, 2 unresolved discussions.

drake/CMakeLists.txt, line 64 at r1 (raw file):

Previously, rdeits (Robin Deits) wrote…

Todo: restore this flag in a way that doesn't break the python bindings

Done.

cmake/config.cmake, line 212 at r1 (raw file):

Previously, rdeits (Robin Deits) wrote…

Todo: something that isn't this

Done.

Comments from Reviewable

rdeits · 2017-02-08T20:56:28Z

woohoo! this is finally working!

rdeits · 2017-02-08T20:57:58Z

+@sammy-tri +@jamiesnape +@mwoehlke-kitware

Review status: 0 of 47 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

jamiesnape · 2017-02-08T21:11:32Z

Reviewed 21 of 47 files at r1, 1 of 2 files at r2, 6 of 27 files at r3, 5 of 13 files at r5, 2 of 10 files at r6.
Review status: 34 of 47 files reviewed at latest revision, 1 unresolved discussion.

drake/bindings/pybind11/CMakeLists.txt, line 9 at r6 (raw file):

endif()

# Look up the python module suffix (which varies by python version).

Could this be added to FindPython.cmake?

Comments from Reviewable

rdeits · 2017-02-08T21:25:47Z

Review status: 34 of 47 files reviewed at latest revision, 1 unresolved discussion.

drake/bindings/pybind11/CMakeLists.txt, line 9 at r6 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Could this be added to FindPython.cmake?

good point. I forgot we were shipping our own FindPython

Comments from Reviewable

rdeits · 2017-02-08T21:37:09Z

Review status: 34 of 48 files reviewed at latest revision, 1 unresolved discussion.

drake/bindings/pybind11/CMakeLists.txt, line 9 at r6 (raw file):

Previously, rdeits (Robin Deits) wrote…

good point. I forgot we were shipping our own FindPython

Done.

Comments from Reviewable

jamiesnape · 2017-02-08T21:42:47Z

Review status: 34 of 48 files reviewed at latest revision, 2 unresolved discussions.

cmake/modules/FindPython.cmake, line 38 at r8 (raw file):

    OUTPUT_STRIP_TRAILING_WHITESPACE)

if(NOT PYTHON_MODULE_EXTENSION_SUCCESS MATCHES 0)

Remove this and add PYTHON_MODULE_EXTENSION to REQUIRED_VARS. If PYTHON_MODULE_EXTENSION is empty, then find_package will fail.

Comments from Reviewable

rdeits · 2017-02-08T21:44:04Z

Review status: 34 of 48 files reviewed at latest revision, 2 unresolved discussions.

cmake/modules/FindPython.cmake, line 38 at r8 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Remove this and add PYTHON_MODULE_EXTENSION to REQUIRED_VARS. If PYTHON_MODULE_EXTENSION is empty, then find_package will fail.

Done.

Comments from Reviewable

jamiesnape · 2017-02-08T21:48:55Z

Review status: 34 of 48 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

cmake/modules/FindPython.cmake, line 38 at r8 (raw file):

Previously, rdeits (Robin Deits) wrote…

Done.

You should only set PYTHON_MODULE_EXTENSION if PYTHON_MODULE_EXTENSION_SUCCESS indicates success. You probably do not need to capture ERROR_VARIABLE.

cmake/modules/FindPython.cmake, line 40 at r9 (raw file):

find_package_handle_standard_args(Python
  REQUIRED_VARS PYTHON_EXECUTABLE PYTHON_INCLUDE_DIR PYTHON_LIBRARY
  VERSION_VAR PYTHON_VERSION_STRING PYTHON_MODULE_EXTENSION)

Needs to be on the line above. VERSION_VAR is the name of an argument rather than a required variable.

Comments from Reviewable

rdeits · 2017-02-08T22:07:36Z

Review status: 34 of 48 files reviewed at latest revision, 3 unresolved discussions.

cmake/modules/FindPython.cmake, line 38 at r8 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

You should only set PYTHON_MODULE_EXTENSION if PYTHON_MODULE_EXTENSION_SUCCESS indicates success. You probably do not need to capture ERROR_VARIABLE.

Done.

cmake/modules/FindPython.cmake, line 40 at r9 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Needs to be on the line above. VERSION_VAR is the name of an argument rather than a required variable.

Agh, sorry. Done.

Comments from Reviewable

sammy-tri · 2017-02-09T16:53:52Z

+(status: curate commits before merging)

Partial pass completed. Left a few comments.

Reviewed 21 of 47 files at r1, 10 of 27 files at r3, 1 of 9 files at r4, 3 of 13 files at r5, 7 of 10 files at r6, 1 of 1 files at r7, 1 of 2 files at r8, 1 of 1 files at r10.
Review status: 46 of 48 files reviewed at latest revision, 8 unresolved discussions.

drake/bindings/CMakeLists.txt, line 1 at r10 (raw file):

if(NOT DISABLE_PYTHON)

BTW It seems odd to duplicate this check in drake/CMakeLists.txt and this file.

drake/bindings/pybind11/pydrake_autodiffutils.cc, line 13 at r10 (raw file):

using std::cos;

template <typename Derived>

BTW this function could maybe use a comment... it's not obvious where an AutoDiffScalar templated on a Derived which isn't VectorXd would come from.

drake/bindings/pybind11/pydrake_rbtree.cc, line 38 at r10 (raw file):

                urdf_filename, floating_base_type, &instance);
        },
        py::arg("urdf_filename"), py::arg("joint_type")="ROLLPITCHYAW"

Missing whitespace around = operator

drake/bindings/python/pydrake/init.py, line 1 at r10 (raw file):

from __future__ import absolute_import, division, print_function

do division and print_function do anything by being imported in this file? (or several other places where they're used?)

drake/bindings/python/pydrake/autodiffutils.py, line 3 at r10 (raw file):

from __future__ import absolute_import, division, print_function

import numpy as np

What effect does this have here when we don't reference np later?

Comments from Reviewable

rdeits · 2017-02-09T16:55:05Z

This currently doesn't compile on OSX due to issues finding the numpy headers. We may need one more update to our pybind11 fork

patmarion · 2017-02-09T17:00:28Z

btw, director uses a copy of this file to find NumPy headers:

https://raw.githubusercontent.com/pydata/numexpr/master/FindNumPy.cmake

and uses it like this:

find_package(NumPy)

if (NOT NUMPY_FOUND)
  message(WARNING "Optimized transformations C extension module is disabled because numpy is not found.")
  return()
endif()

if (NUMPY_VERSION VERSION_LESS 1.7)
  message(WARNING "Optimized transformations C extension module is disabled because numpy version < 1.7 was found.")
  return()
endif()


include_directories(${NUMPY_INCLUDE_DIRS})

rdeits · 2017-02-09T17:06:56Z

Review status: 42 of 48 files reviewed at latest revision, 5 unresolved discussions.

drake/bindings/CMakeLists.txt, line 1 at r10 (raw file):