Request for Changes-91: Better error messages
- Author: Victor Poughon
- Additional Contributors (if different than authors): Manuel Grizonnet
- Submitted on 12.06.2017
- Proposed target release: 6.2
- Adopted / Rejected (with vote results)
- Link to a public git branch: https://git.orfeo-toolbox.org/otb.git/shortlog/refs/heads/better_error_messages
- Merged : (put merge commit when merged)
This RFC improves error messages displayed in otbcli, otbgui and Python bindings. It also improves consistency in the various error handling methods inside OTB and its applications (prefer exceptions), and adds an extensive test suite for many error paths.
Many error message currently displayed are confusing, and in some cases simply wrong. For example, when doing a simple otbcli_Convert -in blabla.tif -out /tmp/out.tif (the input file does not exist), the message displayed is:
2017 May 16 18:07:49 : Application.logger (CRITICAL) Invalid image filename blabla.tif. 2017 May 16 18:07:49 : Application.logger (FATAL) The following error occurred during application execution : /home/otbval/Dashboard/src/OTB/Modules/Wrappers/ApplicationEngine/include/otbWrapperInputImageParameter.txx:79: itk::ERROR: InputImageParameter(0x2158710): No input image or filename detected...
This branch proposes a shorter and more informative message:
2017-05-16 18:07:49 (FATAL): Cannot open file blabla.tif. The file does not exist.
Another strong example of why this RFC is needed is the behaviour of InputImageList. Currently, if any of the input files fails to be read it will silently catch the exception raised by UpdateOutputInformation and stop processing the list, then somehow display an unrelated error.
Error handling is inconsistent in OTB. We are using all of return codes (WrapperCommandLineLauncher), exceptions (otbAppLogFATAL) and silently returning NULL (CreateImageIO). This RFC does not fix this completely, but improves the situation by more cleanly separating error raising and handling (or at least working towards that goal).
There is a large (yet still incomplete) test suite of all error paths I managed to reproduce. The above is just an example, but the complete script output is here: Better_Error_Messages. This page includes, for each command, the output of this branch in release mode, this branch in debug and the current outputs (release-6.0) (which AFAIK are identical in Release and Debug).
Aiming to improve the signal-to-noise of error messages, changes include:
- Add type otb::ApplicationException so that otbLogFATAL can raise it instead of using itkGenericException.
- Create a otb::Logger for better control over log entries formatting:
- Only show DEBUG log level when compiled in debug mode
- Date output in ISO-8601 format
- Don't show logger output name (unused in OTB)
- Remove extra spaces and ellipsis, spell check error messages, harmonize language and tense used (could not vs cannot, etc.)
- Only show exception .what() in debug builds for common errors. Show a friendlier message in release builds.
- Remove many (but far from all) instances of the [try-catch anti-pattern ], and catch most exceptions in top level wrappers.
- Fix segfault when calling: otbApplicationLauncherCommandLine ''
Important places in the source code where there is top-level exception handling, formatting and displaying:
- itkBase.i (Python bindings)
Classes and files
In IO modules, throw more specific exception types, don't catch them, give them more information about the problem, etc:
M Modules/IO/IOGDAL/src/otbGDALImageIO.cxx M Modules/IO/ImageIO/include/otbImageFileReader.h M Modules/IO/ImageIO/include/otbImageFileReader.txx M Modules/IO/ImageIO/include/otbImageFileWriter.txx M Modules/IO/ImageIO/src/otbImageIOFactory.cxx
A Modules/Wrappers/ApplicationEngine/include/otbLogger.h A Modules/Wrappers/ApplicationEngine/src/otbLogger.cxx
In ApplicationEngine, use the new otb::Logger, add the new ApplicationException type:
In ParameterWrappers, don't catch exceptions, refactor message formatting:
M Modules/Wrappers/ApplicationEngine/include/otbWrapperInputImageParameter.txx M Modules/Wrappers/ApplicationEngine/include/otbWrapperMacros.h M Modules/Wrappers/ApplicationEngine/src/CMakeLists.txt M Modules/Wrappers/ApplicationEngine/src/otbWrapperApplication.cxx M Modules/Wrappers/ApplicationEngine/src/otbWrapperInputFilenameListParameter.cxx M Modules/Wrappers/ApplicationEngine/src/otbWrapperInputImageListParameter.cxx M Modules/Wrappers/ApplicationEngine/src/otbWrapperInputImageParameter.cxx
In wrappers, catch more specific exception to improve the displayed message:
M Modules/Wrappers/CommandLine/include/otbWrapperCommandLineLauncher.h M Modules/Wrappers/CommandLine/src/otbApplicationLauncherCommandLine.cxx M Modules/Wrappers/CommandLine/src/otbWrapperCommandLineLauncher.cxx M Modules/Wrappers/QtWidget/src/otbWrapperQtWidgetModel.cxx M Modules/Wrappers/SWIG/src/itkBase.i M Modules/Wrappers/SWIG/src/otbApplication.i
Fix "Monteverdi 2" name:
M Modules/Visualization/Mapla/src/mvdMaplaWin32.rc.in M Modules/Visualization/Monteverdi/src/mvdWin32.rc.in
New file in otb-data (fake tif file): Examples/notActuallyTif.tif.
Qt Wrapper can only be tested manually for this RFC. But the error handling code is very similar (see otbWrapperCommandLineLauncher.cxx and otbWrapperQtWidgetModel.cxx).
When working on this branch, I discovered that the Python bindings do not raise exceptions in case of missing mandatory parameters, but silently fail. You can see this behaviour here. This is not addressed in this RFC.