eschutho commented on code in PR #43842:
URL: https://github.com/apache/superset/pull/43842#discussion_r3946662025
##########
superset/initialization/__init__.py:
##########
@@ -558,7 +558,12 @@ def init_core_dependencies(self) -> None:
)
initialize_core_api_dependencies()
- initialize_core_mcp_dependencies()
+ # MCP host tools only need to be registered in processes that serve MCP
+ # (the web app and the standalone MCP service). Deployments can disable
+ # this in processes that never serve MCP -- e.g. Celery workers -- to
+ # avoid importing the MCP stack where it is unused.
+ if self.config.get("CORE_MCP_HOST_TOOLS_ENABLED", True):
Review Comment:
Good eye — and you're right that `core_mcp_injection` is imported before the
flag is evaluated (its `@tool`/`@prompt` support now has to be, since the
decorator swap always runs). Its module-level imports are `logging`, `typing`,
`superset.extensions.context`, and — the one that matters here — `from
mcp.types import ToolAnnotations` (guarded by `try/except ImportError`, falling
back to `dict`).
I measured the actual cost, and it turns out to be a non-issue:
- **When `mcp`/`fastmcp` are installed** (the case for any worker that could
load a `@tool` extension): the always-on decorator step imports
`fastmcp.tools`, which pulls in `mcp` anyway. With that already loaded, the
module-level `from mcp.types import ToolAnnotations` adds **0 MB** on top —
it's the same package. So the residual import is fully subsumed by the
decorator registration we now have to do regardless.
- **When they're not installed**: `from mcp.types import ToolAnnotations`
hits `ImportError` and falls back to `dict` — zero cost — and the decorator
step early-returns on the `fastmcp` guard.
The expensive part you were worried about — `from superset.mcp_service
import app`, which imports the MCP service app and every host-tool module — has
moved into `initialize_core_mcp_host_tools()` and is the only thing now gated.
That's the real per-process win (it's what was OOMing the 12-way prefork
`worker-med` pool), and it's fully skipped when the flag is off.
So I didn't move the `core_mcp_injection` import inside the gate: it can't
move (the always-on decorator swap lives in that module), and it doesn't need
to (its marginal cost is 0 MB once decorators run, or free when `mcp` is
absent).
##########
superset/initialization/__init__.py:
##########
@@ -558,7 +558,12 @@ def init_core_dependencies(self) -> None:
)
initialize_core_api_dependencies()
- initialize_core_mcp_dependencies()
+ # MCP host tools only need to be registered in processes that serve MCP
+ # (the web app and the standalone MCP service). Deployments can disable
+ # this in processes that never serve MCP -- e.g. Celery workers -- to
+ # avoid importing the MCP stack where it is unused.
+ if self.config.get("CORE_MCP_HOST_TOOLS_ENABLED", True):
Review Comment:
Added in f2009f2 — `tests/unit_tests/initialization_test.py` now has three
focused tests on `SupersetAppInitializer.init_core_dependencies`, patching the
injection functions and asserting the call pattern:
- `test_init_core_dependencies_skips_host_tools_when_disabled` —
`CORE_MCP_HOST_TOOLS_ENABLED=False`: `initialize_core_mcp_host_tools` is
**not** called, while `initialize_core_api_dependencies` and
`initialize_core_mcp_decorators` still run (so core init and the decorator swap
are unaffected).
- `test_init_core_dependencies_registers_host_tools_when_enabled` — flag
`True`: all three run.
- `test_init_core_dependencies_registers_host_tools_by_default` — flag
absent: defaults to on, all three run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
