Repository navigation
Conversation
…sses init_core_dependencies() calls initialize_core_mcp_dependencies() in every process that builds the app, including Celery workers that never serve MCP. Importing the MCP service app to register host tools has a real per-process memory cost that can OOM high-concurrency prefork worker pools on boot. Add a CORE_MCP_HOST_TOOLS_ENABLED config flag (default True, no behavior change) and gate the call on it, so deployments can turn host-tool registration off for processes that do not serve MCP. Signed-off-by: Elizabeth Thompson <elizabeth@preset.io> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #31e528Actionable Suggestions - 0Additional Suggestions - 2
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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if self.config.get("CORE_MCP_HOST_TOOLS_ENABLED", True): | ||
| initialize_core_mcp_dependencies() |
There was a problem hiding this comment.
Suggestion: With this flag false, the abstract tool and prompt decorators remain active, so extensions using them raise NotImplementedError and are skipped during extension loading. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/initialization/__init__.py
**Line:** 565:566
**Comment:**
*Api Mismatch: With this flag false, the abstract `tool` and `prompt` decorators remain active, so extensions using them raise `NotImplementedError` and are skipped during extension loading.
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
There was a problem hiding this comment.
Agreed—the disabled path leaves the Core decorators as raising stubs, so an extension entrypoint using @tool or @prompt can fail before its non-MCP contributions, including Celery tasks, are registered. Should decorator replacement be kept independent of host-tool registration?
There was a problem hiding this comment.
Yes. Decorator replacement should be independent of host-tool registration.
CORE_MCP_HOST_TOOLS_ENABLED=False should prevent importing/registering the MCP host-tool stack, but it must still install the functional @tool and @prompt decorators before extensions load. Otherwise an extension that declares MCP metadata can raise NotImplementedError and prevent unrelated contributions, such as Celery tasks, from being registered.
A minimal structure would be:
initialize_core_mcp_decorators()
if self.config.get("CORE_MCP_HOST_TOOLS_ENABLED", True):
initialize_core_mcp_host_tools()This likely requires splitting initialize_core_mcp_dependencies() into a lightweight decorator-initialization step and a host-tool registration step. The decorator path should avoid importing the MCP service application, preserving the worker memory benefit while maintaining extension-loading compatibility.
There was a problem hiding this comment.
Fixed in f2009f2 — thanks for catching this, and confirming @aminghadersohi's open question from the review.
You're right: the abstract tool/prompt in superset_core.mcp.decorators do raise NotImplementedError (verified against the source — both bodies are literally raise NotImplementedError("MCP ... decorator not initialized. ...")). So with the previous all-or-nothing gate, a process with CORE_MCP_HOST_TOOLS_ENABLED=False that loaded an extension applying @tool/@prompt at import time would have crashed at import.
initialize_core_mcp_dependencies() is now split into two independent steps:
initialize_core_mcp_decorators()— swaps the abstract decorators for the concrete ones. Runs unconditionally ininit_core_dependencies(), so@tool/@promptextensions keep working in every process (workers included). Cheap: nosuperset.mcp_serviceimport.initialize_core_mcp_host_tools()— doesfrom superset.mcp_service import app(the memory-heavy host-tool registration that OOMed the workers). This is the only part now gated on the flag.
Verified end to end: before init, @tool raises NotImplementedError; after initialize_core_mcp_decorators(), superset_core.mcp.decorators.tool is create_tool_decorator and decoration no longer raises — with the host-tool import still skipped when the flag is off.
|
The flagged issue is correct. When To resolve this, you should ensure that the decorators themselves are either conditionally registered or provide a safe fallback when the MCP stack is disabled. A common approach is to make the decorators no-ops or return a dummy implementation when the feature is disabled, rather than raising an error. Would you like me to fetch all comments and validate the rest of the PR to implement a comprehensive fix? superset/initialization/init.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43842 +/- ##
==========================================
+ Coverage 84.63% 84.66% +0.02%
==========================================
Files 3069 3069
Lines 205446 205722 +276
Branches 48306 48346 +40
==========================================
+ Hits 173879 174170 +291
+ Misses 28277 28262 -15
Partials 3290 3290
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:
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
LGTM — approving.
The diagnosis and the shape of the fix both look right, and defaulting to True means this is a genuine no-op until a deployment opts out.
One thing I verified explicitly, because it looks wrong at a glance and I suspect it'll come up again in review: the from superset.core.mcp.core_mcp_injection import initialize_core_mcp_dependencies at the top of init_core_dependencies sits outside the new if, so at first read the import cost appears to be paid regardless of the flag — which would defeat the stated purpose. It isn't. core_mcp_injection's module-level imports are just logging, typing, and superset.extensions.context; the expensive work (import superset_core.mcp.decorators, from fastmcp.tools import Tool, and from superset.mcp_service import app) all happens inside initialize_core_mcp_dependencies(). So gating the call does skip the heavy imports, and the memory win is real. Might be worth a one-line comment saying so, purely to save the next reader the same detour.
One question rather than a change request: gating the call also skips the two decorator replacements —
superset_core.mcp.decorators.tool = create_tool_decorator
superset_core.mcp.decorators.prompt = create_prompt_decoratorso on a process with the flag off, superset_core.mcp.decorators.tool stays the abstract version. If any extension that gets loaded in a Celery worker applies @tool/@prompt at import time, it'll now hit the abstract decorator instead of the concrete one. I couldn't check what the abstract implementation does from here (superset_core isn't in my environment). If it's an inert stub, this is a non-issue; if it raises, it'd be worth either leaving the decorator swap ungated or noting the constraint in the config docstring. You'll know the worker-side extension set better than I do.
Two trivia, neither blocking:
- The new constant lands between the feature-flag lifecycle comment block (
# - stable: .../# - deprecated: ...) andDEFAULT_FEATURE_FLAGS, so it reads as though it belongs to the feature-flag section. It isn't a feature flag — which is the right call, since this is a process-startup concern rather than a runtime toggle — so a few lines earlier or later would make that clearer. - [x] Changes UIis ticked in the description, but this is backend-only.
…port Review follow-up (Amin, sadpandajoe): the previous version gated the whole initialize_core_mcp_dependencies() call, which also skipped swapping the abstract @tool/@prompt decorators in superset_core.mcp.decorators for their concrete implementations. Those abstract decorators raise NotImplementedError, so any extension that applies @tool/@prompt at import time would crash in a process running with CORE_MCP_HOST_TOOLS_ENABLED=False (e.g. Celery workers). Split initialize_core_mcp_dependencies() into: - initialize_core_mcp_decorators(): always run; swaps in the concrete decorators. Cheap -- does not import the MCP service app. - initialize_core_mcp_host_tools(): gated on CORE_MCP_HOST_TOOLS_ENABLED; imports superset.mcp_service.app (the memory-heavy part that OOMed workers). init_core_dependencies() now always registers the decorators and only gates the host-tool import. Extensions using @tool/@prompt keep working in every process while non-MCP workers still skip the heavy MCP service app import. Also clarifies the CORE_MCP_HOST_TOOLS_ENABLED docstring (it gates host tools only) and adds regression tests covering the flag on/off/default paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@aminghadersohi — thanks for the thorough review and the approval. Your open question turned out to be a real issue, so I pushed a follow-up (f2009f2) rather than just documenting the constraint. Summary of what changed and how it lands against your notes: The decorator question (the substantive one). You were right to flag it: the abstract The "import cost is actually paid regardless" note. One correction: Your "one-line comment" suggestion. Added — Trivia: unchecked the erroneous Regression tests for the flag on/off/default paths added per sadpandajoe's request. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #0f7024Actionable 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 |
…import Addresses @sadpandajoe's review on tests/unit_tests/initialization_test.py in PR apache#43842. Joe was correct: the concrete decorators returned by create_tool_decorator / create_prompt_decorator imported superset.mcp_service.app inside their inner decorator (core_mcp_injection.py lines 101 & 211), so a real extension applying @tool/@prompt at import time would load the heavy host-tool stack even in a process that opted out via CORE_MCP_HOST_TOOLS_ENABLED=False -- defeating the worker memory benefit. The concrete decorators now skip registration (and the import) when superset.mcp_service.app is not already loaded in the process. Every MCP-serving process imports it via initialize_core_mcp_host_tools() before any extension applies a decorator, so registration still works there; a disabled worker never imports it and the decoration stays a no-op that returns the original function unchanged. Adds a regression test that exercises the concrete decorators directly (not mocked) with the service app unloaded and asserts it stays unimported, per Joe's request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
requires:rebase
The master merge left two ruff failures CI would catch: - I001: missing blank line between the stdlib and third-party import groups in core_mcp_injection.py (master's `from pydantic import TypeAdapter` and our added `import sys`/`get_type_hints` ended up adjacent). - C901: master's `create_tool_decorator` was exactly at the complexity limit (10); the lazy `_mcp_host_tools_loaded()` guard added for disabled workers tips it to 11. Suppress with `# noqa: C901` (an established pattern in the repo) rather than restructuring master's decorator. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #7a25eaActionable 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 |
github-actions
Bot
removed
the
requires:rebase
|
Heya @eschutho, the CI red here looks like the dual migration heads on master that got merged into one via 60f94cd6cd11 right after your last rebase, so a fresh rebase should clear it up. Also, @sadpandajoe's four threads look addressed in code already (f2009f2/d45a8ae) but GitHub's still showing them open, might be worth marking those resolved when you get a chance. |
| _MCP_SERVICE_APP_MODULE = "superset.mcp_service.app" | ||
|
|
||
|
|
||
| def _mcp_host_tools_loaded() -> bool: |
There was a problem hiding this comment.
_mcp_host_tools_loaded() only checks whether superset.mcp_service.app is a key in sys.modules, and Python adds a module to sys.modules the moment it starts importing it — before that module's own host-tool registration (further down in mcp_service/app.py) has actually run. A caller relying on this helper to mean "host tools are registered" could get a false positive while that module is still mid-import. Could the name/docstring make clear this signals "app module present in sys.modules" rather than "host tools finished registering"?
| def initialize_core_mcp_dependencies() -> None: | ||
| """ | ||
| Initialize MCP dependency injection by replacing abstract functions | ||
| in superset_core.api.mcp with concrete implementations, then registering the |
There was a problem hiding this comment.
This docstring says the decorator swap happens "in superset_core.api.mcp", but the code replaces attributes on superset_core.mcp.decorators (see initialize_core_mcp_decorators above and the assignments a few lines up). A reader tracing where the swap happens from this docstring would look in the wrong module.
| in superset_core.api.mcp with concrete implementations, then registering the | |
| in superset_core.mcp.decorators with concrete implementations, then registering the |
| # The heavy MCP service app import is skipped. | ||
| mock_init_host_tools.assert_not_called() | ||
|
|
||
| def test_disabled_worker_decorators_do_not_import_service_app(self): |
There was a problem hiding this comment.
This test calls create_tool_decorator/create_prompt_decorator directly rather than through superset_core.mcp.decorators.tool/.prompt, and the three tests above it mock initialize_core_mcp_decorators() out entirely. So no test in this file exercises the actual assignment inside initialize_core_mcp_decorators() (superset_core.mcp.decorators.tool = create_tool_decorator, etc.) — a regression there (e.g. binding the wrong function) would pass every current test. Could one test call the real initialize_core_mcp_decorators() and then decorate through superset_core.mcp.decorators.tool/.prompt to cover that wiring?
github-actions
Bot
added
the
requires:rebase
…ools-off-workers # Conflicts: # superset/core/mcp/core_mcp_injection.py
|
github-actions
Bot
removed
the
requires:rebase
…ations Addresses @sadpandajoe's review on PR apache#43842: - Without the optional fastmcp extra, initialize_core_mcp_decorators() skipped quietly but initialize_core_mcp_host_tools() still ran and logged "Failed to register MCP host tools" at ERROR on every boot. It now checks the same fastmcp-availability guard first and returns with a debug log, restoring the pre-split quiet-skip behavior end to end. - When the MCP service app is not loaded, the concrete @tool/@prompt decorators still no-op, but now log a WARNING once per registration so a process that serves MCP with CORE_MCP_HOST_TOOLS_ENABLED=False does not silently lose extension tools/prompts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @eschutho, the split makes sense and Amin's approval plus green CI looks good to me. One leftover: the |
| with patch.object( | ||
| core_mcp_injection, "_fastmcp_available", return_value=False | ||
| ): | ||
| core_mcp_injection.initialize_core_mcp_dependencies() |
There was a problem hiding this comment.
The missing-dependency test can still pass with the _fastmcp_available() guard in initialize_core_mcp_host_tools() deleted. The module-scoped app fixture has already imported the MCP service app, so popping sys.modules["superset.mcp_service.app"] leaves app cached as an attribute on the superset.mcp_service package; from superset.mcp_service import app then succeeds without touching sys.modules and without logging an error, so both assertions hold either way. Could this run in a state where the package attribute is also cleared (for example monkeypatch.delattr(superset.mcp_service, "app", raising=False)), or patch the import so the test fails when the guard is absent?
…guard Also clear the cached superset.mcp_service.app attribute so the import cannot succeed without sys.modules, and assert the debug skip log.
| _skipped_registrations.add(key) | ||
| logger.warning( | ||
| "Skipping MCP %s registration for %s: the MCP service app is not loaded " | ||
| "in this process (CORE_MCP_HOST_TOOLS_ENABLED is False). This is expected " |
There was a problem hiding this comment.
This warning states the flag is False, but the guard only checks whether superset.mcp_service.app is in sys.modules. With the default CORE_MCP_HOST_TOOLS_ENABLED = True, if initialize_core_mcp_host_tools() hits its caught import error, the app never loads and each extension @tool/@prompt then logs that the flag is False, sending an operator to enable a setting that is already enabled. Could the message describe the observed condition (service app not loaded) and mention the flag only as one possible cause?
… warning Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
SUMMARY
init_core_dependencies()calls the MCP host-tool setup in every process thatbuilds the Flask app — including Celery workers, which never serve MCP. Registering the MCP host
tools imports the MCP service app (
from superset.mcp_service import app), which pulls in everyhost-tool module and carries a real per-process memory cost. On high-concurrency prefork worker
pools this import can push workers over their memory limit and OOM them at boot.
This adds a
CORE_MCP_HOST_TOOLS_ENABLEDconfig flag (defaultTrue, so no behavior change)and gates the host-tool registration on it. Deployments can set it to
Falsefor processes thatnever serve MCP (e.g. Celery workers) to avoid importing the MCP service app where it is unused.
MCP decorators are always registered, independent of this flag. Registering the host tools was
originally bundled with swapping the abstract
@tool/@promptdecorators(
superset_core.mcp.decorators) for their concrete implementations. Those abstract decoratorsraise
NotImplementedError, so gating the whole thing would break any extension that applies@tool/@promptat import time in a flag-off worker. The decorator swap is therefore split outand runs unconditionally; only the host-tool import is gated.
CHANGES
superset/config.py: newCORE_MCP_HOST_TOOLS_ENABLED = Trueflag; docstring notes it gateshost-tool registration only (decorators always register).
superset/core/mcp/core_mcp_injection.py: splitinitialize_core_mcp_dependencies()intoinitialize_core_mcp_decorators()(decorator swap, always run) andinitialize_core_mcp_host_tools()(importssuperset.mcp_service.app, gated).initialize_core_mcp_dependencies()is kept as a thin wrapper calling both.superset/initialization/__init__.py:init_core_dependencies()always callsinitialize_core_mcp_decorators()and gatesinitialize_core_mcp_host_tools()on the flag.tests/unit_tests/initialization_test.py: regression tests for the flag on / off / default paths.Decisions made that were not in the original instructions
This PR gates a startup/behavior path (what happens when the flag is off), so calling out the
choices made while resolving review feedback:
the constraint,
initialize_core_mcp_decorators()runs even whenCORE_MCP_HOST_TOOLS_ENABLED=False,so
@tool/@promptextensions keep working in every process. This means a flag-off worker stillimports the decorator support (
fastmcp+mcp); measured to add ~0 MB beyond what the decoratorswap requires anyway, and far below the gated
superset.mcp_service.appimport. This is theintended, observable trade-off: correctness for extensions over shaving the (small, shared) decorator
import.
initialize_core_mcp_host_tools()keeps the existingtry/exceptthat logs failures vialogger.error(...)— the gated/degraded path is not silentlyswallowed.
core_mcp_injectionmodule import stays before the flag check (it can't move — the always-ondecorator swap lives there), which is fine: its marginal cost is ~0 MB once decorators run.
TESTING INSTRUCTIONS
True): the web app and standalone MCP service register decorators and host toolsexactly as before.
CORE_MCP_HOST_TOOLS_ENABLED = Falsein a worker's config: the worker registers the MCPdecorators (so
@tool/@promptextensions still import cleanly) but skips importing the MCPservice app / host-tool stack; web / MCP processes are unaffected.
pytest tests/unit_tests/initialization_test.py -k init_core_dependencies.ADDITIONAL INFORMATION