dpgaspar commented on code in PR #35063:
URL: https://github.com/apache/superset/pull/35063#discussion_r2368943726
##########
superset/utils/webdriver.py:
##########
@@ -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)
Review Comment:
nit: We used to have a rule to be strict with the config, all keys have
defaults configured so we expect `app.config["SOME_KEY"]` instead of
`app.config.get` unsure if we still have this "rule"
##########
superset/utils/screenshots.py:
##########
@@ -169,7 +182,22 @@ def __init__(self, url: str, digest: str | None):
def driver(self, window_size: WindowSize | None = None) -> WebDriver:
window_size = window_size or self.window_size
if
feature_flag_manager.is_feature_enabled("PLAYWRIGHT_REPORTS_AND_THUMBNAILS"):
- return WebDriverPlaywright(self.driver_type, window_size)
+ # Try to use Playwright if available (supports WebGL/DeckGL,
unlike Cypress)
+ if PLAYWRIGHT_AVAILABLE:
+ return WebDriverPlaywright(self.driver_type, window_size)
+
+ # Log fallback only once to avoid log spam on repeated operations
+ global _PLAYWRIGHT_FALLBACK_LOGGED
+ if not _PLAYWRIGHT_FALLBACK_LOGGED:
+ logger.info(
+ "PLAYWRIGHT_REPORTS_AND_THUMBNAILS enabled but Playwright
not "
+ "installed. Falling back to Selenium (WebGL/Canvas charts
may "
+ "not render correctly). %s",
+ PLAYWRIGHT_INSTALL_MESSAGE,
+ )
+ _PLAYWRIGHT_FALLBACK_LOGGED = True
Review Comment:
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).
##########
superset/utils/webdriver.py:
##########
@@ -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)
Review Comment:
there may be a lighter way to do this
--
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]
