Fix import failure when warnings.showwarning is overridden (e.g. under pytest-cov)#20019
Open
Mohit-Ak wants to merge 1 commit into
Open
Fix import failure when warnings.showwarning is overridden (e.g. under pytest-cov)#20019Mohit-Ak wants to merge 1 commit into
Mohit-Ak wants to merge 1 commit into
Conversation
astropy.logger._set_defaults() runs at import time via _init_log(). It unconditionally called disable_warnings_logging()/disable_exception_logging() to reset previously installed hooks, but those methods raise LoggingError when warnings.showwarning / sys.excepthook has been replaced by another library after Astropy installed its own (e.g. the pytest-cov coverage plugin). That made 'import astropy' fail outright under such tools. Make the import-time reset best-effort: catch LoggingError from the disable calls and relinquish Astropy's own hook bookkeeping while leaving the third-party hook untouched. The public disable_*_logging() API contract is unchanged (they still raise for direct callers). Adds regression tests for both the warnings and exception hook paths. Fixes astropy#20005
Contributor
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #20005.
import astropycan fail outright with aLoggingErrorwhen another library hasreplaced
warnings.showwarning(orsys.excepthook) after Astropy installed itsown handler. This bites
pytest-covusers in particular — the coverage plugininstalls its own
showwarning, and then any package that imports Astropy atcollection time (the issue came in from astroquery.mast) crashes on import:
Root cause.
_init_log()runs at import time and callsAstropyLogger._set_defaults(), which resets any previously installed hooks bycalling
disable_warnings_logging()/disable_exception_logging(). Thosemethods deliberately raise
LoggingErrorwhenwarnings.showwarning/sys.excepthookno longer points at Astropy's handler — a sensible contract for adirect caller, but fatal here, because
_set_defaults()is a best-effort resetthat runs unconditionally during
import astropy.Fix. Make the import-time reset tolerant:
_set_defaults()now catches theLoggingErrorfrom the two disable calls, leaves the third-party hook in place(it never clobbers someone else's
showwarning/excepthook), and just clearsAstropy's own bookkeeping so the logger returns to a clean state. The public
disable_warnings_logging()/disable_exception_logging()API is unchanged —they still raise for direct callers, so the existing contract tests
(
test_warnings_logging_overridden,test_exception_logging_overridden) stillpass. Because
conf.log_warningsdefaults toTrue,_set_defaults()thenre-installs Astropy's own handler on top and records the third party's hook as the
recoverable original, so warning capture keeps working and the other library's
hook is restored if Astropy is later disabled.
I fixed both the
showwarningpath (the reported one) and the siblingsys.excepthookpath in the same method, since they make the identicalassumption.
How was this tested?
Reproduced the exact failure first, then confirmed the fix, on a source build of
main:I also confirmed that with
warnings.showwarningoverridden before the import,import astropynow succeeds instead of raising.Two regression tests were added to
astropy/tests/test_logger.py(
test_set_defaults_tolerates_overridden_showwarningandtest_set_defaults_tolerates_overridden_excepthook).Full logger suite is green and lint is clean at the repo-pinned tool version:
A changelog fragment is included at
docs/changes/20005.bugfix.rst(named afterthe issue; happy to rename it to the PR number if preferred).