Usage of assert in CI/Release

[philipportner]

As I was working on error handling in #706 , I noticed test cases failing that should not be affected by my changes. While debugging I realized that we use assert and static_assert in a lot of kernels, and I triggered some by running tests with a debug build. I feel like this is a huge issue as assert does nothing if NDEBUG is defined, causing us to miss assertions one falsely relies on assert.

E.g., in scripts/algorithms/decisionTree_.daph in line 147 (link) we call upperTri with a matrix-of-ones of shape (belen, belen+1). In the upperTri kernel (link) there is an assert that should be triggered if the matrix is not square. In scripts/algorithms/decisionTree_.daph the matrix is never square is it's of shape (belen, belen+1), but the assertion is not triggered during our CI test suite.

In our CI we build with --target all (link), which means we build with -DCMAKE_BUILD_TYPE=Release, which in turn causes cmake to define NDEBUG -> none of the assert in our kernels check anything when running our test suite.

Is it intended that all the assert were not checked in the CI and during release builds? This feels like an oversight but I would love some clarification. As #706 tries to unify error handling, it also replaces all instances of assert with an appropriate throw ... expression, this now correctly tells us if code is being executed with unintended parameters independent of CMAKE_BUILD_TYPE.

[auge]

CI was added in parallel to the development work and my guess is that is was an oversight (not realizing that the binary is build in release configuration). I would suggest/vote for DEBUG builds for CI/testing and RELEASE for the deployment - which probably means to build in both configurations.

As you pointed out, checking input parameters using assert is not ideal as they are never checked in the RELEASE configuration.

[pdamme]

Thanks for bringing up this point, @philipportner. In the early days of the DAPHNE development, we used assertions for error handling (which has never been a conscious design decision); later on, we switched to exceptions for all new code. I guess, at some point we changed the build workflow in a way that caused assertions not to be checked anymore. This problem has been "known" (see #432). It's great that you clean up this inconsistency in #706, @philipportner.

To my mind, the CI is not the problem. Instead, it's really about replacing (almost) all assertions by exceptions, especially in the context of the validation of input data (to kernels etc.). With that, all situations that currently cause "silent" assertions in a release build should be turned into actual errors that are detected by our current CI setup.

In #432, @corepointer also pointed out that a release build may output debug information that could fail our test cases, since it's unexpected output from the test cases' point of view. However, he also said that could be addressed through configuring the logger appropriately.

So bottom line: Let's just change assertions into exceptions.

PS: Testing also the debug build during the CI would surely not hurt, but is not directly related to error handling, to my mind.

[corepointer]

+1 for switching from assert() to exceptions. -1 for doubling the load on the CI. I usually run the test suite in debug mode before merging something to main to catch these issues. Maybe we should make this a standard procedure.

[philipportner]

Thanks all for your input. I agree that we should switch from assert to throw. I have done so, and added a short sentence to the documentation, in #706 .

[philipportner]

closed with #706