Repository navigation
Conversation
llucax
requested review from
florian-wagner-frequenz
and removed request for
a team
September 18, 2026 12:15
github-actions
Bot
added
part:docs
CPython deduplicates warnings (the "default", "module" and "once" actions) through per-module `__warningregistry__` dictionaries, and invalidates all of them whenever the filters change, including on every enter and exit of a `warnings.catch_warnings` block, even when the filters are restored to exactly what they were. So the documented way of suppressing a warning locally (`with catch_warnings(): simplefilter(...)`) makes every warning in the program repeat on every call, including warnings emitted by unrelated callers. This is python/cpython#73858, open since 2017, and it affects the C implementation on 3.11 to 3.15, with and without `-X context_aware_warnings`. The new `frequenz.core.warnings.catch_warnings` is a drop-in replacement that snapshots the registries of all loaded modules on entry and brings them back to a valid state on exit. The catch is that a registry is only valid when stamped with the current filters version, an internal counter that is not exposed, so it is read by emitting a warning of a private category into a private registry, from inside the block, where the "ignore" filter the probe needs is discarded on exit. The enter and exit bumps are then arithmetic, and are verified once at import time: if the interpreter doesn't bump the counter as expected the class degrades to a plain `warnings.catch_warnings`. Correctness takes precedence over deduplication: a history that was already invalid on entry (a filter changed outside any block) stays invalid, history recorded inside the block is only kept when the block added nothing but "ignore" filters through the `action` argument (an "ignore" can only remove warnings, so what was shown is still valid outside), and nothing is restored if the filters after the block differ from the ones before it, which can happen with context-aware warnings when another thread changes them. Nested blocks report their bumps to the enclosing one so they compose. Every race considered degrades to the stdlib behaviour of repeating a warning, never to suppressing one. The workaround suggested in the CPython issue (saving and restoring the current module's own registry and reading the version through a leaked "ignore" filter) was not adopted: it only covers the module defining the block, and it leaves a filter behind on every exit. The cost is a scan of `sys.modules` on entry and exit (tens of microseconds with a couple of hundred modules), documented in the class. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Field testing with the dispatch client showed the registry repair costing ~130 µs per block with ~370 modules loaded, which turned a 2.8 s test suite into a 12 s one: the converters silencing a deprecation are called per message, so scanning `sys.modules` on every call is not acceptable there, and it is the whole reason this class exists. The registries only go stale because the filters version moves, and it only moves through `simplefilter()`, `filterwarnings()` and `catch_warnings`; the lookup itself reads the live filters list on every warning (`warnings.filters`, or the context's `_filters` with context-aware warnings, both through `_get_filters()` on 3.14+). So when the block only adds an `"ignore"` filter and doesn't record, insert the filter tuple into that list directly and take it out again by identity on exit, restoring `showwarning` as the standard library does. Nothing ever becomes stale, so there is nothing to scan or repair, and the block costs about twice the standard one instead of fifty times. This is only sound for `"ignore"`: the registry is consulted before the filters, and an ignore can only remove warnings, so the history recorded on either side of the block is valid on the other. If the filters were changed by hand inside such a block they are restored on exit and the version bumped, like the standard library would (`simplefilter()` even replaces our equal entry with its own, so the comparison is against a copy taken on entry, not by looking for our tuple). Every other use keeps going through the standard library plus the registry repair, and the cost note says which is which. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
llucax
force-pushed
the
fix-warning-registry
branch
from
September 18, 2026 12:38
05e9910 to
27adcf7
Compare
llucax
enabled auto-merge
September 18, 2026 12:38
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Warning-state invalidation and fast-path isolation issues can still repeat or incorrectly suppress warnings.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a CPython-aware catch_warnings replacement that preserves warning deduplication history while safely falling back on unsupported interpreters.
Changes:
- Adds registry preservation and an optimized ignore path.
- Adds extensive behavior, nesting, concurrency, and fallback tests.
- Documents the feature and limitations.
File summaries
| File | Description |
|---|---|
| tests/test_warnings.py | Tests warning history, filtering, fallback, and concurrency. |
| src/frequenz/core/warnings.py | Implements the new context manager. |
| RELEASE_NOTES.md | Announces the new warnings utility. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+410
to
+412
| if self._own_filter is not None and self._own_filter[0] == "ignore": | ||
| filters = _live_filters(self._module) if not self._record else None | ||
| if filters is not None: |
Comment on lines
+77
to
+79
| assert sys.implementation.name == "cpython" | ||
| assert core_warnings._SUPPORTED # pylint: disable=protected-access | ||
| assert core_warnings._check_bump_counts() # pylint: disable=protected-access |
| return (filter_bump, enter_bump, exit_bump) == (1, 1, 1) | ||
|
|
||
|
|
||
| _SUPPORTED: Final[bool] = _check_bump_counts() |
Comment on lines
+420
to
+421
| _, category, lineno, append = self._own_filter | ||
| self._fast_item = ("ignore", None, category, None, lineno) |
Comment on lines
+506
to
+518
| filters = _live_filters(self._module) | ||
| if filters is not None: | ||
| # By identity: somebody may have inserted an equal filter meanwhile. | ||
| for index, existing in enumerate(filters): | ||
| if existing is item: | ||
| del filters[index] | ||
| break | ||
| if filters != self._fast_saved: | ||
| # The filters were changed by hand inside the block: restore them | ||
| # as the standard library would, bumping the version as it does, | ||
| # since the registries were filled under other filters. | ||
| filters[:] = self._fast_saved | ||
| _mutated(self._module) |
Contributor
Author
|
I've been iterating A LOT on this after these copilot comments, and it is bad. All warnings handling in Python is a minefield, not only open unresolved bugs, but also it is all done via globals, so handling breaks on concurrent programs (even for async programs) until Python 3.14, where they added an opt-in fix, so you don't even get it unless you run python with some extra options... I'm finishing some more purpose-specific alternatives, so closing for now. |
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.
CPython deduplicates warnings (the "default", "module" and "once" actions) through per-module
__warningregistry__dictionaries, and invalidates all of them whenever the filters change, including on every enter and exit of awarnings.catch_warningsblock, even when the filters are restored to exactly what they were.So the documented way of suppressing a warning locally (
with catch_warnings(): simplefilter(...)) makes every warning in the program repeat on every call, including warnings emitted by unrelated callers. This is python/cpython#73858, open since 2017, and it affects the C implementation on 3.11 to 3.15.The new
frequenz.core.warnings.catch_warningsis a drop-in replacement that snapshots the registries of all loaded modules on entry and brings them back to a valid state on exit.The catch is that a registry is only valid when stamped with the current filters version, an internal counter that is not exposed, so it is read by emitting a warning of a private category into a private registry, from inside the block, where the
"ignore"filter the probe needs is discarded on exit. The enter and exit bumps are then arithmetic, and are verified once at import time: if the interpreter doesn't bump the counter as expected the class degrades to a plainwarnings.catch_warnings.Correctness takes precedence over deduplication: a history that was already invalid on entry (a filter changed outside any block) stays invalid, history recorded inside the block is only kept when the block added nothing but
"ignore"filters through theactionargument (an"ignore"can only remove warnings, so what was shown is still valid outside), and nothing is restored if the filters after the block differ from the ones before it, which can happen with context-aware warnings when another thread changes them. Nested blocks report their bumps to the enclosing one so they compose. Every race considered degrades to the stdlib behaviour of repeating a warning, never to suppressing one.The workaround suggested in the CPython issue (saving and restoring the current module's own registry and reading the version through a leaked
"ignore"filter) was not adopted: it only covers the module defining the block, and it leaves a filter behind on every exit.The cost is a scan of
sys.moduleson entry and exit (tens of microseconds with a couple of hundred modules), with an optimization to avoid it specifically foraction="ignore", which should be the most common case (at least for deprecation scenarios).