-
Notifications
You must be signed in to change notification settings - Fork 15.8k
fix: Enable Playwright migration with graceful Selenium fallback #35063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
fbe6a8b
2f9a5bf
d500abe
638ce4c
49842ce
4f341bb
23db49d
75f60f5
0a8509a
65f3e49
68797f4
d531d3e
4750b0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,13 @@ | |
WindowSize = tuple[int, int] | ||
logger = logging.getLogger(__name__) | ||
|
||
# Installation message for missing Playwright (Cypress doesn't work with DeckGL) | ||
PLAYWRIGHT_INSTALL_MESSAGE = ( | ||
"To complete the migration from Cypress " | ||
"and enable WebGL/DeckGL screenshot support, install Playwright with: " | ||
"pip install playwright && playwright install chromium" | ||
) | ||
|
||
if TYPE_CHECKING: | ||
from typing import Any | ||
|
||
|
@@ -71,6 +78,58 @@ | |
sync_playwright = None | ||
|
||
|
||
# Check Playwright availability at module level | ||
def check_playwright_availability() -> bool: | ||
""" | ||
Comprehensive check for Playwright availability. | ||
|
||
Verifies not only that the module is imported, but also that | ||
browser binaries are installed and can be launched. | ||
""" | ||
if sync_playwright is None: | ||
return False | ||
|
||
try: | ||
# Try to actually launch a browser to ensure binaries are installed | ||
with sync_playwright() as p: | ||
browser = p.chromium.launch(headless=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there may be a lighter way to do this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed it to check if the browser binary exists first, only launches if that fails |
||
browser.close() | ||
return True | ||
except Exception as e: | ||
logger.warning( | ||
"Playwright module is installed but browser launch failed. " | ||
"Run 'playwright install chromium' to install browser binaries. " | ||
"Error: %s", | ||
str(e), | ||
) | ||
return False | ||
|
||
|
||
PLAYWRIGHT_AVAILABLE = check_playwright_availability() | ||
|
||
|
||
def validate_webdriver_config() -> dict[str, Any]: | ||
""" | ||
Validate webdriver configuration and dependencies. | ||
|
||
Used to check migration status from Cypress to Playwright. | ||
Returns a dictionary with the status of available webdrivers | ||
and feature flags. | ||
""" | ||
from superset import feature_flag_manager | ||
|
||
return { | ||
"selenium_available": True, # Always available as required dependency | ||
"playwright_available": PLAYWRIGHT_AVAILABLE, | ||
"playwright_feature_enabled": feature_flag_manager.is_feature_enabled( | ||
"PLAYWRIGHT_REPORTS_AND_THUMBNAILS" | ||
), | ||
"recommended_action": ( | ||
PLAYWRIGHT_INSTALL_MESSAGE if not PLAYWRIGHT_AVAILABLE else None | ||
), | ||
} | ||
|
||
|
||
class DashboardStandaloneMode(Enum): | ||
HIDE_NAV = 1 | ||
HIDE_NAV_AND_TITLE = 2 | ||
|
@@ -87,8 +146,8 @@ class WebDriverProxy(ABC): | |
def __init__(self, driver_type: str, window: WindowSize | None = None): | ||
self._driver_type = driver_type | ||
self._window: WindowSize = window or (800, 600) | ||
self._screenshot_locate_wait = app.config["SCREENSHOT_LOCATE_WAIT"] | ||
self._screenshot_load_wait = app.config["SCREENSHOT_LOAD_WAIT"] | ||
self._screenshot_locate_wait = app.config.get("SCREENSHOT_LOCATE_WAIT", 10) | ||
self._screenshot_load_wait = app.config.get("SCREENSHOT_LOAD_WAIT", 60) | ||
|
||
|
||
@abstractmethod | ||
def get_screenshot(self, url: str, element_name: str, user: User) -> bytes | None: | ||
|
@@ -151,6 +210,15 @@ def find_unexpected_errors(page: Page) -> list[str]: | |
def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # noqa: C901 | ||
self, url: str, element_name: str, user: User | ||
) -> bytes | None: | ||
if not PLAYWRIGHT_AVAILABLE: | ||
logger.info( | ||
"Playwright not available - falling back to Selenium. " | ||
"Note: WebGL/Canvas charts may not render correctly with Selenium. " | ||
"%s", | ||
PLAYWRIGHT_INSTALL_MESSAGE, | ||
) | ||
return None | ||
|
||
with sync_playwright() as playwright: | ||
browser_args = app.config["WEBDRIVER_OPTION_ARGS"] | ||
browser = playwright.chromium.launch(args=browser_args) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a Flask app (multiple threads) or a Celery worker (multiple processes), this may not behave as we would expect:
Each process will have its own memory space, so the log suppression will only work per process, not globally.
In threaded mode, modifying the global without a lock could lead to race conditions (unlikely to matter much for a boolean flag, but still possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about the concurrency issues. Added thread safety with a lock to prevent race conditions in threaded Flask mode. The per-process behavior in Celery is actually fine (one log per worker), but the threading needed fixing.