py systems: Ensure usage plays well with caching (invalidation)

[EricCousineau-TRI]

Per discussion here and on slack, copying transcript(s):

From @russtedrake:

I'm torn about walking the line with const access. I like the idea of the python code being simpler to read, but are we playing with fire? And even if python can't enforce it, how do the actual pybind bindings get away with writing to a const reference?

From me:

Unfortunately, this is a limitation of pybind: http://pybind11.readthedocs.io/en/stable/limitations.html

If it's an issue, then I could perhaps tinker some with our pybind fork to make it more strict w.r.t. to const (decorate instances as such), but this is at the risk of making it really hard to get into upstream pybind, as I think it would complicate the implementation details quite a bit - multiple Python instances (const and mutable) for what used to be a single Python instance.

That being said, I think it would probably take a couple of days to spike test.

@sherm1 For caching, can I ask if cache invalidation is done through access to mutators (e.g. calling get_mutable_state() will invalidate the ticket for the state), or is it some other way? I ask because if we keep pybind as-is, and if cache invalidation is triggered by mutator access, then a Python user could potentially bypass this by calling get_state and mutating it there.

From @jwnimmer-tri:

My recollection is that yes, the get_mutable_stuff() vs get_stuff() is part of the contract. One proposal is to have the Python always call get_mutable_stuff (for either flavor). Possibly with a readonly = True kwarg if they promise not to write through it.

From me:

I'm concerned about performance impact for Python users, but yes, that would be an excellent first step - possibly binding "get_stuff" in Python to "get_mutable_stuff" just for shorter access, and add the readonly argument.

From @jwnimmer-tri:

I mean, I guess our bindings for get_stuff could return a python object decorator, that forwarded const methods to the wrapped C++ object, and threw on non-const ones?

From me:

Ooh, that'd work. It could be a proxy object, such that it will forward all other access too (in such a way that it'd prevent mutable methods from being called).

From @jwnimmer-tri:

https://github.com/pybind/pybind11/issues/717 some related discussion

\cc @amcastro-tri

[EricCousineau-TRI]

To list potential solutions:

• [ ] (Stopgap) Keep get_vector and get_mutable_vector exposed in Python, but bind both to get_mutable_vector in C++.
• [ ] Use dual instance (const and mutable) prototype mentioned in pybind/pybind11#717
• [ ] Use const-Python proxy object, with the appropriate type_caster in Pybind*.
• [ ] For mutation, only bind SetVector(...). (This would be effective only if get_vector() is correctly const in Python, or always returns a copy.)

* May still require changes to core pybind, unless there's an easy way register implicit pointer conversion.

EDIT: Updated with Sherm's alternative.

[sherm1]

Possible alternative:

• Make sure there are "Set" methods for anything in context we care about setting.
• In Python, bind only the get_vector() and SetVector() methods, so we don't have to worry about invalidation side effects, and (worse) missed invalidations due to handing someone a mutable reference.
• I would then advocate using get/Set only in C++ as well, except for extremely advanced users who could be trusted not to make hidden changes through mutable objects (that's approximately nobody).

[EricCousineau-TRI]

Thanks!

I'd be hesitant to limit mutators in Python to SetVector(...), as (a) for direct access to raw vectors as references, NumPy already handles const-ness, and (b) for indirect access to raw vectors (e.g. BasicVector), that means extra copy operations (and overhead) for ensuring the bindings (or C++ code) have SetX(...).

For large vectors, that means you're now required to always copy when you only want to change a small bit.

Caveat: For NumPy arrays with dtype=object (e.g. AutoDiffXd and Expression), we cannot have direct access to MatrixX<T>::data() in Python, since dtype=object does not directly map onto that memory (it creates an array of Python instances, rather than using the existing array, as it does with double). There may be workaround: https://github.com/pybind/pybind11/pull/1152#issuecomment-355725117

So with that said, maybe ensuring that SetVector(...) is always available when dealing with matrices is the most robust, consistent solution.

[EricCousineau-TRI]

I've thrown together a prototype for the const-proxy method, first testing in Python: cpp_const.py cpp_const_test.py

I believe this could then be handled with custom wrappings of pybind methods (not using the type_caster setup), since this would not allow us to intercept const T&.

I have a prototype of doing this wrapping here: wrap_function.cc wrap_function.output.txt (which also paves the way toward a workaround for callbacks + references as mentioned in https://github.com/pybind/pybind11/issues/1241)

[EricCousineau-TRI]

@sherm1 Here's a prototype of teaching Python and pybind how to deal with const-ness (third bullet point from the above comment): C++ Pybind Code - cpp_const_pybind_test_py.cc Python Code, using C++ - cpp_const_pybind_test.py

How does this look to you?

[sherm1]

How does this look to you?

Sorry, @EricCousineau-TRI -- I'm unable to extrapolate from that code what it would mean in practice in Drake. I don't have an opinion about how it should look in Python; my concern is just that we must avoid unintentional invalidation.

[EricCousineau-TRI]

Hup, sorry about that! This would prevent unintentional invalidation by ensuring that non-mutable accessors, e.g. get_vector, returns an object that does not allow modification in Python, preserving C++ semantics such that we can rely on triggering cache invalidation via get_mutable_vector accessors in both C++ and Python. (The with self.ex*(): statements imply that these blocks throw an error.) The C++ code implies that we don't have to do anything too special to make this work.

[EricCousineau-TRI]

Follow-up slack convo: https://drakedevelopers.slack.com/archives/C3YB3EV5W/p1549562118060400

Discussion is about on par with prior discussion: Either use getters / setters, or propagate const access.

At some point in the future, I need to draw up a minor usability doc and all edge cases to more explicitly capture pain points with both solutions.

[EricCousineau-TRI]

I'm not sure if I've seen people get bit by this. We have the (production-level) code ready for this if it ever arises, but less work is always nicer.

Closing for now, and may consider removing cpp_const since it's not yet used.

[EricCousineau-TRI]

This came up in Drake Slack: https://drakedevelopers.slack.com/archives/C3YB3EV5W/p1626366534036100 (\cc @rpoyner-tri)

FTR cpp_const was removed in #14270