Proposal: runLog.exception for logging an error message and raising an exception

[drewj-tp]

We have some patterns internally and downstream that look like

if somethingHasCatastophicallyFailed:
    msg = "Something has catastrophically failed!"
    runLog.error(msg)
    raise RuntimeError(msg)

but with different exceptions, more useful message, etc. etc. But we like to have a runLog.error with a message so it goes in stdout, and then the exception is thrown.

There are some cases where someone may want to log an error but not trash the run. Maybe not at that spot, maybe not at all. But that gets more into a philosphy discussion: does an error always mean you want to crash out?

I propose instead to skip that and just add something like runLog.exception(msg, excType=RuntimeError) that would

1. Log the error message via runLog.error(msg), and then
2. Raise the provided exception with the msg like raise excType(msg). Probably always want this to be a RuntimeError but maybe you want to provide some more context in the exception.

Alternatively, if you caught some error and want to provide some more context, you may have a pattern like

try:
    doThing()
except Exception as ee:
    msg = "dothing failed!"
    runLog.error(msg)
    raise RuntimeError(msg) from ee

then you still have the traceback from the thing that caused the real error, but with some higher-level context.

This might look like runLog.exception(msg, RuntimeError, ee) where you pass in the caught exception like

try:
    doThing()
except Exception as ee:
    runLog.exception("doThing failed!", RuntimeError, ee)

Things to consider

The downside of putting the literal raise command one level away is the traceback is going to terminate in the runLog.exception call. Not the actual location you'd want to point a developer or user at. That's not ideal but I think there are some stack manipulations we could do to avoid that

[john-science]

I think raising an error when runLog.error() is called is a fine idea.

And it would be laughably easy for me to do from the ARMI side.

The QUESTION I have is: what percentage of downstream applications would immediately fail when I made this change?

Now, for internal TP code, we could run such a test and find out. For external projects, it would be far less direct.

But for me this change is more about taking time to do the due diligence to make sure there aren't 1,000 workflows that will immediately fail when making this change. That's all.

[albeanth]

Forcing an exception with runLog.error() is logical. The fact that it doesn't surprises people, I think.

It's been awhile, but I have seen some downstream TP cases hit these non-fatal errors, only to crash out sometime later in a different interface. If a dev/user doesn't know about non-fatal errors, they'll be debugging in the wrong spot. Even worse, analysts may never even know that their run is hitting some non-fatal "error" even if their run finishes.

I strongly support forcing runLog.error() to error out as @drewj-tp points out.

Even if it break workflows, I think it's probably for the best.

[keckler]

Yep, it surprised me for sure

[john-science]

Okay, if it surprised the three of you, I am convinced it will be generally surprising.

The strategy here would be straight forward:

• Make a one-line ARMI PR
• Fix the ARMI unit tests
• Run all the downstream projects we can find, and see what fails
• Go through all the failed downstream codes and either:
  • Refactor their code to catch the error and handle it
  • Change their runLog.error() to runLog.warning() if it shouldn't kill the code.

Any of us could do that work. I can't tell if it would mean 1 downstream PR or 12, but that's where all the time would go on the task.