py systems: DiagramBuilder.AddSystem() may not propagate existing keep-alive "edges"

[EricCousineau-TRI]

See Slack convo:
https://drakedevelopers.slack.com/archives/C3YB3EV5W/p1605703451063400

Currently, we have this for DiagramBuilder.AddSystem (which is locally correct):

drake/bindings/pydrake/systems/framework_py_semantics.cc

Lines 454 to 461 in 6d56ea0

	.def(
	"AddSystem",
	[](DiagramBuilder<T>* self, unique_ptr<System<T>> system) {
	return self->AddSystem(std::move(system));
	},
	py::arg("system"),
	// Keep alive, ownership: `system` keeps `self` alive.
	py::keep_alive<2, 1>(), doc.DiagramBuilder.AddSystem.doc)

But @RussTedrake ran into an edge case where the following code would segfault without the workaround to keep controller_plant alive:
https://github.com/RussTedrake/manipulation/blob/805d4c582268b3803e90125266803e13dbc9ddbc/manipulation_station.ipynb

This keep_alive on the ID looks fine:

drake/bindings/pydrake/systems/controllers_py.cc

Line 95 in 6d56ea0

py::keep_alive<1, 2>(),

But most likely, the PyObject for the InverseDynamics evaporates since it's inside the MakeManipulationStation() function, so the keep_alive "graph" evaporates, so nothing is there to keep the controller_plant alive.

Hack solution is to just add a reference cycle for now, and reap the issues later :(
Something like:

AddSystem ...
// Keep alive, ownership: `self` keeps `return` alive
// HACKHACHKACHKHACK
keep_alive<1, 0>(),

\cc @SeanCurtis-TRI

[EricCousineau-TRI]

Also, Russ pointed out that this is probably an issue with LCM systems in Python too.

And now that I look, it appears that I worked around this in Anzu code too 🤦 (this was in mid-June 2020)
#13075 was insufficient

# TODO(eric.cousineau): Keep alive fixes:
# - https://github.com/robotlocomotion/drake/pull/13075 - didn't work :(
# - InverseDynamicsController - doesn't keep control plant alive
_keep_alive = []

...

def build_arm_control(
    builder,
    plant,
    arm_instance,
    *,
    kp=np.array([2000., 1500, 1500, 1500, 1500, 500, 500]),
    kd=None,
    use_velocity_control=True,
    time_step=0.005,
):
...
    control_plant, _ = make_control_model(plant, [arm_instance])
    # TODO(eric.cousineau): Remove the need for `_keep_alive` hack.
    _keep_alive.append(control_plant)
...
    controller = builder.AddSystem(InverseDynamicsController(
        robot=control_plant, kp=kp, ki=ki, kd=kd,
        has_reference_acceleration=False))
...

[RussTedrake]

Thanks @EricCousineau-TRI . I'm not sure it's that much of an edge case. It seems that any time I'm hoping for ownership for a system that is pushed through AddSystem, the keep_alive semantics will be lost. That might happen often?

[EricCousineau-TRI]

It might happen often, yup!

And to modify your statement: keep_alive semantics will be lost if you don't keep a reference to the original system hanging around in Python.

[EricCousineau-TRI]

This is one of the "ownership trees" that Jeremy mentioned should be documented: #13058