Fix catching of generic Exceptions

XMLWordPrintable

Details

• Type: Epic
• Status: To Do
• Resolution: Unresolved
• Fix Version/s: None
• Component/s: None
• Labels:
• Epic Name:
generic Exceptions

Description

There are a lot of except Exception blocks in our code. These blocks often just swallow the error, either logging a message or just continue-ing without saying anything. In almost every case (I can't prove it's every case without further exploration), we should only catch a very specific and short list of exceptions. The same applies to except lsst.pex.exceptions.Exception and except pexExcept.Exception.

I'm making this an epic as I started filling in the components list and ended up with just about every LSST owned package, so I figure this is best done on a package-by-package basis, or at least on groups of packages.

Filing as critical, as we already know of cases where this bug has hidden other problems (e.g. DM-13803).

Activity

Hide
Kian-Tat Lim added a comment -

While the general principle is correct, I'm not sure we should ban cases like this:

 try:  do_something_specific except Exception as e:  log.debug("Couldn't do something: %s", e)  do_something_in_another_way 

where overall failure in the case of an unexpected exception is undesirable.

Show
Kian-Tat Lim added a comment - While the general principle is correct, I'm not sure we should ban cases like this: try : do_something_specific except Exception as e: log.debug( "Couldn't do something: %s" , e) do_something_in_another_way where overall failure in the case of an unexpected exception is undesirable.
Hide
John Parejko added a comment -

do_something_specific has a handful of known exceptions it can raise. If something else gets raised, something strange is probably going wrong. If an exception outside those defined for do_something_specific is raised, how do you have any idea if do_something_in_another_way could possibly work?

Show
John Parejko added a comment - do_something_specific has a handful of known exceptions it can raise. If something else gets raised, something strange is probably going wrong. If an exception outside those defined for do_something_specific is raised, how do you have any idea if do_something_in_another_way could possibly work?
Hide
Russell Owen added a comment -

We have a number of cases where we want to make a "best effort" to do something, and continue if not. Plotting code is typical. In such cases I think it is appropriate to log and move on.

We also have a lot of code that does not document what exceptions it may raise. And even if all our code did document the exceptions it could raise, that documentation is likely to go out of date as our code changes. It can be very tricky to maintain such documentation because code calls other code that calls other code...

I agree that logging exceptions is much better than silently ignoring them. I am much less sanguine that we can always catch just a few specific exceptions. Sometimes we certainly can. But for complex code it is often safest to cast a broader net, which in Python usually means catching Exception.

Show
Russell Owen added a comment - We have a number of cases where we want to make a "best effort" to do something, and continue if not. Plotting code is typical. In such cases I think it is appropriate to log and move on. We also have a lot of code that does not document what exceptions it may raise. And even if all our code did document the exceptions it could raise, that documentation is likely to go out of date as our code changes. It can be very tricky to maintain such documentation because code calls other code that calls other code... I agree that logging exceptions is much better than silently ignoring them. I am much less sanguine that we can always catch just a few specific exceptions. Sometimes we certainly can. But for complex code it is often safest to cast a broader net, which in Python usually means catching Exception .
Hide
John Parejko added a comment -

Having just been burned by this again just now in makeCoaddTempExp (whose run contains a for loop with 4 "catch and mask all exceptions" blocks!), I'm going to try to make a list of these cases. At absolutely bare minimum, they should be modified to respect doRaise.

Show
John Parejko added a comment - Having just been burned by this again just now in makeCoaddTempExp (whose run contains a for loop with 4 "catch and mask all exceptions" blocks!), I'm going to try to make a list of these cases. At absolutely bare minimum, they should be modified to respect doRaise .
Hide
Merlin Fisher-Levine added a comment -

Show

People

Assignee:
Unassigned
Reporter:
John Parejko
Watchers:
Hsin-Fang Chiang, Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Merlin Fisher-Levine, Michael Wood-Vasey, Russell Owen, Tim Jenness, Yusra AlSayyad