eschutho commented on PR #43842:
URL: https://github.com/apache/superset/pull/43842#issuecomment-5565263098
@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 `superset_core.mcp.decorators.tool`/`prompt` **raise
`NotImplementedError`** (verified against source — both bodies are `raise
NotImplementedError("MCP ... decorator not initialized ...")`). So the previous
all-or-nothing gate would have broken any extension that applies
`@tool`/`@prompt` at import time in a flag-off worker. Fixed by splitting the
decorator swap (now **always** runs — cheap, no `mcp_service` import) from the
host-tool registration (`from superset.mcp_service import app`, the heavy part,
still gated). So `@tool`/`@prompt` extensions keep working everywhere, and
non-MCP workers still skip the heavy import.
**The "import cost is actually paid regardless" note.** One correction:
`core_mcp_injection`'s module-level imports aren't only
`logging`/`typing`/`superset.extensions.context` — there's also a guarded `from
mcp.types import ToolAnnotations`. sadpandajoe flagged the same thing. I
measured it: once the (now always-on) decorator step imports `fastmcp.tools`,
`mcp` is already loaded, so the module-level `mcp.types` import adds **0 MB**
on top — and when `mcp`/`fastmcp` aren't installed it falls back to `dict` for
free. The real win is gating `from superset.mcp_service import app` (the
service app + all host-tool modules), which is preserved. Details in the thread
on that line.
**Your "one-line comment" suggestion.** Added — `init_core_dependencies()`
now has inline comments explaining that the decorator swap is unconditional and
only the host-tool import is gated, and the `CORE_MCP_HOST_TOOLS_ENABLED`
docstring now spells out that it gates host tools only (decorators always
register).
**Trivia:** unchecked the erroneous `Changes UI` box (this is backend-only).
On the constant placement — agreed it isn't a feature flag; I kept it out of
`DEFAULT_FEATURE_FLAGS` for exactly that reason and the docstring now makes the
"process-startup concern, not a runtime toggle" intent explicit.
Regression tests for the flag on/off/default paths added per sadpandajoe's
request.
--
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]
