eschutho commented on code in PR #43842:
URL: https://github.com/apache/superset/pull/43842#discussion_r4210806140
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -181,6 +197,12 @@ def create_tool_decorator(
"""
def decorator(func: F) -> Callable[..., Any]:
+ # Skip registration when the MCP service app is not loaded in this
+ # process, so applying @tool never imports the heavy host-tool stack
+ # (see the module-level note on _MCP_SERVICE_APP_MODULE).
+ if not _mcp_host_tools_loaded():
Review Comment:
Fixed in 9c2bfad. The concrete `@tool`/`@prompt` decorators still no-op when
`superset.mcp_service.app` isn't loaded (that's what keeps opted-out workers
from paying for the host-tool stack), but the skip is no longer silent. They
now call `_warn_skipped_registration()`, which logs a WARNING naming the
tool/prompt (with its extension prefix) and says that
`CORE_MCP_HOST_TOOLS_ENABLED` must be on in any process that serves MCP. Each
registration is logged once, deduplicated per `kind:id`, so re-imports don't
flood the logs. A shared-config `superset mcp run` that drops extension tools
will now show exactly which ones and why.
I didn't take the other option (forcing host-tool loading in the standalone
entrypoint regardless of the flag). That would override an explicit operator
setting, and the warning is enough to diagnose it. Happy to add that too if
you'd prefer it.
New test: `test_skipped_decoration_warns_once`. It applies the same tool
twice and a prompt once with the service app unloaded, and asserts exactly two
warnings (tool deduped, plus the prompt) and that the app stays unimported. It
fails on the previous head.
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -396,6 +433,18 @@ def initialize_core_mcp_dependencies() -> None:
logger.info("MCP dependency injection initialized successfully")
+
+def initialize_core_mcp_host_tools() -> None:
Review Comment:
Fixed in 9c2bfad, with the same guard in both steps. I factored the
availability check into `_fastmcp_available()`.
`initialize_core_mcp_decorators()` uses it as before, and
`initialize_core_mcp_host_tools()` now checks it first: with no fastmcp it logs
at DEBUG and returns before attempting `from superset.mcp_service import app`.
An install without the `fastmcp` extra is back to a quiet skip on boot, with no
`Failed to register MCP host tools` ERROR. The two-function split and the
`CORE_MCP_HOST_TOOLS_ENABLED` gate are unchanged. The guard lives in the
function itself, so the compat wrapper `initialize_core_mcp_dependencies()` and
any direct caller are covered as well as `init_core_dependencies()`.
New test: `test_host_tools_quietly_skipped_without_fastmcp`. It patches
`_fastmcp_available` to False, runs `initialize_core_mcp_dependencies()`, and
asserts no ERROR is logged and the service app isn't imported. It fails on the
previous head.
--
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]
