sadpandajoe commented on code in PR #43842:
URL: https://github.com/apache/superset/pull/43842#discussion_r3930831971
##########
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:
This opt-out still imports `superset.core.mcp.core_mcp_injection` before
evaluating the flag. That module imports `mcp.types`, whose package initializer
imports MCP client/server modules, so high-concurrency workers still load much
of the stack this setting is meant to avoid. Could the injection import move
inside the enabled branch?
##########
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:
There is no regression coverage for the process-level opt-out. Could a
focused initialization test set `CORE_MCP_HOST_TOOLS_ENABLED=False` and assert
MCP registration/import is skipped while core API initialization still runs,
alongside the default-enabled case?
##########
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):
+ initialize_core_mcp_dependencies()
Review 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?
--
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]
