runLog.error should raise an error

[john-science]

There are currently 117 places in ARMI where runLog.error() is called, and it logs a message at the error level. For instance:

armi/armi/settings/setting.py

Line 190 in 0e830d2

runLog.error(f"Error in setting {self.name}, val: {val}.")

armi/armi/reactor/assemblyParameters.py

Line 139 in 0e830d2

runLog.error("Values stored in the `notes` parameter must be strings of less than 1000 characters!")

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.

[john-science]

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: #2056

[partofthething]

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.
-- #304 (comment)

[drewj-tp]

#2056

[john-science]

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:

armi/armi/runLog.py

Line 481 in 0e830d2

class RunLogger(logging.Logger):

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?

[john-science]

@sombrereau @keckler Would this change hurt you guys?

[john-science]

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:

armi/armi/runLog.py

Lines 621 to 622 in 95d94a4

	def warningReport(self):
	"""Summarize all warnings for the run."""

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().

[opotowsky]

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.

[opotowsky]

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

[keckler]

@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.