madhushreeag commented on code in PR #43028:
URL: https://github.com/apache/superset/pull/43028#discussion_r3760518789
##########
superset/utils/webdriver.py:
##########
@@ -94,39 +79,8 @@
def check_playwright_availability() -> bool:
- """
- Lightweight check for Playwright availability.
-
- First checks if browser binary exists, falls back to launch test if needed.
- """
- if sync_playwright is None:
- return False
-
- try:
- with sync_playwright() as p:
- # First try lightweight check - just verify executable exists
- try:
- executable_path = p.chromium.executable_path
- if executable_path:
- return True
- except Exception:
- # Fall back to full launch test if executable_path fails
- logger.debug(
- "Executable path check failed, falling back to launch test"
- )
-
- # Fallback: actually launch browser to ensure it works
- browser = p.chromium.launch(headless=True)
- 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
+ """Check Playwright availability by verifying the module is importable."""
+ return sync_playwright is not None
Review Comment:
get_browser() is now wrapped in a try/except that catches any launch failure
(e.g. missing browser binary after pip install playwright without playwright
install chromium) and re-raises it as a RuntimeError with the same actionable
installation message as the module-unavailable path.
##########
superset/tasks/cache.py:
##########
@@ -397,21 +397,17 @@ def cache_warmup(
return results
- wd: WebDriverSelenium = WebDriverSelenium(
- current_app.config["WEBDRIVER_TYPE"], user=user
+ wd: WebDriverPlaywright = WebDriverPlaywright(
+ "", current_app.config["WEBDRIVER_WINDOW"]["dashboard"]
)
- try:
- for url in strategy.get_urls():
- try:
- logger.info("Fetching %s", url)
- wd.get_screenshot(url, "grid-container")
- results["success"].append(url)
- except Exception: # noqa: BLE001
- logger.exception("Error warming up cache for %s", url)
- results["errors"].append(url)
- finally:
- # Ensure WebDriver is properly cleaned up
- wd.destroy()
+ for url in strategy.get_urls():
+ try:
+ logger.info("Fetching %s", url)
+ wd.get_screenshot(url, "grid-container", user=user)
+ results["success"].append(url)
Review Comment:
get_screenshot return value is now checked. A falsey result (including None)
is logged as a warning and appended to errors rather than success. Added a test
covering the None case.
--
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]
