DM-13155: Switch from log.getName() to log.name #196
Conversation
python/lsst/ip/diffim/psfMatch.py
Outdated
| @@ -1033,11 +1033,11 @@ def _solve(self, kernelCellSet, basisList, returnOnExcept=False): | |||
| except Exception as e: | |||
| self.log.error("ERROR: Unable to calculate psf matching kernel") | |||
|
|
|||
| log.log("TRACE1." + self.log.getName() + "._solve", log.DEBUG, str(e)) | |||
| log.log("TRACE1." + self.log.name + "._solve", log.DEBUG, str(e)) | |||
There was a problem hiding this comment.
That str at the end looks suspect to me. Pre-existing of course but I think it probably wants to instead be a simple message as for the previous log entry with exc_info=True at the end to include the exception information in the standard way
There was a problem hiding this comment.
Perhaps I dove in too deep on this ticket. I was just hoping to get rid of a gazillion deprecation warnings...I really am totally ignorant when it comes to the details and best practices of the various ways of logging things, so I have no knowledge of the exc_info parameter (it doesn't come up in a search of the Dev Guide...), nor the "standard way" of using it. I did a search of the stack for it and found very few examples, and they include cases where both are used in the same file and I'm not totally sure where/why one would be preferred, e.g.
https://github.com/lsst/ctrl_mpexec/blob/master/python/lsst/ctrl/mpexec/singleQuantumExecutor.py#L427-L435
If you are able to fill me in on exactly how this should look in this case, I'm happy to change it here, but I don't want to make any guesses as potentially changing behavior was not meant to be an intention of this simple ticket!
There was a problem hiding this comment.
The identical reporting should be something like:
log.log("TRACE1." + self.log.name + "._solve", log.DEBUG, "%s", e)since that will defer the stringification until it's needed. Given that this only triggers in an exception block it's not overly important to change it and what you have now is fine.
The tweak would be:
log.log("TRACE1." + self.log.name + "._solve", log.DEBUG, "Unable to calculate psf matching kernel", exc_info=True)and what that will do is include the stack trace in the log itself if this logger is enabled. That probably seems more inline with what a TRACE logger would be used for (rather than just seeing the stringified exception -- otherwise the trace would be in some other place outside the logging system).
There was a problem hiding this comment.
PS Your changes can be merged here as is. My tweak is a slight improvement unrelated to your original goal so feel free to ignore.
There was a problem hiding this comment.
Ok, I went with the "identical reporting" option. Thanks again for all your detailed explanations!
laurenam
force-pushed
the
tickets/DM-31355
branch
from
a54de51
to
f6bd78f
Compare
The former is now deprecated, so replace all instances.
laurenam
force-pushed
the
tickets/DM-31355
branch
from
f6bd78f
to
864b2c1
Compare
No description provided.