fitzee commented on code in PR #43784:
URL: https://github.com/apache/superset/pull/43784#discussion_r3920663703
##########
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:
Agreed and fixed in 138a6d351b. The repaint setup is now a synchronous
evaluate, followed by a separately bounded 5-second wait_for_function. The
whole recovery step is guarded: timeout/evaluation/context errors are logged as
report_capture_repaint_timeout and the next capture attempt proceeds. Added
test_repaint_wait_timeout_is_bounded_and_retried; the focused suite passes 68
tests.
##########
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:
Agreed and fixed in 138a6d351b. The diagnostic now returns both total and
contentful holder counts. Evaluation/shape failures fail closed for
uniform-tile retry purposes, and a report that expects charts but observes
total=0 emits report_capture_no_chart_holders rather than silently disabling
the check. Persistent uniform content is still retained after bounded retries
to avoid rejecting legitimate maps/tables. The diagnostic-failure test now
proves three retries plus warning/retention.
##########
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:
Agreed and fixed in 138a6d351b. A holder must now overlap more than 10% of
the smaller of the holder height and tile height, avoiding one-pixel boundary
false positives. Any suspicious-pixel retries also emit a dedicated INFO event,
report_capture_blank_tile_retries, in addition to the per-attempt warnings and
final readiness summary.
##########
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:
Agreed and fixed in 138a6d351b. Persistent capture timeouts remain explicit
failures for report executions so execution history keeps the reason, while
non-report tiled captures return None and preserve the existing thumbnail
cache-error contract. UPDATING.md now describes the bounded retries and the
distinct report/thumbnail terminal behavior. Added a focused thumbnail-contract
regression test.
--
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]
