framework_py: Expose AbstractValue and Value<> instantiations.

[EricCousineau-TRI]

Partially addresses the AbstractValues bullet point of #7660. Next step is to implement unrestricted updates.

This change is 

[EricCousineau-TRI]

+@jadecastro for feature review, please.
+@sherm1 for platform review, please.

\cc @jwnimmer-tri Regarding the potential lifetime issues with Value<>::set_value for non-copyable types, can I ask if you have a suggestion on how to address this? (see comments in systems_pybind.h)

---

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

---

Comments from Reviewable

[EricCousineau-TRI]

Follow up: Just thinking about it more, this caveat (deleting memory in C++ due to unique_ptr<>::reset() which is still referenced in Python) is something that we will face in other areas, aside from just usage of unique_ptr<>, such as passing Eigen::Ref<> to Python. The memory is valid unless you resize the original matrix, in which case the pre-resize memory is not guaranteed to be valid.

In this case, I'll add some __doc__ to the accessor / mutator methods with a warning, telling users to avoid storing direct references when possible, and just call get_value() if they want to access values.

[sherm1]

I'd like to wait until at least a round of feature review before platforming.

---

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

---

Comments from Reviewable

[jadecastro]

Pass complete.

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/systems/framework_py.cc, line 434 at r1 (raw file):

    .def("__copy__", &AbstractValue::Clone)
    // Use only exception variant.
    .def("SetFrom", &AbstractValue::SetFromOrThrow)

BTW, here the spelling binding name is different from the class name.

---

bindings/pydrake/systems/systems_pybind.h, line 18 at r1 (raw file):

/// @param scope Parent scope.
/// @tparam T Inner parameter of `Value<T>`.
/// @tparam Class Class to be bound. By default, `Value<T>` is used.

BTW, It seems like this binding is designed to wrap classes like Value and BasicVector, but not others. Could this be documented more explicitly?

---

bindings/pydrake/systems/systems_pybind.h, line 38 at r1 (raw file):

    // dangling Python reference, since the `unique_ptr` will be reset
    // internally.
    // TODO(eric.cousineau): Resolve this, either by (a) using a snowflake

BTW, I'm not following what you mean in option (a). Consider rephrasing this.

---

Comments from Reviewable

[EricCousineau-TRI]

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

---

bindings/pydrake/systems/framework_py.cc, line 434 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, here the spelling binding name is different from the class name.

Done. (Added rationale for why this was done.)

---

bindings/pydrake/systems/systems_pybind.h, line 18 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, It seems like this binding is designed to wrap classes like Value and BasicVector, but not others. Could this be documented more explicitly?

Done.

---

bindings/pydrake/systems/systems_pybind.h, line 38 at r1 (raw file):

Previously, jadecastro (Jonathan DeCastro) wrote…

BTW, I'm not following what you mean in option (a). Consider rephrasing this.

Done. Given the comment I made at the top, I've bubbled this up as a user-visible docstring.

---

Comments from Reviewable

[sherm1]

Platform with a few BTWs.

---

Reviewed 2 of 6 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/systems/BUILD.bazel, line 72 at r2 (raw file):

    py_deps = [
        ":module_py",
    ] + systems_pybind_py,

BTW I hadn't seen this before -- is that just to avoid repeating the long name?

---

bindings/pydrake/systems/framework_py.cc, line 453 at r2 (raw file):

    // Override `Value<py::object>::Clone()` to perform a shallow copy on the
    // object.
    std::unique_ptr<AbstractValue> Clone() const override {

BTW just want to make sure this is what you intended. Value<T> always does a deep copy in C++ Drake but here it is being made shallow. Can you explain why?

---

bindings/pydrake/systems/test/value_test.py, line 79 at r2 (raw file):

    def test_abstract_value_move_only(self):
        obj = MoveOnlyType(10)
        # This *always* copies the object.

BTW unclear which object -- presumably the move-only object can't be copied?

---

Comments from Reviewable

[jadecastro]

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[EricCousineau-TRI]

Added a generalization of the emplace constructor.

---

Review status: 1 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

bindings/pydrake/systems/BUILD.bazel, line 72 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW I hadn't seen this before -- is that just to avoid repeating the long name?

Done. This was more of a way to try and bundle transient dependencies of mixed types - I haven't tried out a way to try and make a provider which can provide both Python and C++ dependencies (e.g. cpp_template_py and cpp_template_pybind).

For now, just removed the alias, and used the target.
TBH, I could remove cpp_template_py from test_util_py, but I'll defer that to some later point.

---

bindings/pydrake/systems/framework_py.cc, line 453 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW just want to make sure this is what you intended. Value<T> always does a deep copy in C++ Drake but here it is being made shallow. Can you explain why?

Done.
No other reason other than I was being hesitant on committing to deepcopy for fear of cycles; but after reading the documentation for __deepcopy__, and seeing the memo dictionary, it looks like that's a moot point.

---

bindings/pydrake/systems/test/value_test.py, line 79 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW unclear which object -- presumably the move-only object can't be copied?

Done. Clarified both the action, cloning rather than copying, and obj rather than the object.

---

Comments from Reviewable

[jadecastro]

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable