terrapower
runLog.error should raise an error
There are currently 117 places in ARMI where runLog.error() is called, and it logs a message at the error level. For instance:
https://github.com/terrapower/armi/blob/0e830d2c8dece43c57b657caefaed8e8c7ba276b/armi/settings/setting.py#L190
https://github.com/terrapower/armi/blob/0e830d2c8dece43c57b657caefaed8e8c7ba276b/armi/reactor/assemblyParameters.py#L139
But what is the purpose of "logging an error?" If you don't raise Exception, then this is really just a warning message.
So, let's change
runLog.error()to raise the error after logging the message.
NOTE: The original discussion is here.
This ticket was supported by: @albeanth , @drewj-tp , @keckler, and @opotowsky .
Also, I noted this ticket is a variant of some of the ideas in this Discussion: https://github.com/terrapower/armi/discussions/2056
Good idea. Reminder also that in 2021 we imagined that runLog would be long gone by now if we had continued the desired migration to standard built-in python logging.
This way, we can refactor modules one at a time until runLog evaporates. -- https://github.com/terrapower/armi/issues/304#issuecomment-843553657
Reminder also that in 2021 we imagined that runLog would be long gone by now if we had continued the desired migration to standard built-in python logging.
The runLog tool is just a little custom wrapper around the Python standard library tools at the moment:
https://github.com/terrapower/armi/blob/0e830d2c8dece43c57b657caefaed8e8c7ba276b/armi/runLog.py#L481
If we wanted to remove runLog, we would (I am pretty sure) want to preserve the customization of the logging messages we do. I don't think (immediately) that would save us many lines of code. I could look into it.
It would be nice if someone (in some downstream code) could use the standard library (logging) commands directly (not using runLog) and their log statements would conform to what people have come to expect from ARMI. Is that an interesting goal to anyone?
A fun thing I just noticed is that at the end over every ARMI-based run we do a full report of all the warnings we encounter:
https://github.com/terrapower/armi/blob/95d94a4d58b175cd88e25a64670c1cdf1ea02cd4/armi/runLog.py#L621-L622
But we don't do the same report for ERRORS that we encounter.
That is so backwards it is kind of funny. BUT I think this ticket is a better solution than building a runLog.errorReport().
It would be nice if someone (in some downstream code) could use the standard library (logging) commands directly (not using runLog) and their log statements would conform to what people have come to expect from ARMI. Is that an interesting goal to anyone?
There was maybe a case of this, although we may have gotten it changed to ARMI runLog. Ping me offline and we can use that repo to test it out.
That is so backwards it is kind of funny. BUT I think this ticket is a better solution than building a
runLog.errorReport().
Yes I think the goal is to audit all the errors: change some to warnings and change some to raising an error. (I'd imagine most errors without raises are just warnings). No new reporting needed
@sombrereau @keckler Would this change hurt you guys?
If, when you say "this change", you are referring to raising an error upon runLog.error() calls, then no I do not have any qualms against doing this.
