cpp_param: Add ability to map C++ template paramters to Python to enable simple templating.

[EricCousineau-TRI]

This relates #7660: template classes and methods in pybind11.

This adds cpp_types, to be consumed by something like cpp_template. More context in #7682:

For a protoype, see Python output for binding basic templates and a Python derived-class scalar conversion example.

---

This change is 

[EricCousineau-TRI]

+@jadecastro for feature review, please. (This paves the way towards ease of scalar-type converting in Python, and also handling AbstractValues.)
+@soonho-tri for platform review, please.

---

Review status: 0 of 10 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[soonho-tri]

Checkpoint.

@EricCousineau-TRI , could you check the CI failure?

Also, I have the following test failure when I tested it on my mac:

$ bazel test //bindings/pydrake/util/... -c dbg

INFO: Analysed 37 targets (1 packages loaded).
INFO: Found 16 targets and 21 test targets...
FAIL: //bindings/pydrake/util:cpp_types_test (see /private/var/tmp/_bazel_soonho.kong/9a1423bc05dcfe121267333f6dfa4caa/execroot/drake/bazel-out/darwin-dbg/testlogs/bindings/pydrake/util/cpp_types_test/test.log)
INFO: From Testing //bindings/pydrake/util:cpp_types_test:
==================== Test output for //bindings/pydrake/util:cpp_types_test:
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from CppTypesTest
[ RUN      ] CppTypesTest.InPython
[       OK ] CppTypesTest.InPython (205 ms)
[ RUN      ] CppTypesTest.InCpp
unknown file: Failure
C++ exception with description "Unknown custom type" thrown in the test body.
[  FAILED  ] CppTypesTest.InCpp (1 ms)
[----------] 2 tests from CppTypesTest (206 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (206 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CppTypesTest.InCpp

 1 FAILED TEST
Traceback (most recent call last):
  File "/private/var/tmp/_bazel_soonho.kong/9a1423bc05dcfe121267333f6dfa4caa/bazel-sandbox/2693311784021142864/execroot/drake/bazel-out/darwin-dbg/bin/bindings/pydrake/util/cpp_types_test.runfiles/drake/tools/skylark/py_env_runner.py", line 15, in <module>
    subprocess.check_call(sys.argv[1:])
  File "/usr/local/Cellar/python/2.7.14_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 186, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['bindings/pydrake/util/cpp_types_test_cc']' returned non-zero exit status 1
================================================================================

---

Reviewed 7 of 12 files at r1.
Review status: 7 of 12 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

bindings/pydrake/util/BUILD.bazel, line 9 at r1 (raw file):

    "drake_cc_googletest",
    "drake_cc_library",
    "drake_cc_binary",

BTW, not used.

---

Comments from Reviewable

[jadecastro]

Checkpoint.
BTW, I have the same failure as @soonho-tri (in mac).

---

Reviewed 8 of 12 files at r1.
Review status: 9 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

bindings/pydrake/util/cpp_types_py.cc, line 8 at r1 (raw file):

namespace py = pybind11;

PYBIND11_MODULE(_cpp_types_py, m) {

BTW, m appears to be unused.

---

bindings/pydrake/util/cpp_types_pybind.h, line 20 at r1 (raw file):

namespace internal {

py::object GetTypeRegistry();

Please document this function.

---

bindings/pydrake/util/test/cpp_types_test.cc, line 44 at r1 (raw file):

  // Check pure-Python behavior.
  py::dict locals;
  py::exec(R"""(

BTW, I don't see anything against inline function argument sizes in the style guide, but is it reasonable to move this massive raw string into a python file that's called using eval_file()? The code might be easier to read/maintain in if it's separated from C++ (e.g. syntax highlighting, pycodestyle coverage, etc).

---

Comments from Reviewable

[EricCousineau-TRI]

After my changes, I've tested on macsim and was able to run it successfully (in release and debug mode).
Will add to Jenkins tests.

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please.

---

Review status: 6 of 12 files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/util/BUILD.bazel, line 9 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, not used.

Done.

---

bindings/pydrake/util/cpp_types_pybind.h, line 20 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

Please document this function.

Done.

---

bindings/pydrake/util/cpp_types_py.cc, line 8 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, m appears to be unused.

Done. (Removed module.)

---

bindings/pydrake/util/test/cpp_types_test.cc, line 44 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, I don't see anything against inline function argument sizes in the style guide, but is it reasonable to move this massive raw string into a python file that's called using eval_file()? The code might be easier to read/maintain in if it's separated from C++ (e.g. syntax highlighting, pycodestyle coverage, etc).

Done. Created cpp_types_test.py, renamed cpp_types_test.cc to cpp_types_pybind_test.cc.

---

Comments from Reviewable

[EricCousineau-TRI]

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please.

[jadecastro]

Sorry for the delay, first pass complete.

---

Reviewed 7 of 8 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

bindings/pydrake/util/test/cpp_types_pybind_test.cc, line 9 at r2 (raw file):

#include <gtest/gtest.h>
#include <pybind11/embed.h>

BTW, Unused?

---

bindings/pydrake/util/test/cpp_types_pybind_test.cc, line 34 at r2 (raw file):

struct CustomCppType {};

GTEST_TEST(CppTypesTest, InCpp) {

I assume it's beyond the scope to test the actual content of the Python and C++ data structures (i.e. testing the int literals might somehow suffice)? Either way, please add a blurb explaining what sorts of specific tests this unit test covers (the comments below are fairly generic).

---

bindings/pydrake/util/test/cpp_types_pybind_test.cc, line 41 at r2 (raw file):

  py::class_<CustomCppType>(m, "CustomCppType");

  // Check C++ behavior.

BTW, is the test for np.uint_64 missing?

---

bindings/pydrake/util/test/cpp_types_test.py, line 26 at r2 (raw file):

    def test_idempotent(self):
        # One-to-one.
        self._check_alias(bool, bool)

I'm concerned that the checks below are incapable of distinguishing the return of get_canonical() when a valid alias mapping exists versus when no alias mapping has been found. In the latter case, the default is returned, but in python, default is the same as alias, and so as near as I can tell, I don't think this unit test checks that a particular C++ type has been registered in Python at all.

---

Comments from Reviewable

[jadecastro]

Reviewed 1 of 8 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[soonho-tri]

First pass completed.

---

Reviewed 1 of 12 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

bindings/pydrake/util/cpp_types_pybind.cc, line 19 at r2 (raw file):

template <typename T>
void RegisterType(
    py::module m, py::object aliases, const std::string& canonical_str) {

BTW, we're passing m and aliases by value here and RegisterCommon function below. Is there a reason for that? (i.e. instead of either by const-ref?)

---

bindings/pydrake/util/cpp_types_pybind.h, line 14 at r2 (raw file):
FYI, from http://drake.mit.edu/styleguide/cppguide.html#Namespaces

Do not use Namespace aliases at namespace scope in header files except in explicitly marked internal-only namespaces, because anything imported into a namespace in a header file becomes part of the public API exported by that file.

Please consider moving this line inside of namespace internal below (and use pybind11 in the non-internal functions).

---

bindings/pydrake/util/type_pack.h, line 86 at r2 (raw file):

};

// @note Cannot bind templates with parameters packs arbitrarily.

Could you provide more information/documentation for this? (also need ///)

---

bindings/pydrake/util/type_pack.h, line 158 at r2 (raw file):

/// Visits each type in a type pack.
/// @tparam VisitWith
///   Visit helper. @see `type_visit_with_default`, `type_visit_with_tag.

BTW, there is a missing backtick at the end of this line.

---

bindings/pydrake/util/type_pack.h, line 167 at r2 (raw file):

void type_visit(
    Visitor&& visitor, type_pack<Ts...> = {}, template_tag<Predicate> = {},
    VisitWith = {}) {

FYI, there are no tests calling this function with four arguments.

---

tools/skylark/py_env_runner.py, line 8 at r2 (raw file):

"""

# TODO(eric.cousineau): See if there is a way to do this in pure C++, such

BTW, I think this comment can be removed now?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 5 of 12 files reviewed at latest revision, 10 unresolved discussions.

---

bindings/pydrake/util/cpp_types_pybind.cc, line 19 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, we're passing m and aliases by value here and RegisterCommon function below. Is there a reason for that? (i.e. instead of either by const-ref?)

I do it because that's what I've seen in pybind11 conventions. py::module and py::object are effectively cheap wrappers to PyObject* (with some reference counting including).
As the reference counting seems cheap (negligible w.r.t. to the rest), I'd prefer to keep this as-is.

---

bindings/pydrake/util/cpp_types_pybind.h, line 14 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

FYI, from http://drake.mit.edu/styleguide/cppguide.html#Namespaces

Do not use Namespace aliases at namespace scope in header files except in explicitly marked internal-only namespaces, because anything imported into a namespace in a header file becomes part of the public API exported by that file.

Please consider moving this line inside of namespace internal below (and use pybind11 in the non-internal functions).

I prefer to keep this in the global scope, because I actually do want it to be part of the public API.

Since we are using pybind11, it is convention to declare namespace py = pybind11 at the global scope, per the pybind examples.

Additionally, these modules are only intended for consumption in Python bindings.
The only common case in which this would make an issue is if someone tries something like mixing pybind11 and Boost.Python code. If that happens, this can be changed at that time.

---

bindings/pydrake/util/type_pack.h, line 86 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Could you provide more information/documentation for this? (also need ///)

Done.

Illustrated with analogous cases here:
https://github.com/EricCousineau-TRI/repro/blob/49cfad8adc8134a64be8d2aaf104ec84ee89a917/cpp/tpl_tag.cc

---

bindings/pydrake/util/type_pack.h, line 158 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, there is a missing backtick at the end of this line.

Done. Thanks!

---

bindings/pydrake/util/type_pack.h, line 167 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

FYI, there are no tests calling this function with four arguments.

Done. Removed argument. Thanks!

---

bindings/pydrake/util/test/cpp_types_pybind_test.cc, line 9 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, Unused?

py::scoped_interpreter comes from .../embed.h - I believe all of these headers are used.
(py::eval from eval.h, and the other stuff from pybind.h)

---

bindings/pydrake/util/test/cpp_types_pybind_test.cc, line 34 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

I assume it's beyond the scope to test the actual content of the Python and C++ data structures (i.e. testing the int literals might somehow suffice)? Either way, please add a blurb explaining what sorts of specific tests this unit test covers (the comments below are fairly generic).

Done.

---

bindings/pydrake/util/test/cpp_types_pybind_test.cc, line 41 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, is the test for np.uint_64 missing?

Done.

---

bindings/pydrake/util/test/cpp_types_test.py, line 26 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

I'm concerned that the checks below are incapable of distinguishing the return of get_canonical() when a valid alias mapping exists versus when no alias mapping has been found. In the latter case, the default is returned, but in python, default is the same as alias, and so as near as I can tell, I don't think this unit test checks that a particular C++ type has been registered in Python at all.

That's the scope of this test - checking that pure Python types are pass-through, and that aliased types are appropriately mapped.

No C++ types are registered, because that would causes this module to depend on the downstream module cpp_types_pybind, and then somehow expose those C++ types. Keeping the testing this way simplifies the coupling.

---

tools/skylark/py_env_runner.py, line 8 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, I think this comment can be removed now?

Er, I'd prefer to keep this. It feels very hacky, and makes debugging a pain... If there's a better solution, I'd love to use it.

---

Comments from Reviewable

[soonho-tri]

Reviewed 1 of 7 files at r3.
Review status: 6 of 12 files reviewed at latest revision, 5 unresolved discussions.

---

bindings/pydrake/util/cpp_types_pybind.h, line 14 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

I prefer to keep this in the global scope, because I actually do want it to be part of the public API.

Since we are using pybind11, it is convention to declare namespace py = pybind11 at the global scope, per the pybind examples.

Additionally, these modules are only intended for consumption in Python bindings.
The only common case in which this would make an issue is if someone tries something like mixing pybind11 and Boost.Python code. If that happens, this can be changed at that time.

I think it's still not a good idea to keep this line here. How about moving it inside of namespace drake or namespace drake::pydrake to localize the effect?

---

bindings/pydrake/util/type_pack.h, line 86 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

Done.

Illustrated with analogous cases here:
https://github.com/EricCousineau-TRI/repro/blob/49cfad8adc8134a64be8d2aaf104ec84ee89a917/cpp/tpl_tag.cc

Thanks!

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 6 of 12 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/util/cpp_types_pybind.h, line 14 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

I think it's still not a good idea to keep this line here. How about moving it inside of namespace drake or namespace drake::pydrake to localize the effect?

Done. Moved inside of drake::pydrake.

---

Comments from Reviewable

[jadecastro]

Reviewed 7 of 7 files at r3.
Review status: 11 of 12 files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/util/test/cpp_types_test.py, line 26 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

That's the scope of this test - checking that pure Python types are pass-through, and that aliased types are appropriately mapped.

No C++ types are registered, because that would causes this module to depend on the downstream module cpp_types_pybind, and then somehow expose those C++ types. Keeping the testing this way simplifies the coupling.

I see, this makes sense to me. Thanks for clarifying. I see now that only default_same == True is meaningful for Python.

---

Comments from Reviewable

[EricCousineau-TRI]

Applied rename.

---

Review status: 6 of 12 files reviewed at latest revision, all discussions resolved.

---

bindings/pydrake/util/test/cpp_param_test.py, line 26 at r2 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

I see, this makes sense to me. Thanks for clarifying. I see now that only default_same == True is meaningful for Python.

Welcome! (And sorry that it was a little hard to follow).
I've added is_aliased to try and clear that up a bit. (Let me know if you'd like me to switch the logic back.)

---

Comments from Reviewable

[jadecastro]

pending some BTWs.

---

Reviewed 10 of 11 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/util/cpp_param.py, line 15 at r5 (raw file):

def _get_type_name(t, verbose):
    # Gets type name as a string.
    # Defaults to just returning the name to shorten template names.

BTW, there's no default in this definition?

---

bindings/pydrake/util/cpp_param_pybind.h, line 48 at r5 (raw file):

/// @returns Python tuple of canonical parameters.
/// @throws std::runtime_error on the first type it encounters that is not
/// registered with `pybind11`.

BTW, I believe it throws if not registered in pybind11 nor aliased in RegisterCommon().

---

bindings/pydrake/util/test/cpp_param_test.py, line 26 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

Welcome! (And sorry that it was a little hard to follow).
I've added is_aliased to try and clear that up a bit. (Let me know if you'd like me to switch the logic back.)

I like this much better actually, and as an added bonus we eliminate that default Boolean argument :)

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 10 of 12 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/util/cpp_param.py, line 15 at r5 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, there's no default in this definition?

Done.
Changed it to a positional argument at the call site - would prefer to keep the number of defaults down if possible.

---

bindings/pydrake/util/cpp_param_pybind.h, line 48 at r5 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, I believe it throws if not registered in pybind11 nor aliased in RegisterCommon().

Done. D'oh, thanks for catching that!

---

Comments from Reviewable

[jadecastro]

Reviewed 1 of 11 files at r4, 1 of 2 files at r6.
Review status: 11 of 12 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[soonho-tri]

---

Reviewed 2 of 12 files at r3, 7 of 11 files at r4, 2 of 3 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable