ImperialCollegeLondon / ImperialCollegeLondon/virtual_ecosystem

Approach to logging and error handling

Open
#160 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
20
Forks
5
Avg merge
2d 1h
Merged PRs (30d)
34

Description

@alexdewar raised this point in a comment on #158 (see that link for a bit more context and discussion) and I have converted it to an issue to keep it live when I merge that PR.

This is def a cleaner approach than the `log_and_raise` business that came before and this PR could totally be merged as is. I do have some broader suggestions about how error handling is being done in VR though, which could be dealt with at another point.

Firstly, you're using the `critical` log level everywhere when it should probably be `error`. Critical errors are generally (I think) *unrecoverable* errors, but if you're re-raising an exception then it's not really unrecoverable by definition, because the caller could catch it and continue execution of the program. You could have places where you use the critical log level, but these really should be higher-level errors (e.g. "failed to load data files") at a higher level in the call stack, rather than lower-level stuff (e.g. "data file X is missing required property Y"), and it should really be the last thing you do before you terminate the program. It's fine to just call `sys.exit(1)` if your error really is unrecoverable.

Secondly, while it's good that you've removed the `log_and_raise` function, I think the general approach of "log an exception then (re-)raise it" isn't the best way to handle some of the errors you're dealing with. Let's take the `validate_array` function, for example. There are three places where you raise errors in this function, logging the errors before doing so. `validate_array` is currently only called in `Data.__setitem__`. To my mind, if you *are* going to catch and re-raise errors for this (though I'm not sure that it's necessary), it would make more sense to do it there rather than inside `validate_array`, e.g.:

```py
try:
# Validate and store the data array
value, valid_dict = validate_dataarray(value=value, grid=self.grid)
self.data[key] = value
self.variable_validation[key] = valid_dict
except Exception as ex:
LOGGER.error(f"Data validation failed: {str(ex)}")
raise
```

Note that this also tells gives the user more context as to what the errors are actually about. That said, in this case, personally I would probably raise a new exception here (maybe `from ex`), rather than logging the error and then the caller (or the caller's caller) can decide what to do with it.

An alternative approach to explicitly logging and reraising errors everywhere would be to write a custom handler for uncaught exceptions, by overriding [`sys.excepthook`](https://docs.python.org/3/library/sys.html#sys.excepthook). That way you'd only have to write this boilerplate error-handling code once, rather than every time any error happens.

In general, I'm not convinced that logging statements need to be put in the code every time an exception is raised anyway. If the exception isn't caught at *any point* in the call stack, then, yes, it should definitely go in your program log, but there are cleaner ways of achieving this than by having logging statements accompanying every time an error is raised (remember: DRY).* On the other hand, if an exception *is* caught, that kind of implies that the caller was expecting it, in which case, reporting the "error" isn't likely to be helpful anyway. Note that I'm not saying logging isn't important! For `validate_dataarray` for example, I would imagine that if something goes wrong in there then the program will exit 100% of the time, in which case all of those errors would percolate all of the way up the call stack and be logged anyway. The extra effort of explicitly logging these errors where they are raised is unnecessary and clutters things.

\* One way is overriding `sys.excepthook`, as I suggested, but another way is to just wrap the entry point to your program in `try/except` block and do any extra handling you want to there.

_Originally posted by @alexdewar in https://github.com/ImperialCollegeLondon/virtual_rainforest/pull/158#pullrequestreview-1279403226_

Contributor guide

Open the contributing guide

Research direction

Start by reading PR #158 and tracing the named error paths through validate_array, validate_dataarray, and Data.__setitem__. Compare the alternatives raised here, including sys.excepthook and handling at the program entry point; done means the project has an agreed, consistent logging and exception-handling approach with redundant logging removed.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.