-
Notifications
You must be signed in to change notification settings - Fork 124
Logging refactor #2012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Logging refactor #2012
Conversation
|
Good move to integrate with standard library logger. One cool feature is the use of the |
rcooke-ast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this. It's great that you've brought this part of the code into the present times!
I only reviewed __init__.py, test_logger.py and logger.py.
I couldn't checkout a test drive of the code, just to see what it looks like as terminal printout. Could you please post a representative screenshot? Thanks!
tbowers7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great advancement for PypeIt. There were several small things I noticed, but nothing major. Merging other PRs into this one will be challenging and will require patience. Also, there was a blanket addition of log and PypeItError to files that previously had msgs, but both were not needed all of the time. May be worth a fine-tooth comb to remove spurious imports, by maybe not?
The only request I have is to re-include the Ctrl+C catcher and provide a graceful shutdown. With the use of atexit.register(), it should be possible to close out QA's and logging gracefully before sys.exit()-ing.
| def _excepthook(self, etype, value, trace): | ||
| """ | ||
| Override the default exception hook to log an error message. | ||
| """ | ||
| tb = trace | ||
| if tb is None: | ||
| exc_info = None | ||
| else: | ||
| # If the traceback is available, jump to the calling frame, which | ||
| # gets passed to makeRecord | ||
| while tb.tb_next: | ||
| tb = tb.tb_next | ||
| exc_info = (etype, value, tb) | ||
|
|
||
| # Add the error type to the message. | ||
| if len(value.args) > 0: | ||
| message = f"{etype.__name__}: {str(value)}" | ||
| else: | ||
| message = str(etype.__name__) | ||
|
|
||
| # Log the error | ||
| self.error(message, exc_info=exc_info) | ||
|
|
||
| # Call the original exception hook | ||
| self._excepthook_orig(etype, value, trace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this function mean that we log errors but still allow them to crash the code?
Is there a more graceful way to print useful traceback without a full-on crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the purpose of this function is basically to add the log entry before executing the same exception code that would have been executed without the bespoke logger.
Similar to a comment above, I'm not sure what you mean by exiting more gracefully, but let's discuss.
kbwestfall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed review, @tbowers7 ! I'd like to talk through how we exit because I don't understand the details well enough. Let's talk at the next development telecon about that and/or over Slack.
| ## Imports for signal and log handling | ||
| #import sys | ||
| #import signal | ||
| ## Send all signals to messages to be dealt with (i.e. someone hits ctrl+c) | ||
| #def signal_handler(signalnum, handler): | ||
| # """ | ||
| # Handle signals sent by the keyboard during code execution | ||
| # """ | ||
| # if signalnum == 2: | ||
| # log.info('Ctrl+C was pressed. Ending processes...') | ||
| # sys.exit() | ||
| # | ||
| #signal.signal(signal.SIGINT, signal_handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never used the atexit package, but it seems pretty useful. I also don't have a good sense of why we should prefer to exit gracefully (I'm not sure what that would look like) when someone intentionally kills the program using Ctrl+C.
Let's discuss at the next telecon and/or over Slack.
| def _excepthook(self, etype, value, trace): | ||
| """ | ||
| Override the default exception hook to log an error message. | ||
| """ | ||
| tb = trace | ||
| if tb is None: | ||
| exc_info = None | ||
| else: | ||
| # If the traceback is available, jump to the calling frame, which | ||
| # gets passed to makeRecord | ||
| while tb.tb_next: | ||
| tb = tb.tb_next | ||
| exc_info = (etype, value, tb) | ||
|
|
||
| # Add the error type to the message. | ||
| if len(value.args) > 0: | ||
| message = f"{etype.__name__}: {str(value)}" | ||
| else: | ||
| message = str(etype.__name__) | ||
|
|
||
| # Log the error | ||
| self.error(message, exc_info=exc_info) | ||
|
|
||
| # Call the original exception hook | ||
| self._excepthook_orig(etype, value, trace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the purpose of this function is basically to add the log entry before executing the same exception code that would have been executed without the bespoke logger.
Similar to a comment above, I'm not sure what you mean by exiting more gracefully, but let's discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors PypeIt's logging infrastructure to use Python's standard logging library instead of the custom pypmsgs module. The new implementation provides standard logging levels (DEBUG, INFO, WARNING, ERROR) with improved flexibility, including separate verbosity control for console and file outputs.
Key changes:
- Replace
pypmsgs.pywithlogger.pyusing standard Python logging - Add new
exceptions.pymodule to centralize exception definitions - Update all scripts to support verbosity and log file options by default
Reviewed changes
Copilot reviewed 300 out of 515 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pypeit/init.py | Replaces pypmsgs import with new logger module and exception imports |
| pypeit/cache.py | Updates logging calls from msgs.warn/error to log.warning and raise PypeItError |
| pypeit/archive.py | Updates logging calls from msgs.info/error to log.info and raise PypeItError |
| pypeit/alignframe.py | Updates logging calls, replaces msgs.work with log.debug and msgs.error with raise PypeItError |
| doc/pypeit_par.rst | Updates parameter documentation with new default values and parameters |
| doc/conf.py | Adds sphinx configuration for type hints and additional intersphinx mappings |
| doc/api/* | Updates API documentation structure with new logger module and exceptions module |
| doc/help/* | Updates script help documentation with new verbosity and logging options |
| doc/include/* | Updates datamodel documentation versions and dependency tables |
Comments suppressed due to low confidence (1)
pypeit/cache.py:1
- Multi-line error messages are missing line breaks between concatenated f-strings. Add
\ncharacters to ensure proper formatting when displayed.
# -*- coding: utf-8 -*-
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e can be refactored.
Remove cancel button from setup gui progress dialog
|
Dev-suite tests started. |
This refactors the logging infrastructure for PypeIt so that it now uses the standard python
logginglibrary (I don't know why I torture myself...). The new logger draws heavily from the ones implemented byastropyandsdsstools.The python logging package gives the following levels, DEBUG, INFO, WARNING, ERROR, and CRITICAL. In their parlance, we basically consider all ERRORs as CRITICAL, so the
log.criticalfunction isn't really useful to us. For the scripts, I've mapped theverbosityto these levels as DEBUG = 2, INFO = 1, and WARNING = 0. I.e., ifverbosity == 0, we only log warnings and errors, whereasverbosity == 1adds info message, andverbosity == 2adds debug messages. All the scripts now also allow us to set the level specifically for the file log (using the same integer mapping as the console verbosity), which can be different from the console log.The key changes are:
pypmsgs.pyis effectively replaced bylogger.py. There is some functionality loss (I think this is primarily limited to the fact that we will no longer catch Ctrl-c events and the QA html will not be automatically constructed when failures happen), so let me know if you think we're losing something criticalfrom pypeit import msgs, basically every file will need tofrom pypeit import log, PypeItError.msgs.error(...), you should insteadraise PypeItError(...). All exceptions (and warnings) are now caught and processed by thePypeItLogger(not justPypeItErrorinstances); so if you think its more appropriate to raise a different error type, go for it. The logging class haserrorandcriticalfunctions, but they're not that useful for pypeit.warnfunction in theloggingpackage has been deprecated, so all warnings should uselog.warninginstead ofmsgs.warn.debugmessage level. Anything that used to usemsgs.work, I've now converted tolog.debug, but there are some conceptual differences there.msgs.newline()function no longer exists. If you want a new line in the log message, include the\ncharacter directly. I was thinking of adding some line-wrapping tools for messages in the future, but formatted output that suits everyone's terminal width choices is tricky.DEBUG, which displays all messages and adds the function file, name, line number information.INFOlevel does not include the calling function informationpypeit/logger.py,pypeit/__init__.py,pypeit/tests/test_log.py.I'm submitting this as a draft PR, but I think it's mostly done. But, because this PR basically touches everything, we should discuss when it gets merged. I expect it should at least come after #2007 and #1971 , and I will deal with the likely conflicts. So if you have another big PR in the works and don't want to deal with the conflicts, let's try to get those in before this one.