bird-house / flyingpigeon

WPS processes for climate model data, indices and extreme events
http://flyingpigeon.readthedocs.io/en/latest/
Apache License 2.0
19 stars 15 forks source link

Error handling for FP algorithms #254

Closed Zeitsperre closed 5 years ago

Zeitsperre commented 6 years ago

@cehbrecht I'm looking at some of the error-handling within the processes found here and on the Ouranosinc fork and I'm wondering if the following is intentional. My goal is to implement a more pythonic (and Python2/3 compatible) approach to error-handling in generic processes. One method I've found in many processes is to raise a bare exception with a generic statement:

try:
    tarf = archive(results)
    LOGGER.info('Tar file prepared')
except:
    msg = 'Tar file preparation failed'
    LOGGER.exception(msg)
    raise Exception(msg)

or

except:
     LOGGER.exception('failed to write resources to textfile')

From what I understand, this catches any and all exceptions, including exceptions that can arise from other parts of FP. This also hides the actual error (SystemError, FileError, TypeError, etc.).

There's a lot of debate on what is the best way forward. Depending on whether or not the clause needs to be re-raised elsewhere, bare excepts can be useful (especially in production environments, from what I've read, e.g. https://github.com/PyCQA/pycodestyle/issues/703). I can see this generic error being useful for client-facing logs, but wouldn't it also make sense to catch the specific error within the log file?

I`ve also come across a few other error handling methods that either append the error to the message:

except Exception as e:
    msg = "Failed to create figure: {}".format(e)
    LOGGER.error(msg)
    raise Exception(msg)

or raise a portion of the stacktrace to the error (which makes a lot of sense, re: debugging):

except:
    raise Exception(traceback.format_exc())

For many of these processes, logging is not consistently implemented, which makes me wonder which algorithms we want to log (all of them? only those in development?). There's also the issue of different error-handling methods creating temporary/permanent files whereas other methods simply print to screen (e.g. do we want to erase the ones that are raised on the UI of PAVICS after they are presented?).

I guess my question here is which messages are presented to the user when an Exception occurs (raise, LOGGER) and are the bare except clauses that I'm finding intentional or should they be cleaned up?

TL;DR: What is a good convention for raising errors from processes?

cehbrecht commented 6 years ago

@Zeitsperre the exception handling is more "experimental" and it needs clean up. Some exceptions are "ignored" ... I don't think this is always intentional.

I can't give you good advice ... I would need this as well ;)

I sometimes catch exceptions to throw a more understandable error message which is shown to the user (using the wps exception protocol) ... but I log the full exception report. So, in this case exceptions are used for communication.

In other cases you might expect exceptions and you know how to handle those in your program logic. Maybe those are logged as warnings but they don't stop the program execution.

nilshempelmann commented 5 years ago

@Zeitsperre @cehbrecht In an oder version there was a guideline how to write a process and how to call infos, debugs, exeptions or errors. feel free to add a good practise here: https://birdhouse.readthedocs.io/en/latest/dev_guide.html#writing-functions

nilshempelmann commented 5 years ago

Moved to issue in birdhouse docs https://github.com/bird-house/birdhouse-docs/issues/32