Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow the build to be configured in "fail on error" mode #687

Merged
merged 13 commits into from
Feb 9, 2021
Merged

feat: Allow the build to be configured in "fail on error" mode #687

merged 13 commits into from
Feb 9, 2021

Conversation

HadrienG2
Copy link
Contributor

This PR enables the Acts logger to be configured in such a way that all log messages above a certain debug level (typically WARNING or ERROR) cause an exception to be thrown.

This is useful because Acts has a certain number of non-fatal error scenarios, which should not crash real-world physics reconstruction but indicate suspicious behavior that we will usually want to catch in automated testing (where we can control the inputs and make sure that non-fatal errors do not occur "by chance").

Throwing an exception on logging did not compose well with the Logger::OutStream mechanism, which would emit logs in its destructor, so after some discussion with @paulgessinger I decided to fix this by removing this mechanism and reworking the logger API in a much simpler direction. After all, logging is not meant to be fast, so passing logs as std::strings is okay as long as there's a way for the caller to avoid generating the std::string when it's not going to be printed.

Since Logger is not documented to be part of the public Acts API, I do not consider this API rework to be a breaking change.

Fixes #482 .

@HadrienG2 HadrienG2 added Component - Core Affects the Core module Feature Development to integrate a new feature labels Feb 1, 2021
@HadrienG2 HadrienG2 added this to the next milestone Feb 1, 2021
@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Feb 1, 2021

This PR does not yet enable the newly introduced functionality in CI for two reasons:

  1. Stacking CI and code changes in a single PR seems counter to the spirit of using semantic labels for PRs. So this should probably be taken care of in a follow-up PR with a "chore" label.
  2. It would lead to a red CI due to a previously uncaught bug in AtlasStepperTests, which is caught by the new mechanism, and which I don't know how to fix. The symptoms look suspiciously similar to Fatal Error in local-to-global transformations #579 .

@HadrienG2
Copy link
Contributor Author

@paulgessinger It looks like the macOS build is misbehaving. I think we already observed a similar issue before?

@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #687 (2350d1a) into master (5c6bbbb) will increase coverage by 0.03%.
The diff coverage is 16.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
+ Coverage   49.00%   49.03%   +0.03%     
==========================================
  Files         331      331              
  Lines       16549    16549              
  Branches     7723     7725       +2     
==========================================
+ Hits         8110     8115       +5     
- Misses       3009     3013       +4     
+ Partials     5430     5421       -9     
Impacted Files Coverage Δ
Core/include/Acts/Propagator/Navigator.hpp 57.54% <0.00%> (-1.09%) ⬇️
Core/src/Geometry/TrackingVolume.cpp 43.36% <0.00%> (+1.30%) ⬆️
Core/src/Utilities/Logger.cpp 44.00% <0.00%> (+8.00%) ⬆️
Core/include/Acts/Utilities/Logger.hpp 61.22% <47.36%> (-2.42%) ⬇️
Core/src/Geometry/CylinderVolumeHelper.cpp 29.25% <0.00%> (-0.16%) ⬇️
...e/include/Acts/Vertexing/IterativeVertexFinder.ipp 22.91% <0.00%> (+1.66%) ⬆️
Core/include/Acts/Geometry/SurfaceArrayCreator.hpp 41.81% <0.00%> (+1.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c6bbbb...a1a5f88. Read the comment docs.

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's override the coverage report.

@asalzburger asalzburger merged commit 478a565 into acts-project:master Feb 9, 2021
@HadrienG2 HadrienG2 deleted the fail-on-error branch February 9, 2021 12:54
@paulgessinger paulgessinger modified the milestones: next, v6.0.0 Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Examples should have a "fail on error" mode
3 participants