Drake: Drake's internal assertions should be enabled by default

[jwnimmer-tri]

_Edited to add: The italics text is from a revised, round two proposal for this debate. (Struck-through text is from the first round, now removed.)_

(I may have misunderstood something, but here's my best summary. Please correct me if I'm wrong somewhere here.)

The default build type for Drake is a release build. This puts -DNDEBUG in the compiler flags. Drake code (drake/drake/**.{h,cc}) uses assert statements (from <cassert>). Those turn into no-ops when -DNDEBUG is in use. The developer quick setup ends up with us using release builds, and all the CIs that I can find use release builds as well. That means that almost nobody is testing whether the assertions pass!

_Drake's_ assertions should be enabled by default, and decoupled from whether -O2 and other speed-ups are in use. If we want to provide users a shotgun to point at their foot (turning off assertions for "speed"), that should be an orthogonal configuration option from everything else.

In other words, I propose

• (1) forbidding "assert" in drake code,
• (2) providing our own drake-specific assertion function(s) that muse be used instead.
• If there is a cry from the community to turn off our assertions, we can add a cmake option for that (and spin up a bunch more CI servers to test that build variant).
• _(3) By default, drake_assert being on or off will obey NDEBUG._
• _(4) Drake users can set an option to override the default, and turn drake_assert on or off independently._
• _(5) To the extent that pre-submit PR builds still do not have any debug variants, we turn on drake_assert for at least some PR builders._

How does the community feel about this kind of proposal?

[nate-tri]

Why not just remove -DNDEBUG from the release build and carry on with asserts?

[jwnimmer-tri]

Drake uses other libraries, and sometimes those libraries' headers may need to have -DNDEBUG in order to perform well (this is often this case with numerical libraries, when code lives inline in headers). I can live with turning off third-party assertions, but want to keep Drake's own assertions enabled by default.

[david-german-tri]

Why not just switch the default build type to debug?

[jwnimmer-tri]

My assumption was that debug gets you -O0, and my experience makes me think that -O0 on windows is intractably slow for this type of code.

[david-german-tri]

Triangulating between those arguments - why not create a third build type that is release-without-NDEBUG and make that the default?

• Release: NDEBUG, O3
• Dev/CI (default): O2
• Debug: O0

I'm not trying to nitpick here; it just seems like there should be a less intrusive solution.

[nate-tri]

@david-german-tri: I'm not sure I would agree with the idea that release builds have
assertions disabled as a matter of practice, but this is not my project.

On Thu, Mar 24, 2016 at 4:49 PM, David German [email protected]
wrote:

Triangulating between those arguments - why not create a third build type
that is release-without-NDEBUG and make that the default?

• Release: NDEBUG, O3
• Dev/CI (default): O2
• Debug: O0

I'm not trying to nitpick here; it just seems like there should be a less
intrusive solution.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/RobotLocomotion/drake/issues/1935#issuecomment-201014972

[jwnimmer-tri]

The basic premise is to have the ability to independently toggle Drake's assertions versus third-party assertions, per my comment above.

[david-german-tri]

Oh, I thought you were speaking to Release specifically there. You're saying you think third-party asserts should be off, even at development time, for performance reasons? That doesn't seem right to me unless the performance hit is pretty severe, in which case you have to wonder why the asserts were added in the first place.

[jwnimmer-tri]

Bottom line, I see "s/assert/drake_assert/g" in our tree as very cheap and easy, and worth doing up front. We can argue about what knobs should affect that function's behaviour (I'd start with "it's not possible to disable it, ever"), but I think spelling it as drake_assert from now on seems like a small burden to carry, and gives us lots of flexibility in tuning performance knobs down the road.

If we want to start with "no Drake builds ever use -DNDEBUG, and keep using assert()", I'd also support that as good-enough resolution to this issue.

[david-german-tri]

If you think the former is better, SGTM - we've probably exhausted the gains from quibbling. :-)

[jwnimmer-tri]

I'd like to hear what @RussTedrake thinks, in case these things have already been discussed. After that, I'll work on a PR.

[sherm1]

I don't think we should attempt to alter the expected behavior of the C++ standard assert() macro, which is that it is present in debug builds and gone in release builds. We will encounter lots of 3rd party code that has asserts in it, and the programmer who put it there was expecting it not to hurt release performance.

OTOH, for our own code how about a pair of custom assert macros something like DRAKE_ASSERT_DEBUG() and DRAKE_ASSERT_ALWAYS(). The latter form is for low-cost assertions that the programmer believes should be present in release code. The former acts like assert() and is suitable for expensive assertions. This pair would make the programmer's intention explicit, would allow us to control formatting, and would allow more control such as logging or compile time enabling of Drake's debug asserts in release builds.

[jwnimmer-tri]

That's pretty good. I would nitpick and call them DRAKE_EXPENSIVE_ASSERT() and DRAKE_ASSERT() perhaps (because the most commonly used one should have the shortest / most obvious name), but I don't want to too get hung up on the names. I can happily go with your names if they are generally liked better.

I agree that having the ability to control the failure formatting / handling is a nice bonus.

Even if a drake "release" build to the wider world turns off DRAKE_EXPENSIVE_ASSERT by default, I'm still going to argue for keeping it on by default for all day-to-day active in-tree development (and having CI coverage of keeping it enabled). Latent bugs are ~generally more problematic than a few CPU cycles. (Maybe we just have different guesses for how "expensive" the assertions that people will want to write will turn out be. Perhaps we can survey current assert()use in Drake and make a hypothesis.)

[sherm1]

I like your names too, although I would be inclined to use DRAKE_CHEAP_ASSERT() just to make the programmer think about it. People who have never seen a DRAKE_EXPENSIVE_ASSERT are otherwise likely to assume that DRAKE_ASSERT behaves like the standard assert().

I don't think you can assess potential cost by surveying existing code. Most will be very cheap-looking but whether they are expensive is a matter of context. For example, bounds checking for element access is so expensive that the standard has special at() methods that do it. But the code for the non-checked methods looks something like:

double operator[](size_t index) const {
  assert(i < size());
  return data[index];
}

Despite the innocuous expression, the correct translation would be assert->DRAKE_EXPENSIVE_ASSERT. (This is another case where the usual problem is failure to inline.)

At the other end of the spectrum I have seen asserts that invoke horrible O(n^2) checks to validate that some large data structure hasn't been corrupted.

That said, I'd be fine with having our normal development proceed with expensive asserts enabled as long as it is somehow visible and a little painful so that we don't accidentally leave them in for releases.

[jwnimmer-tri]

I think all of these proposals are way better than Drake-right-now. If I don't hear anything else by next week, I'll put together a PR.

Given that getting configuration permutations tested under CI seems to be underdeveloped / misunderstood right now, I may start with DRAKE_{CHEAP,EXPENSIVE}_ASSERT being never-disableable, and then file a second issue to add the build option and CI coverage concurrently.

[RussTedrake]

wow. haven't read the entire thread carefully, but at least one quick thought:

do NOT turn off NDEBUG. we use asserts for things that could cause performance hits in runtime code in a very small number of places in drake, but Eigen uses them everywhere. The difference in performance for Eigen code in debug mode vs release mode is incredible.

having said that, I always develop in debug mode. i would have thought that was standard practice. maybe we should add it to the developers guide?

[jwnimmer-tri]

The #2129 failure being missed by the PR CI builders makes me want to bump the priority of fixing our assertions so that they are, you know, ever run by developers and PR CI. I may work on this soon.

[RussTedrake]

I still think the assertions should be debug-mode only (and allowed to contain expensive checks).

[sammy-tri]

I would be more OK if assertions only fired in debug mode if it wasn't so easy to fall out of debug mode.. (see #2129). At any rate, +1 for having at least one PR builder in debug mode.

[jwnimmer-tri]

Russ, I guess that means you disagree with this ticket, and we should open a different one to update the developers guide to tell people how to turn on debug mode (it is _not easy_), and to add CI coverage for debug builds on PRs?

[RussTedrake]

that's my current stance, thought I'm certainly willing to listen and be convinced otherwise. But our existing code, and eigen in particular, does a lot of expensive checking in debug mode (via asserts) that should remain in debug mode. I've not had trouble developing in debug mode and testing (admittedly sometimes only via CI) in release.

[sherm1]

Somewhere in the thread above we talked about creating macros DRAKE_CHEAP_ASSERT() and DRAKE_EXPENSIVE_ASSERT() with the latter normally disabled except in Debug. I like that idea, and it introduces the possibility of a specific compile-time override that enables even the expensive ones in Release builds for development. I agree with Russ that we should not mess with NDEBUG or the behavior of the standard assert() macro.

[jwnimmer-tri]

My line of reasoning was "I want developers to be able to see when asserts trip, so we should turn on asserts more often", but perhaps the better line would be "I want developers to be able to see when asserts trip, so we should make it less-impossible to successfully execute a debug build". As such, I will return this ticket to the pool, and open a new issue to request that someone document how to achieve a debug build on Ubuntu. (_Edited: I've just reused #2129 for this purpose._)

[david-german-tri]

OK, since this was vetoed by @RussTedrake and everyone else seems to have come to terms with that (no activity in a month) I'm closing it.

[jwnimmer-tri]

I literally cannot comprehend how reconcile these statements:

• developers should be writing asserts in their new drake code;
• developers should have asserts turned on in drake when developing new code, otherwise how do you know if they are even syntactically correct (nevermind functionally correct);
• superbuild libraries have assert statements that make code run 100x (unbearably) slower;
• there shall be only one toggle for all of our asserts, the conventional NDEBUG.

It seems like the conclusion is "asserts must always be turned on during development, and thus all developers must suffer through 100x slower code, forever".

I am going to re-open this ticket, retitle it, and edit the summary with a perhaps more plausible consensus.

[jwnimmer-tri]

Top post has been edited now (hopefully clearly); re-opening the issue for discussion.

[sherm1]

I believe the solution that was more-or-less a consensus (and didn't get vetoed) was

• don't touch NDEBUG or the standard assert() behavior
• add a pair of macros, say DRAKE_ASSERT_ALWAYS and DRAKE_ASSERT_DEBUG that have always-on and debug-only default behavior but can be overridden at our discretion
• start using those macros instead of standard assert()

[jwnimmer-tri]

For the record, I can easily accept sherm1's proposal as well, though I may rue the PR code review griping of "this assertion too expensive, it should be DEBUG not ALWAYS". The thought with my new proposal in the top post is that in true release builds, asserts are off. But we can opt-in to having them on when we want them, even in "release" builds, which Drake developers should always do.

[RussTedrake]

i like the idea that true release builds have all assertions disabled. (that is the entire point of an assert statement, in my mind). So having a cmake option, or whatever, allowing developers to work with those assertions enabled but not incurring the cost of running eigen in debug mode seems reasonable to me.

[nate-tri]

As a note, I would like to point out that for programs that accept input
from disk or the network, if assertions are to be turned off, it's
important that the input validation logic not rely on assertions.

I think it would be misleading to title something "DRAKE_ASSERT_ALWAYS" if
in fact it does not always perform the assertion check.

On Fri, May 20, 2016 at 6:15 AM, Russ Tedrake [email protected]
wrote:

i like the idea that true release builds have all assertions disabled.
(that is the entire point of an assert statement, in my mind). So having a
cmake option, or whatever, allowing developers to work with those
assertions enabled but not incurring the cost of running eigen in debug
mode seems reasonable to me.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/RobotLocomotion/drake/issues/1935#issuecomment-220569011

[sherm1]

The idea with DRAKE_ASSERT_ALWAYS is that it represents some kind of cheap bugcatcher that is to be left on in Release builds (i.e. always!). It still should not be used for input validation checks though -- an assert is there to express invariants that are guaranteed to be true if the code is correct, regardless of user input. Typical use of assert_always is to check some precondition before entering an expensive computation.

[liangfok]

I think exceptions surrounded by if statements should be used to deal with invalid input from a disk or network since that would allow a meaningful message to be included with the thrown exception and printed to the console. Is the concern here that the logic within the if statement that is necessary to decide whether to throw an exception is too costly?

[liangfok]

As @jwnimmer-tri mentioned in the above-linked post, let's generalize this discussion to include a reusable DRAKE_ASSERT() macro for throwing exceptions. This macro will include details like the file name, method name, and line number, which will be useful for debugging. It will also allow us to modify all exceptions from a central location.

[jwnimmer-tri]

Work in progress will be here:
https://github.com/jwnimmer-tri/drake/tree/assertion-1935

[jwnimmer-tri]

As @jwnimmer-tri mentioned in the above-linked post, let's generalize this discussion to include a reusable DRAKE_ASSERT() macro for throwing exceptions...

I'm declaring that exception-related feature discussions now belong in #2605. This issue is only about assertions / aborts.

[jwnimmer-tri]

Next on the backlog: changing Drake's code to use DRAKE_ASSERT instead of assert(), and updating code_style.rst to forbid assert().

[jwnimmer-tri]

I believe the only remaining item is to make it easy / easier to arm DRAKE_ASSERT in release builds.