fix: close gaps in pkg_resources/sqlalchemy-redshift warning suppression#41935
fix: close gaps in pkg_resources/sqlalchemy-redshift warning suppression#41935eschutho wants to merge 3 commits into
Conversation
The pkg_resources UserWarning from sqlalchemy-redshift's __init__.py (apache#36082) was already suppressed in db_engine_specs/redshift.py, but production logs still show ~76/day of it firing: - mcp_service runs as a separate process and never imports db_engine_specs/redshift.py, so it had no filter for this warning at all (unlike its sibling authlib.jose filter, which is present in both __init__.py and server.py). Added it in both places, mirroring the existing authlib.jose pattern exactly. - The main web/celery filter only registers the first time something accesses Database.db_engine_spec, which isn't guaranteed to be the first thing to run in every process. Registered the same filter unconditionally in DefaultLoggingConfigurator.configure_logging(), which runs first in SupersetAppInitializer.init_app(), as a more robust primary suppression point. The existing redshift.py filter is left in place as a fallback for standalone scripts.
dosubot
Bot
added
data:connect:redshift
Code Review Agent Run #d1c7d3Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41935 +/- ##
==========================================
- Coverage 64.77% 64.47% -0.31%
==========================================
Files 2739 2780 +41
Lines 152981 156731 +3750
Branches 35058 35561 +503
==========================================
+ Hits 99101 101057 +1956
- Misses 51980 53706 +1726
- Partials 1900 1968 +68
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Functional — worth resolving before merge
superset/utils/logging_configurator.py:65: could we move this filter toSupersetAppInitializer.configure_logging()before it delegates toLOGGING_CONFIGURATOR, or another common startup hook?LOGGING_CONFIGURATORis replaceable, so deployments with a custom configurator skipDefaultLoggingConfigurator.configure_logging()and still depend on the original order-sensitive Redshift import filter. That leaves the web/Celery gap open for those deployments.
Other suggestion
- Could we extend the existing
test_suppress_third_party_warningsto assert the newpkg_resourcesfilter, plus cover the final app/worker startup location? Right now that test still passes if the new MCP filter is removed.
The focused implementation and dependency rationale look good otherwise. Reviewed at 16db953; CI is green.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| pkg_resources_filters = [ | ||
| f | ||
| for f in warnings.filters | ||
| if f[0] == "ignore" | ||
| and isinstance(f[1], re.Pattern) | ||
| and f[1].pattern == r"pkg_resources is deprecated as an API" | ||
| ] | ||
| assert len(pkg_resources_filters) >= 1, "Expected pkg_resources warning filter" |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to the newly introduced warning-filter collection variable. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The added local variable pkg_resources_filters is a clearly typable Python variable introduced in a new hunk and it has no annotation. This matches the rule requiring type hints for relevant variables that can be annotated.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_server.py
**Line:** 150:157
**Comment:**
*Custom Rule: Add an explicit type annotation to the newly introduced warning-filter collection variable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The variable To resolve this, you can update the variable declaration in pkg_resources_filters: list[Any] = [
f
for f in warnings.filters
if f[0] == "ignore"
and isinstance(f[1], re.Pattern)
and f[1].pattern == r"pkg_resources is deprecated as an API"
]I have checked the other comments on this PR, and there are no other pending review comments to address. Would you like me to perform any other tasks? tests/unit_tests/mcp_service/test_mcp_server.py |
Code Review Agent Run #0829e6Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for tracking down the gap, this looks like the right fix and the reasoning on the three suppression points is solid. |
Aligns with CI's pinned ruff version; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Functional — worth resolving before merge
- The new unconditional filters match only the warning message, so they suppress the same
pkg_resourceswarning from every extension or dependency, not justsqlalchemy-redshift. Since setuptools emits it as aUserWarningwithstacklevel=2, could we scope each filter withcategory=UserWarningandmodule=r"sqlalchemy_redshift(?:\\..*)?"? That preserves unrelated dependency-migration signals.
Other suggestions
- Could we add a regression test proving
SupersetAppInitializer.configure_logging()installs the filter before a customLOGGING_CONFIGURATORruns? The current MCP test already sees the package-level filter during import, so it does not independently exercise_suppress_third_party_warnings(). - The additional
server.pyregistration appears redundant becausesuperset.mcp_service.__init__necessarily runs first and the filter persists. Could we remove it unless there is a real reset scenario to cover? - Could we make the changed comments timeless and refresh the PR description? It still describes
logging_configurator.pyas the final owner and says no test assertion was added.
Reviewed at 3fd3db5; current CI is green.
Code Review Agent Run #db17ceActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
What warning
Datadog showed the third-party
UserWarning: pkg_resources is deprecated as an API(raised fromsqlalchemy-redshift's own__init__.py, which still doesfrom pkg_resources import ...) firing at high volume (~76/day) in production logs, split across the main web/celery process (py3.10) andsuperset/mcp_service(a separate py3.11 process).There's already an attempted suppression for this in
superset/db_engine_specs/redshift.py(warnings.filterwarnings("ignore", message=r"pkg_resources is deprecated as an API")), with a comment reasoning thatDatabase.db_engine_specis always accessed (which imports this module) beforecreate_engine()ever triggers SQLAlchemy's lazy dialect import ofsqlalchemy_redshift. Despite that, the warning is still firing in production, so the existing fix has a gap somewhere in the ordering.What changed
superset/mcp_service/__init__.py+superset/mcp_service/server.py: added the same filter, mirroring the exact existing pattern already used for theauthlib.josedeprecation warning in these same two files (mcp_service is a separate process/import graph from the main app and had zero suppression for this specific warning).superset/utils/logging_configurator.py: registered the same filter unconditionally inDefaultLoggingConfigurator.configure_logging(), which runs first thing inSupersetAppInitializer.init_app()(both the web app and celery workers go throughcreate_app()→init_app()). This is a more robust, order-independent suppression point that doesn't depend onDatabase.db_engine_spechaving been accessed first.superset/db_engine_specs/redshift.py: left the original filter in place (harmless, idempotent) and updated its comment to point at the new primary suppression location.No behavior change
Purely additive
warnings.filterwarnings("ignore", ...)calls — no change to any request/response behavior, just suppressing log noise from a warning that carries no actionable signal (the underlyingsqlalchemy-redshiftpin can't move past<0.9to thepkg_resources-free1.0.0release without a SQLAlchemy 2.0 migration, per the existing comment referencing #39750).Test plan
python3 -m py_compileon all 4 changed files — clean.ruff check— clean.pkg_resources/setuptools 80.x package.superset/tasks/celery_app.py) callscreate_app(), so it goes through the sameinit_app()→configure_logging()path as the web process.sqlalchemy_redshift/create_engine(touchpoints — no additional code paths need their own filter.authlib.josefilter it mirrors).