Re-raising a RuntimeError - good practice?

[Victor Hooi]

Hi,

I have a Python class that represents a loading job.

Each job has a run_all() method that calls a number of other class methods.

I'm calling run_all() on a bunch of jobs.

Some of methods called by run_all() can raise exceptions (e.g. missing files, DB connection failures) which I'm catching and logging.

If any of the methods fails, I'd like to terminate running that job, and move onto the next job.

I'm currently re-raising a RuntimeError, so that I can break out the run_all() and move onto the next job.

For example:

def run_all(self):
self.logger.debug('Running loading job for %s' % self.friendly_name)
try:
self.export_to_csv()
self.gzip_csv_file()
self.upload_to_foo()
self.load_foo_to_bar()
except RuntimeError as e:
self.logger.error('Error running job %s' % self.friendly_name)
....
def export_to_csv(self):
...
try:
with open(self.export_sql_file, 'r') as f:
self.logger.debug('Attempting to read in SQL export statement from %s' % self.export_sql_file)
self.export_sql_statement = f.read()
self.logger.debug('Successfully read in SQL export statement')
except Exception as e:
self.logger.error('Error reading in %s - %s' % (self.export_sql_file, e), exc_info=True)
raise RuntimeError

My question is - is the above Pythonic, or an acceptable practice?

Or is there another way I should be handling errors, and moving on from failures, and if so what is it please?

Cheers,
Victor

 

[Steven D'Aprano]

Hi,

I have a Python class that represents a loading job.

Each job has a run_all() method that calls a number of other class
methods.

I'm calling run_all() on a bunch of jobs.

Some of methods called by run_all() can raise exceptions (e.g. missing
files, DB connection failures) which I'm catching and logging.

If any of the methods fails, I'd like to terminate running that job, and
move onto the next job.

That should be simple:

for job in many_jobs:
try:
job.run_all()
except RunAllException as err:
logger.error(err)


Another approach is to put all your error handling in the one place (or
as few places as possible):

for job in jobs:
try:
try:
job.run_all()
except Exception as err: # catch *everything*
logger.error(err)
raise
except (SpamError, EggsError, CheeseError):
# We expect these exceptions, and ignore them.
# Everything else is a bug.
pass


With this approach, only your top-level code needs to care about catching
exceptions. Although some of your lower-level code may do so as well, but
it's (potentially) no longer their responsibility to deal with logging.

However, the down-side of this approach is that the list of "ignorable
exceptions" needs to be kept small and manageable. That may requires
either diligence on your part, tracking which exceptions can be raised,
or having each method be responsible for generating the right exceptions.
See below.

I'm currently re-raising a RuntimeError, so that I can break out the
run_all() and move onto the next job.

For example:

def run_all(self):
self.logger.debug('Running loading job for %s' %
self.friendly_name)
try:
self.export_to_csv()
self.gzip_csv_file()
self.upload_to_foo()
self.load_foo_to_bar()
except RuntimeError as e:
self.logger.error('Error running job %s' %
self.friendly_name)

At first glance, that looks pretty nasty to me. The *actual* exception
causing the problem is lost. This is one step up from the infamous, and
horrible

"An error occurred"

message. At least you report the name of the job that failed, but you
don't report the actual error, or which component failed.

Fortunately the actual error is logged by the method which fails, which
makes it much better, but suggests that this exception handler isn't
doing anything: the error message it generates is pointless and is just
noise in the log file, so you ought to get rid of it.

def export_to_csv(self):
...
try:
with open(self.export_sql_file, 'r') as f:
self.logger.debug('Attempting to read in SQL export
statement from %s' % self.export_sql_file)
self.export_sql_statement = f.read()
self.logger.debug('Successfully read in SQL export
statement')
except Exception as e:
self.logger.error('Error reading in %s - %s' %
(self.export_sql_file, e), exc_info=True)
raise RuntimeError

My question is - is the above Pythonic, or an acceptable practice?

It's certainly acceptable practice, but the downside is that *every*
method needs to care about catching exceptions and logging them. It's
better to push as much of that responsibility as you can out of the
individual methods and into the caller.

Sometimes, though, a method may have to deal with too many different
possible exceptions, which makes managing the list of expected exceptions
to ignore too hard. In that case, use custom exceptions:


class SpamException(Exception):
pass


class Job:
def export_to_csv(self):
...
try:
with open(self.export_sql_file, 'r') as f:
self.logger.debug('Attempting to read in SQL export
statement from %s' % self.export_sql_file)
self.export_sql_statement = f.read()
self.logger.debug('Successfully read in SQL export
statement')
except IOError, OSError as err:
# Low-level methods should always be conservative in what they
# catch, not too eager to cover up bugs in the code.
raise SpamError("blah blah blah", err)

or similar. E.g. you can use RuntimeError, as you do.

 

[Andrew Berg]

For example:

def run_all(self):
self.logger.debug('Running loading job for %s' % self.friendly_name)
try:
self.export_to_csv()
self.gzip_csv_file()
self.upload_to_foo()
self.load_foo_to_bar()
except RuntimeError as e:
self.logger.error('Error running job %s' % self.friendly_name)
...
def export_to_csv(self):
...
try:
with open(self.export_sql_file, 'r') as f:
self.logger.debug('Attempting to read in SQL export statement from %s' % self.export_sql_file)
self.export_sql_statement = f.read()
self.logger.debug('Successfully read in SQL export statement')
except Exception as e:
self.logger.error('Error reading in %s - %s' % (self.export_sql_file, e), exc_info=True)
raise RuntimeError

You're not re-raising a RuntimeError. You're swallowing all exceptions and then raising a RuntimeError. Re-raise the original exception in
export_to_csv() and then handle it higher up. As Steven suggested, it is a good idea to handle exceptions in as few places as possible (and
as specifically as possible). Also, loggers have an exception method, which can be very helpful in debugging when unexpected things happen,
especially when you need to catch a wide range of exceptions.

 

[Victor Hooi]

Hi,

Thanks to @Stephen D'APrano and @Andrew Berg for your advice.

The advice seems to be that I should move my exception higher up, and try to handle it all in one place:

for job in jobs:
try:
try:
job.run_all()
except Exception as err: # catch *everything*
logger.error(err)
raise
except (SpamError, EggsError, CheeseError):
# We expect these exceptions, and ignore them.
# Everything else is a bug.
pass

That makes sense, but I'm sorry but I'm still a bit confused.

Essentially, my requirements are:

1. If any job raises an exception, end that particular job, and continue with the next job.
2. Be able to differentiate between different exceptions in different stages of the job. For example, if I get a IOError in self.export_to_csv() versus one in self.gzip_csv_file(), I want to be able to handle them differently. Often this may just result in logging a slightly different friendly error message to the logfile.

Am I still able to handle 2. if I handle all exceptions in the "for job in jobs" loop? How will I be able to distinguish between the same types of Exceptions being raise by different methods?

Also, @Andrew Berg - you mentioned I'm just swallowing the original exception and re-raising a new RuntimeError - I'm guessing this is a bad practice,right? If I use just "raise"

except Exception as err: # catch *everything*
logger.error(err)
raise

that will just re-raise the original exception right?

Cheers,
Victor

 

[Andrew Berg]

Also, @Andrew Berg - you mentioned I'm just swallowing the original exception and re-raising a new RuntimeError - I'm guessing this is a bad practice, right? If I use just "raise"

except Exception as err: # catch *everything*
logger.error(err)
raise

that will just re-raise the original exception right?

Yes. However, if you are doing logging higher up where you actually handle the exception, then logging here is redundant, and you can simply
eliminate the try/catch block completely.

 

[Steven D'Aprano]

Well, maybe, maybe not. It depends on how much of a black-box you want
the method or function to be, and whether or not the exception you raise
gives the caller enough information to fix the problem.

For instance, consider this:

# Pseudocode
def read(uri):
try:
scheme, address = uri.split(":", 1)
if scheme == "file":
obj = open(address)
elif scheme == "http":
obj = httplib.open(address)
elif scheme == "ftp":
obj = ftplib.open(address)
return obj.read()
except Exception:
raise URIReadError(some useful message here)

since it swallows too much: anything from file not found, permissions
errors, low-level network errors, high-level HTTP errors, memory errors,
type errors, *everything* gets swallowed and turned into a single
exception.

But it isn't that the principle is wrong, just that the execution is too
greedy. The designer of this function needs to think hard about which
exceptions should be swallowed, and which let through unchanged. It may
be, that after thinking it through, the designer decides to stick with
the "black box" approach. Or perhaps she'll decide to make the function a
white box, and not catch any exceptions at all. There's no right answer,
it partly depends on how you intend to use this function, and partly on
personal taste.

My personal taste would be to let TypeError, OS-, IO- and networking
errors through unchanged, and capture other errors (like malformed URIs)
and raise a custom exception for those.

Yes. However, if you are doing logging higher up where you actually
handle the exception, then logging here is redundant, and you can simply
eliminate the try/catch block completely.

Yes, this! Try to avoid having too many methods responsible for logging.
In an ideal world, each job should be the responsibility for one piece of
code. Sometimes you have to compromise on that ideal, but you should
always aim for it.

 

[Peter Cacioppi]

I'm surprised no-one is mentioning what seems the obvious pattern here - exception wrapping.

So you define your exception class as containing the root exception.

Then your specific job routines wrap the exceptions they encounter in your personal exception class. This is where you go add in specific information re: the circumstances under which the exception occurred.

Then you throw your exception, which is captured at the appropriate level, and logged appropriately.

The nice thing here is that you to tend to write all the exception logging in one place, instead of scattered all around.

Your code looks very close to this pattern. Just raise an exception that can wrap the low level exception.