RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.35k stars 1.27k forks source link

Add runtime enforcement of cache dependencies & remove symbolic inspection #12709

Open sherm1 opened 4 years ago

sherm1 commented 4 years ago

Per discussion with @SeanCurtis-TRI today and earlier discussions with @jwnimmer-tri:

Background

Loop detection in a Drake Diagram depends on accurate determination of the dependencies of output ports on particular input ports. Currently, that determination depends partially on under-the-covers transmogrification to symbolic form (as detailed in #12253). This causes much trouble (e.g. #12697) and doesn't always return the correct result (e.g. #12706). There is already a manual scheme for declaring output port dependencies ("tickets") which should be sufficient, but we don't trust users always to get that right and we don't currently have a way to check.

Proposal

Make it impossible to Eval() anything without having an appropriate declared ticket, without changing the public API. Specifically:

  1. Modify the Context to include a mutable stack of "ticket sets", the top of which is the only available set of dependency tickets.
  2. Modify the various Eval() methods to check that their own ticket is present in the top-of-stack set. Throw helpful message if not.
  3. Push this Eval's set of tickets on top of the stack and invoke Calc() method. Pop stack on return.
  4. ~Remove all attempts at using symbolics to determine pass through at Diagram build time.~ Defer to discussion in #12253.

We would leave the current "cache probing" build-time scheme (possibly with tweaks, also see #12253) that looks for blatant feedthrough, but we won't be able to catch lies (access to a resource for which no dependency has been declared) until an execution path reveals them, at which point we can throw a nice error message.

Question

Does that sound like it would work?

jwnimmer-tri commented 4 years ago

I don't understand why our disposition for Diagram-build-time checking needs to relate to this issue (the item (4) above)? This ticket proposes a strategy for runtime checking. Why would the runtime design differ depending on the details of how Diagram-build-time checking occurs? We have a nice thread going in #12253 about that topic. Let's not pollute this topic with those details?

sherm1 commented 4 years ago

In my mind they are related because adding bulletproof runtime checking reduces the need for extreme measures at build time.

jwnimmer-tri commented 4 years ago

But will the design of this feature change depending on that answer? If not, we can just implement this issue, and then come back to #12253 (or do them in parallel). There is a ton more to discuss about #12253, and I'd rather not spread that discussion between a half dozen tickets.

sherm1 commented 4 years ago

OK, I agree (mostly). The troubles with symbolic are part of the motivation for doing this sooner rather than later, so they aren't entirely uncoupled (in my mind). That said, no further need to discuss symbolics here.

jwnimmer-tri commented 4 months ago

Symbolic inspection leads to traps like #21707. It would be good to make it opt-in instead of opt-out.