Spike test extent to which Python can use the existing System framework

[EricCousineau-TRI]

This is meant to spike test the extent to which Python can use the existing Systems framework.

From Robin's initial prototype with pybind11 and the follow-up implementations Drake now has, it seems like pybind11 should be able to handle most of the basic API.

I've only hit two main sticking points thus far:

• Taking ownership of Python-created objects in C++ - the use of std::unique_ptr<> may provide some barriers to easily taking ownership of Python-created objects. Discussion from pybind11 here, an example repro here (comparing unique_ptr and shared_ptr destruction).
• Templates (scalar type conversion) - a messy prototype is available here. This seems rough, but achievable, but runs into issues with the above issue on std::unique_ptr<> (specifically, when trying to have Python-defined Converter<> functions passed back to BaseConverter - for testing Python-defined classes).

Motivation for both of these:

• Templates / scalar type conversion - also for handling Value<T> instances for AbstractValues
• std::unique_ptr - Ensure that we can properly construct diagrams, create model values, do cloning, etc.

@RussTedrake - can I ask if you had run into any of these issues when you were prototyping these things?

[RussTedrake]

sorry -- i dodged both on my quick spike (was just prototyping LinearSystem at the time).

[EricCousineau-TRI]

It seems like there may be a way to make unique_ptr<> stuff work in the Python <-> C++ bridge.
Posted a draft PR for pybind11 here: pybind/pybind11#1132

[rdeits]

My $0.02 is that requiring users to ensure that a python object has exactly one reference is too much of a burden. References can be created in all sorts of interesting and non-obvious ways, like inside a closure or a generator, and I've never encountered any python code that relied on an object having exactly one reference. Reference counting is also something that's not generally taught except when actually implementing python extensions in C, so it's something that I'd venture most python users won't understand or be able to easily work around.

Just as an example, if I run the following inside an IPython shell:

In [1]: import sys

In [2]: x = [1, 2, 3]

In [3]: sys.getrefcount(x)
Out[3]: 2

In [4]: x
Out[4]: [1, 2, 3]

In [5]: sys.getrefcount(x)
Out[5]: 8

IPython helpfully stores the result of each execution in the Out[] array (like ans in Matlab), so just accessing x a few times causes the refcount to grow. I would have to manually edit the contents of Out[] to try to get x's refcount down to zero, and I'm actually not sure I could do that. A normal Python shell doesn't have this particular issue, but is still likely to create some unexpected references.

[EricCousineau-TRI]

That makes sense, did not think about that use case. Thanks!

Per the discussion on Slack with @jwnimmer-tri and @sammy-tri, the following options available are:

1. Do not permit users to directly extend the System / LeafSystem / VectorSystem/ etc. classes in Python; rather, add a proxy C++ class that, such that Python does not have to deal directly with unique_ptr<> ownership semantics.
2. Permit extending the classes in Python, and teach pybind11 how to deal with unique_ptr<> ownership semantics.
3. Add alternative APIs to passing unique_ptr<>. Pass non-owned instances via T* (like DiscreteValues).
4. Use shared_ptr<> everywhere. Suggested by pybind11 contributors / authors, employed by other toolkits, like ecto, where Tendril (analog of AbstractValue) and Cell (analog of System, denoted as noncopyable) are managed by shared_ptr<>. (I believe a similar sort of setup is used in the VTK Python bindings?)

The main caveat to avoid is to try and avoid forcing users to worry about Python references (such as the strict reference counting option in (2) that Robin mentioned) and also C++ object lifetime (such as permitting the object lifetime to be detached via (1)).

For (1), the proxy could possibly place something that is shared_ptr<>-compatible such that the user need not worry about lifetime of the Python-created class.
One caveat is that this structure would need to be duplicated for anything else that a user would like to extend.

For (1) and/or (2), the Drake codebase could be refactored in such a way that we have a unique_ptr<T, Deleter> (with Deleter = function<void(T*)>) used throughout the codebase. This type-erased deleter could then be used to (a) contain a reference to the Python object to keep it alive, and (b) signal when the object is destroyed in C++ to invalidate the Python reference in pybind11.

However, in all situations, people generating wrappers would need to ensure that any pure-references (such as returning a pointer to a C++ object that is not owned by the bound class) are returned as reference-only. There is still the possibility that this object will go out of scope in C++ before Python reference dry up, which would cause a (really annoying) segfault, especially in interactive sessions.

This could be mitigated by using some variant of pybind11::keep_alive mechanisms, possibly using a notification system similar to unique_ptr<> to invalidate references; or, when a mutable reference is not necessary, consider copying the object into a shared_ptr.

For (3), this may pose an issue if a Python object is destructed (seemingly disconnected to Python) when it is actually referenced by a C++ object.

For (4), it is possible to change API interfaces to use shared_ptr<T>, and still pass in rvalue unique_ptr<T>&&, as this is a permitted implicit conversion.

[dmsj]

weighing in as eager python users - we would like to use drake to find collision-free trajectories in dynamic environments where we are adding, removing, or moving bodies in the RigidBodyTree.

Since creating a new RigidBodyTree is expensive, we would like to add / update the bodies in the tree before each IK call.

For example, some of our current pydrake code:
self.robot = pydrake.rbtree.RigidBodyTree(self.config["ik_urdf"])

result = ik.InverseKin(self.robot, q_seed, q_seed, constraints, options)

result = ik.InverseKinTraj(self.robot, times, q_seed, q_seed, constraints, options)

we would like to be able to do something like:

self.robot.add_rigid_body(body)
self.robot.addCollisionElement(collision_element, body, "group1") analogous to tree_->addCollisionElement(collision_element, body, "group1");

Then we would like to do things like RigidBodyTree<T>::updateCollisionElements and possibly removeCollisionElements? and remove_rigid_body?

how do the aforementioned design choices affect this workflow? Are there better ways to do this?

[EricCousineau-TRI]

@dmsj The design choices above do not really affect RigidBodyTree; they are more focused on how things would work in the Systems Framework (customized user systems, ownership, etc.). If anything, they would change the pybind11 holder type to shared_ptr<>, but that change should not be visible to actual Python usage.

I've added #7251.
One thing is that the RigidBodyTree will soon be deprecated in lieu of MultibodyTree, but I have added this as a feature request in this comment in #7025.

[EricCousineau-TRI]

Posted a (large and messy) WIP PR to pybind11, and will see if the authors are available to look into it a little more within the next couple of weeks:
pybind/pybind11#1146

For now, I'll start generating basic bindings for existing System objects, and check on transitioning our usage of unique_ptr<> to shared_ptr<> per prior discussions, but will not worry about customization workflows until the pybind11 has some sort of resolution.
If the above PR is rejected, then we can take the proxy approach, and consider strategies to avoid opaque reference cycles when mixing shared_ptr<> and py::object references.

I will briefly investigate the usage of binder to see to what extent this can be used.

For a non-pybind11 route, @thduynguyen has mentioned a method similar to binder, but with custom parsing, that he had employed in gtsam:
https://bitbucket.org/gtborg/gtsam/src/7ecdbd5908d6/cython/?at=feature/cython_wrapper

[EricCousineau-TRI]

Given that #7590 has landed, I believe this issue is now satisfied per the scope of spike testing.
Will create a follow-up issue to address future deliverables.