aminghadersohi commented on code in PR #43784:
URL: https://github.com/apache/superset/pull/43784#discussion_r3920525955
##########
superset/utils/screenshot_utils.py:
##########
@@ -805,28 +889,127 @@ def _raise_if_budget_exhausted() -> None:
"height": clip_height,
}
+ try:
+ contentful_chart_holders = page.evaluate(
+ CONTENTFUL_CHART_HOLDERS_IN_CLIP_JS,
+ {"top": clip_y, "bottom": clip_y + clip_height},
+ )
+ if not isinstance(contentful_chart_holders, int):
+ contentful_chart_holders = 0
+ except Exception: # noqa: BLE001
+ logger.warning(
+ "Unable to count chart holders intersecting tile %s/%s%s",
+ i + 1,
+ num_tiles,
+ context_suffix,
+ exc_info=True,
+ )
+ contentful_chart_holders = 0
Review Comment:
**`0` means both "no charts here" and "the check broke", and one of those
silently disables the fix.**
`is_blank = is_uniform and contentful_chart_holders > 0`, so
`contentful_chart_holders = 0` makes a uniform tile unconditionally acceptable.
Two ways to reach `0` that are not "this tile legitimately has no charts":
1. **This `except`** — `evaluate` raised. Logged, at least.
2. **`CHART_HOLDER_SELECTOR` stops matching** — a renamed dashboard class, a
DOM restructure. The count is a legitimate `0`, no exception, no warning. The
only trace is `contentful_chart_holders=0` in a **DEBUG** log that is off in
production.
In case 2 every uniform tile is accepted and the blank-tile detection is
silently a no-op, which is the failure this PR exists to prevent. The readiness
gate still stands in front of it, so this degrades to pre-PR behaviour rather
than something worse — but it degrades *invisibly*, and a defence that can
silently switch itself off is hard to trust later.
#43348 handled the identical ambiguity deliberately, writing `holders.length
> 0 && unready.length === 0` in the report readiness predicates so an empty
holder set could never read as ready. Worth carrying that reasoning here — e.g.
have the JS return `{total, contentful}` and treat `total === 0` across *every*
tile of a dashboard that `expected_chart_count` says has charts as a WARNING,
distinct from a per-tile `contentful === 0`.
At minimum, make the `except` path fail *closed* (treat as contentful)
rather than open — a wasted retry is much cheaper than a blank report.
##########
superset/utils/screenshot_utils.py:
##########
@@ -805,28 +889,127 @@ def _raise_if_budget_exhausted() -> None:
"height": clip_height,
}
+ try:
+ contentful_chart_holders = page.evaluate(
+ CONTENTFUL_CHART_HOLDERS_IN_CLIP_JS,
+ {"top": clip_y, "bottom": clip_y + clip_height},
+ )
+ if not isinstance(contentful_chart_holders, int):
+ contentful_chart_holders = 0
+ except Exception: # noqa: BLE001
+ logger.warning(
+ "Unable to count chart holders intersecting tile %s/%s%s",
+ i + 1,
+ num_tiles,
+ context_suffix,
+ exc_info=True,
+ )
+ contentful_chart_holders = 0
+
# Take screenshot with clipping to capture only this tile's content
- capture_timeout = (
- _timeout_seconds(
- "screenshot_capture",
- reserve_seconds=(
- report_execution_context.post_capture_reserve_seconds
- if report_execution_context
- else 0.0
- ),
+ tile_screenshot: bytes | None = None
+ for capture_attempt in range(1,
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS + 1):
+ capture_timeout = (
+ _timeout_seconds(
+ "screenshot_capture",
+
requested_seconds=TILED_SCREENSHOT_CAPTURE_TIMEOUT_SECONDS,
+ reserve_seconds=(
+
report_execution_context.post_capture_reserve_seconds
+ if report_execution_context
+ else 0.0
+ ),
+ )
+ if report_execution_context or task_budget is not None
+ else None
)
- if report_execution_context or task_budget is not None
- else None
- )
- tile_screenshot = page.screenshot(
- type="png",
- clip=clip,
- **(
- {"timeout": capture_timeout * 1000}
- if capture_timeout is not None
- else {}
- ),
- )
+ capture_started_at = time.monotonic()
+ try:
+ candidate = page.screenshot(
+ type="png",
+ clip=clip,
+ **(
+ {"timeout": capture_timeout * 1000}
+ if capture_timeout is not None
+ else {}
+ ),
+ )
+ except PlaywrightTimeout as ex:
+ capture_elapsed = time.monotonic() - capture_started_at
+ logger.warning(
+ "report_capture_tile_timeout tile=%s/%s attempt=%s/%s "
+ "capture_elapsed_seconds=%.2f%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS,
+ capture_elapsed,
+ context_suffix,
+ )
+ if capture_attempt ==
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS:
+ raise ScreenshotCaptureTimeoutError(
+ f"Chromium timed out capturing tile {i +
1}/{num_tiles} "
+ f"after {capture_attempt} attempts"
+ ) from ex
+ else:
+ capture_elapsed = time.monotonic() - capture_started_at
+ is_uniform, dominant_ratio =
is_screenshot_nearly_uniform(candidate)
+ is_blank = is_uniform and contentful_chart_holders > 0
+ logger.debug(
+ "Captured tile %s/%s attempt %s/%s in %.2fs "
+ "(contentful_chart_holders=%s
dominant_pixel_ratio=%.5f)%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS,
+ capture_elapsed,
+ contentful_chart_holders,
+ dominant_ratio,
+ context_suffix,
+ )
+ if not is_blank:
+ tile_screenshot = candidate
+ break
+ blank_tile_retries += 1
+ logger.warning(
+ "report_capture_blank_tile tile=%s/%s attempt=%s/%s "
+ "capture_elapsed_seconds=%.2f
contentful_chart_holders=%s "
+ "dominant_pixel_ratio=%.5f%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS,
+ capture_elapsed,
+ contentful_chart_holders,
+ dominant_ratio,
+ context_suffix,
+ )
+ if capture_attempt ==
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS:
+ tile_screenshot = candidate
+ logger.warning(
+ "report_capture_uniform_tile_retained tile=%s/%s "
+ "attempts=%s contentful_chart_holders=%s "
+ "dominant_pixel_ratio=%.5f%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ contentful_chart_holders,
+ dominant_ratio,
+ context_suffix,
+ )
+ break
+
+ _raise_if_budget_exhausted()
+ page.bring_to_front()
+ page.evaluate(
Review Comment:
**The bounded capture is followed by an unbounded recovery.**
`TILED_SCREENSHOT_CAPTURE_TIMEOUT_SECONDS` exists, per its own comment, to
*"keep each CDP capture bounded so a wedged compositor cannot consume the
report deadline."* This `evaluate` then reopens that hole.
`page.evaluate()` in the Playwright sync API takes `(expression, arg)` —
**there is no `timeout` parameter**, and Playwright's default timeout does not
apply to it. The expression returns a Promise that only settles inside a nested
`requestAnimationFrame`. If rAF is not being serviced, the promise never
resolves and this call blocks until the Celery hard time limit kills the worker
— past the report deadline and past the `_raise_if_budget_exhausted()` on the
line above, which has already run by then.
The condition under which rAF stops being serviced is a stalled compositor,
which is the literal premise of this retry path. `page.bring_to_front()` covers
the occluded-tab case, but not a genuinely wedged one.
This is also the only promise-returning `evaluate` in the module. Every
other wait here uses `page.wait_for_function(..., timeout=...)` (lines 611,
741). Same thing would work:
```python
page.evaluate("() => { window.scrollBy(0, 1); window.scrollBy(0, -1); "
"window.__supersetRepaint = false; "
"requestAnimationFrame(() => requestAnimationFrame("
"() => { window.__supersetRepaint = true; })); }")
page.wait_for_function("() => window.__supersetRepaint === true",
timeout=min(5, remaining) * 1000)
```
with the timeout swallowed — failing to repaint should cost one more capture
attempt, not the whole report.
##########
UPDATING.md:
##########
@@ -230,8 +230,10 @@ Behavior changes to be aware of:
fail fast at the first phase check rather than erroring at setup.
- Dashboard reports whose charts have not mounted are no longer captured
blank: readiness is polled until the deadline, and the report fails loudly
- if charts never mount. Thumbnails and non-report screenshots keep their
- previous behavior.
+ if charts never mount. Large tiled reports also retry Chromium screenshot
+ stalls and suspicious uniform tiles, while persistent screenshot timeouts
+ fail loudly. Thumbnails and non-report screenshots keep their previous
Review Comment:
*"Thumbnails and non-report screenshots keep their previous behavior"* is no
longer accurate for this PR.
Neither the retry loop nor `ScreenshotCaptureTimeoutError` is gated on
`report_execution_context`, and `take_tiled_screenshot` is reached for any
dashboard over the tiling threshold, report context or not
(`webdriver.py:793`). So for a large-dashboard thumbnail:
- a uniform tile with visible chart holders now costs up to 3 capture
attempts instead of 1;
- a persistent capture timeout now raises `ScreenshotCaptureTimeoutError`.
The second one is the substantive change. Previously that path returned
`None`, and `webdriver.py:820` converted it into `PlaywrightTimeout` — which
the comment there describes as deliberate, *"for thumbnails too, since the
caller treats the raise as a clean cache-ERROR."*
`ScreenshotCaptureTimeoutError` is a bare `RuntimeError`, so it escapes both
handlers at `webdriver.py:978-980` (`PlaywrightTimeout` re-raise,
`PlaywrightError` log-and-continue) and never reaches the `if not img:`
conversion.
Confirmed by reverting the production hunks:
`test_capture_timeout_retries_with_remaining_budget` fails with `assert None ==
b'combined'`, i.e. the old path returned `None`.
Two options: gate the raise on `report_execution_context` so thumbnails keep
returning `None`, or subclass `PlaywrightTimeout` instead of `RuntimeError` so
the existing contract holds. Either way this line needs updating — the rest of
the note matches the diff accurately.
##########
superset/utils/screenshot_utils.py:
##########
@@ -136,8 +169,18 @@ class
TiledScreenshotBudgetExceededError(ScreenshotTaskBudgetExceededError):
SLICE_CONTAINER_SELECTOR = r".slice_container"
LOADING_SELECTOR = r".loading"
ALERT_SELECTOR = r'[role="alert"]'
-EMPTY_SELECTOR = r".ant-empty"
+EMPTY_SELECTOR = r".ant-empty, .ag-overlay-no-rows-wrapper:not(.ag-hidden)"
MISSING_CHART_SELECTOR = r".missing-chart-container"
+CONTENTFUL_CHART_HOLDERS_IN_CLIP_JS = f"""clip => Array.from(
+ document.querySelectorAll('{CHART_HOLDER_SELECTOR}')
+).filter(holder => {{
+ const rect = holder.getBoundingClientRect();
+ const intersectsClip = rect.bottom > clip.top && rect.top < clip.bottom;
Review Comment:
**Any overlap counts, including 1px** — `rect.bottom > clip.top && rect.top
< clip.bottom`.
A chart holder that crosses a tile boundary by a single pixel marks the
*neighbouring* tile as contentful. If that neighbour is genuinely near-uniform
whitespace — a tall holder with a small painted region, a Big Number, a sparse
table — it is treated as a blank capture and burns the full 3 attempts plus 2
forced repaints, every run, deterministically.
That is the concrete answer to "can a healthy dashboard now time out":
nothing here is unbounded on its own (`_raise_if_budget_exhausted()` runs
between attempts), but the added cost is `2 × (screenshot + bring_to_front +
repaint)` per affected tile, and it lands on dashboards with many tiles —
exactly the ones already closest to the budget. A worst case of `3 × 120s` per
tile of capture alone is a large amount of newly reachable headroom consumption
for a dashboard that was previously capturing fine on one attempt.
Requiring a meaningful overlap would remove most of the false positives
cheaply:
```js
const overlap = Math.min(rect.bottom, clip.bottom) - Math.max(rect.top,
clip.top);
const intersectsClip = overlap > Math.min(rect.height, clip.bottom -
clip.top) * 0.1;
```
Worth logging `blank_tile_retries` at INFO rather than only folding it into
the readiness line, so this is measurable in the field before it shows up as a
budget-exceeded report.
--
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]
