dpgaspar commented on code in PR #35163: URL: https://github.com/apache/superset/pull/35163#discussion_r2355419642
########## superset/mcp_service/scripts/setup.py: ########## @@ -0,0 +1,149 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Setup utilities for MCP service configuration""" + +import secrets +from pathlib import Path + +import click +from colorama import Fore, Style + + +def run_setup(force: bool) -> None: Review Comment: Why do we need this? How about using a JINJA template, or just create a base file and append `SECRET_KEY = '{secrets.token_urlsafe(42)}'` saying it because it does not seem very maintainable this way (specifically talking about the `config_content`. WDYT? ########## superset/mcp_service/flask_singleton.py: ########## @@ -0,0 +1,78 @@ +""" +Singleton pattern for Flask app creation in MCP service. + +This module ensures that only one Flask app instance is created and reused +throughout the MCP service lifecycle. This prevents issues with multiple +app instances and improves performance. +""" + +import logging +import threading +from typing import Optional + +from flask import Flask + +logger = logging.getLogger(__name__) + +# Singleton instance storage +_flask_app: Optional[Flask] = None +_flask_app_lock = threading.Lock() + + +def get_flask_app() -> Flask: + """ + Get or create the singleton Flask app instance. + + This function ensures that only one Flask app is created, even when called + from multiple threads or contexts. The app is created lazily on first access. + + Returns: + Flask: The singleton Flask app instance + """ + global _flask_app + + # Fast path: if app already exists, return it + if _flask_app is not None: + return _flask_app + + # Slow path: acquire lock and create app if needed + with _flask_app_lock: + # Double-check pattern: verify app still doesn't exist after acquiring lock + if _flask_app is not None: + return _flask_app + + logger.info("Creating singleton Flask app instance for MCP service") + + try: + from superset.app import create_app + from superset.mcp_service.config import DEFAULT_CONFIG + + # Create the Flask app instance + _flask_app = create_app() Review Comment: This could create issues on multiple environments ########## superset/mcp_service/scripts/setup.py: ########## @@ -0,0 +1,149 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Setup utilities for MCP service configuration""" + +import secrets +from pathlib import Path + +import click +from colorama import Fore, Style + + +def run_setup(force: bool) -> None: + """Set up MCP service configuration for Apache Superset""" + click.echo(f"{Fore.CYAN}=== Apache Superset MCP Service Setup ==={Style.RESET_ALL}") + click.echo() + + # Check if already set up + config_path = Path("superset_config.py") + + # Configuration file setup + if config_path.exists() and not force: + click.echo( + f"{Fore.YELLOW}⚠️ superset_config.py already exists{Style.RESET_ALL}" + ) + if click.confirm("Do you want to check/add missing MCP settings?"): + _update_config_file(config_path) + else: + click.echo("Keeping existing configuration") + else: + _create_config_file(config_path) + + click.echo() + click.echo(f"{Fore.GREEN}=== Setup Complete! ==={Style.RESET_ALL}") + click.echo() + click.echo("To start MCP service:") + click.echo(" superset mcp run") + + +def _create_config_file(config_path: Path) -> None: + """Create a new superset_config.py file with MCP configuration""" + click.echo("Creating new superset_config.py...") + + config_content = f"""# Apache Superset Configuration +SECRET_KEY = '{secrets.token_urlsafe(42)}' + +# Session configuration for local development +SESSION_COOKIE_HTTPONLY = True +SESSION_COOKIE_SECURE = False +SESSION_COOKIE_SAMESITE = 'Lax' +SESSION_COOKIE_NAME = 'superset_session' +PERMANENT_SESSION_LIFETIME = 86400 + +# CSRF Protection (disable if login loop occurs) +WTF_CSRF_ENABLED = True +WTF_CSRF_TIME_LIMIT = None + +# MCP Service Configuration +MCP_ADMIN_USERNAME = 'admin' +MCP_DEV_USERNAME = 'admin' +SUPERSET_WEBSERVER_ADDRESS = 'http://localhost:9001' + +# WebDriver Configuration for screenshots +WEBDRIVER_BASEURL = 'http://localhost:9001/' +WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL + +# Feature flags +FEATURE_FLAGS = {{ + "MCP_SERVICE": True, +}} + +# MCP Service Host/Port +MCP_SERVICE_HOST = "localhost" +MCP_SERVICE_PORT = 5008 +""" + + config_path.write_text(config_content) + click.echo(f"{Fore.GREEN}✓ Created superset_config.py{Style.RESET_ALL}") + + +def _update_config_file(config_path: Path) -> None: + """Update existing config file with missing MCP settings""" + content = config_path.read_text() + updated = False + additions = [] + + # Check for missing settings + if "SECRET_KEY" not in content: + additions.append(f"SECRET_KEY = '{secrets.token_urlsafe(42)}'") + updated = True + + # Add MCP configuration block if missing + if "MCP_ADMIN_USERNAME" not in content: + additions.append("\n# MCP Service Configuration") + additions.append("MCP_ADMIN_USERNAME = 'admin'") + additions.append("MCP_DEV_USERNAME = 'admin'") + additions.append("SUPERSET_WEBSERVER_ADDRESS = 'http://localhost:9001'") + updated = True + + # Add WebDriver configuration if missing + if "WEBDRIVER_BASEURL" not in content: + additions.append("\n# WebDriver Configuration for screenshots") + additions.append("WEBDRIVER_BASEURL = 'http://localhost:9001/'") + additions.append("WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL") Review Comment: oh! does MCP need a webdriver, just out of curiosity ########## superset/mcp_service/app.py: ########## @@ -0,0 +1,100 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +FastMCP app instance and initialization for Superset MCP service. +This file provides the global FastMCP instance (mcp) and a function to initialize +the server. All tool modules should import mcp from here and use @mcp.tool decorators. +""" + +import logging + +from fastmcp import FastMCP + +logger = logging.getLogger(__name__) + +# Create MCP instance without auth for scaffold +mcp = FastMCP( Review Comment: It would be great if we could have a configurable factory to create the FastMCP instance, this way users can freely configure it the way they want/need, add custom Auth, Middleware etc. Copying the Flask pattern. https://gofastmcp.com/servers/server#creating-a-server ########## superset/mcp_service/server.py: ########## @@ -0,0 +1,74 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +MCP server for Apache Superset +""" + +import logging +import os + +# Apply Flask-AppBuilder compatibility patches before any Superset imports +from superset.mcp_service.app import init_fastmcp_server, mcp + + +def configure_logging(debug: bool = False) -> None: + """Configure logging for the MCP service.""" + import sys + + if debug or os.environ.get("SQLALCHEMY_DEBUG"): + logging.basicConfig( + level=logging.INFO, + format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", + stream=sys.stderr, # Always log to stderr, not stdout + ) + for logger_name in [ + "sqlalchemy.engine", + "sqlalchemy.pool", + "sqlalchemy.dialects", + ]: + logging.getLogger(logger_name).setLevel(logging.INFO) Review Comment: Will this remove the possibility to configure logging using a `logger.ini` file? I find it to be the easiest and most flexible way to do it in production ########## superset/mcp_service/__main__.py: ########## @@ -0,0 +1,136 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Main entry point for running the MCP service in stdio mode. +This allows running the service with: python -m superset.mcp_service +""" + +import contextlib +import io +import logging +import os +import sys +import warnings +from typing import Any + +# Must redirect click output BEFORE importing anything that uses it +import click + +# Monkey-patch click to redirect output to stderr in stdio mode +if os.environ.get("FASTMCP_TRANSPORT", "stdio") == "stdio": + original_secho = click.secho + + def secho_to_stderr(*args: Any, **kwargs: Any) -> Any: + kwargs["file"] = sys.stderr + return original_secho(*args, **kwargs) + + click.secho = secho_to_stderr + click.echo = lambda *args, **kwargs: click.echo(*args, file=sys.stderr, **kwargs) + +from superset.mcp_service.app import init_fastmcp_server, mcp + + +def main() -> None: + """ + Run the MCP service in stdio mode with proper output suppression. + """ + # Determine if we're running in stdio mode + transport = os.environ.get("FASTMCP_TRANSPORT", "stdio") + + if transport == "stdio": + # Suppress ALL output to stdout except for MCP messages + # This includes Flask initialization messages, warnings, etc. + + # Redirect stderr to suppress logging output + # We'll keep stderr for debugging if needed + logging.basicConfig( + level=logging.CRITICAL, # Only show critical errors + stream=sys.stderr, + format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", + ) + + # Disable all Flask/Superset logging to stdout + for logger_name in [ + "superset", + "flask", + "werkzeug", + "sqlalchemy", + "flask_appbuilder", + "celery", + "alembic", + ]: + logger = logging.getLogger(logger_name) + logger.setLevel(logging.CRITICAL) + # Filter out stdout handlers safely + new_handlers = [] + for h in logger.handlers: + if hasattr(h, "stream") and h.stream != sys.stdout: + new_handlers.append(h) + elif not hasattr(h, "stream"): + # Keep handlers that don't have a stream attribute + new_handlers.append(h) + logger.handlers = new_handlers + + # Suppress warnings + warnings.filterwarnings("ignore") + + # Capture any print statements during initialization + captured_output = io.StringIO() + + # Set up Flask app context for database access + from superset.mcp_service.flask_singleton import get_flask_app + + # Temporarily redirect stdout during Flask app creation + with contextlib.redirect_stdout(captured_output): + flask_app = get_flask_app() + # Initialize the FastMCP server + # Disable auth config for stdio mode to avoid Flask app output + init_fastmcp_server() + + # Log captured output to stderr for debugging (optional) + captured = captured_output.getvalue() + if captured and os.environ.get("MCP_DEBUG"): Review Comment: This OS environ var possibility is a bit hidden in the code. Use app.config instead? app.config can easily be templated to use env vars ########## superset/mcp_service/__main__.py: ########## @@ -0,0 +1,136 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Main entry point for running the MCP service in stdio mode. +This allows running the service with: python -m superset.mcp_service +""" + +import contextlib +import io +import logging +import os +import sys +import warnings +from typing import Any + +# Must redirect click output BEFORE importing anything that uses it +import click + +# Monkey-patch click to redirect output to stderr in stdio mode +if os.environ.get("FASTMCP_TRANSPORT", "stdio") == "stdio": + original_secho = click.secho + + def secho_to_stderr(*args: Any, **kwargs: Any) -> Any: + kwargs["file"] = sys.stderr + return original_secho(*args, **kwargs) + + click.secho = secho_to_stderr + click.echo = lambda *args, **kwargs: click.echo(*args, file=sys.stderr, **kwargs) + +from superset.mcp_service.app import init_fastmcp_server, mcp + + +def main() -> None: + """ + Run the MCP service in stdio mode with proper output suppression. + """ + # Determine if we're running in stdio mode + transport = os.environ.get("FASTMCP_TRANSPORT", "stdio") + + if transport == "stdio": + # Suppress ALL output to stdout except for MCP messages + # This includes Flask initialization messages, warnings, etc. + + # Redirect stderr to suppress logging output + # We'll keep stderr for debugging if needed + logging.basicConfig( + level=logging.CRITICAL, # Only show critical errors + stream=sys.stderr, + format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", + ) + + # Disable all Flask/Superset logging to stdout + for logger_name in [ + "superset", + "flask", + "werkzeug", + "sqlalchemy", + "flask_appbuilder", + "celery", + "alembic", + ]: + logger = logging.getLogger(logger_name) + logger.setLevel(logging.CRITICAL) + # Filter out stdout handlers safely + new_handlers = [] + for h in logger.handlers: + if hasattr(h, "stream") and h.stream != sys.stdout: + new_handlers.append(h) + elif not hasattr(h, "stream"): + # Keep handlers that don't have a stream attribute + new_handlers.append(h) + logger.handlers = new_handlers + + # Suppress warnings + warnings.filterwarnings("ignore") Review Comment: I prefer not to be opinionated about logging configuration. Just let `logger.ini` do it's work ########## superset/mcp_service/flask_singleton.py: ########## @@ -0,0 +1,78 @@ +""" +Singleton pattern for Flask app creation in MCP service. + +This module ensures that only one Flask app instance is created and reused +throughout the MCP service lifecycle. This prevents issues with multiple +app instances and improves performance. +""" + +import logging +import threading +from typing import Optional + +from flask import Flask + +logger = logging.getLogger(__name__) + +# Singleton instance storage +_flask_app: Optional[Flask] = None +_flask_app_lock = threading.Lock() + + +def get_flask_app() -> Flask: Review Comment: Why do we need this? Under normal circumstances a simple module with just the instance is enough https://stackoverflow.com/questions/6760685/what-is-the-best-way-of-implementing-a-singleton-in-python -- 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]
