[BUGFIX] Stop silencing warnings for users with no logging configured - #12070
Open
aleks-drozy wants to merge 1 commit into
Open
[BUGFIX] Stop silencing warnings for users with no logging configured#12070aleks-drozy wants to merge 1 commit into
aleks-drozy wants to merge 1 commit into
Conversation
Five modules (validator.py, validation_graph.py, metrics_calculator.py, batch_manager.py, expect_column_kl_divergence_to_be_less_than.py) called logging.captureWarnings(True) at import time. This is process-global: it redirects warnings.showwarning into the logging module for the entire interpreter. Since CPython's captureWarnings machinery attaches a NullHandler to the "py.warnings" logger on first use when no handler is configured, any user importing great_expectations with no logging setup had warnings.warn(...) silently swallowed everywhere in their process, not just within great_expectations code. Remove the five captureWarnings(True) calls; the module-level `logger = logging.getLogger(__name__)` lines are untouched since they're used independently. Applications that want warnings routed through logging can still opt in themselves via logging.captureWarnings(True). Fixes fivetran#12067
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
Contributor
|
Thank you for your contribution! Before we can merge this pull request, every committer needs to have signed our Contributor License Agreement (CLA). We could not find a signed CLA for: @aleks-drozy. Please sign the Individual Contributor License Agreement, or the Software Grant and Corporate Contributor License Agreement if you are contributing on behalf of your employer (see CLA.md for details). Once resolved, comment |
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.
Root cause
Five modules call
logging.captureWarnings(True)unconditionally at import time (module top-level):great_expectations/validator/validator.pygreat_expectations/validator/validation_graph.pygreat_expectations/validator/metrics_calculator.pygreat_expectations/core/batch_manager.pygreat_expectations/expectations/core/expect_column_kl_divergence_to_be_less_than.pylogging.captureWarnings(True)is process-global — it monkeypatcheswarnings.showwarningfor the entire interpreter. Becauseimport great_expectationstransitively imports all five modules, simply importing the library flips this behavior for the whole process, including code that never touchesgreat_expectations.The silencing itself comes from a well-known CPython
logginggotcha, not anythinggreat_expectationscodes explicitly. CPython's replacementshowwarningdoes roughly:So the first warning emitted after
captureWarnings(True)causes Python to attach aNullHandlerto thepy.warningslogger if nothing else is configured. That satisfiesLogger.callHandlers's "was a handler found anywhere in the chain?" check, so the record is silently consumed andlogging.lastResort(which would otherwise print unhandled WARNING+ records to stderr) never fires. Users who already configure logging themselves aren't affected, since their own handler intercepts the record — only users with zero logging configuration lose their warnings.Fix
Removed the five module-level
logging.captureWarnings(True)calls. Thelogger = logging.getLogger(__name__)lines directly above each were left intact since they're independently used elsewhere in those modules. No replacement logic is needed — removing the calls restores Python's defaultwarningsdisplay behavior. Applications that want warnings routed throughloggingcan still opt in themselves by callinglogging.captureWarnings(True).Testing
Added
tests/test_warnings_not_silenced.py, which spawns a subprocess runningimport great_expectations; warnings.warn(...)and asserts the warning appears on stderr. A subprocess is required (rather thanpytest.warns/catch_warnings) because pytest's own warning-capture fixtures install their ownshowwarninghook and would mask the regression — this mirrors the reproduction steps from the issue.develop(stderr is empty) and passes after the fix (stderr contains the warning).tests/validator/test_validation_graph.py,tests/validator/test_metrics_calculator.py,tests/validator/test_validator.py,tests/validator/test_v1_validator.py, and the KL-divergence expectation integration tests (tests/integration/data_sources_and_expectations/expectations/test_expect_column_kl_divergence_to_be_less_than.py, pandas backend) — all passing.ruff checkclean on all changed files.Fixes #12067