py systems: Add keep_alive cycle to DiagramBuilder.AddSystem #14356
Conversation
There was a problem hiding this comment.
+@RussTedrake for feature review, please!
+@sammy-tri for platform review, please!
On the whole, my feelings for this PR: 🤮
But it works, so my more dominant feelings as well: 🤷
Reviewable status: LGTM missing from assignees RussTedrake(platform),sammy-tri(platform) (waiting on @RussTedrake and @sammy-tri)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @EricCousineau-TRI and @sammy-tri)
bindings/pydrake/systems/framework_py_semantics.cc, line 461 at r1 (raw file):
py::arg("system"), // TODO(eric.cousineau): These two keep_alive's purposely form a // reference cycle as a workaround or #14355. We should find a
nit: or => for
EricCousineau-TRI
force-pushed
the
issue-14355-keep-alive-cycle
branch
from
c6d5808
to
7dd7510
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @RussTedrake and @sammy-tri)
bindings/pydrake/systems/framework_py_semantics.cc, line 461 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
nit: or => for
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)
There was a problem hiding this comment.
Not a huge fan of this "workaround" myself but after spelunking in the issues/discussions I guess I could l live with it. Holding off final review until CI is happy.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee sammy-tri(platform)
This is a workaround to ensure we propagate keep_alive relationships Resolves RobotLocomotion#14355
EricCousineau-TRI
force-pushed
the
issue-14355-keep-alive-cycle
branch
from
7dd7510
to
4063c1b
Compare
There was a problem hiding this comment.
Fixed CI error - had to propagate new expectations due to the hack
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @RussTedrake and @sammy-tri)
|
Yup, looks like it's good to go! |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all discussions resolved, LGTM from assignees sammy-tri(platform),RussTedrake(platform)
This is a workaround to ensure we propagate keep_alive relationships
Resolves #14355
This change is