feat: Implement using Playwright for taking screenshots in reports #25247

kgabryje · 2023-09-10T11:50:16Z

SUMMARY

Implements SIP-98.

If a feature flag PLAYWRIGHT_REPORTS_AND_THUMBNAILS is on, use Playwright instead of Selenium to take screenshots of dashboards and charts when preparing reports and thumbnails.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

The change should be visible when running a report on a dashboard that contains deck.gl charts.
Before:

After:

TESTING INSTRUCTIONS

Enable PLAYWRIGHT_REPORTS_AND_THUMBNAILS ff
Setup reports (https://superset.apache.org/docs/installation/alerts-reports/)
Create a report and verify that the screenshot was taken correctly. If possible, also verify using deck.gl charts
Generate thumbnails (https://superset.apache.org/docs/installation/cache/#caching-thumbnails)
Verify that thumbnails screenshots were taken correctly (including deck.gl charts)
If the feature flag is disabled, the screenshots should be taken using Selenium - no changes.

ADDITIONAL INFORMATION

Has associated issue: [SIP-98] Proposal for Adding Playwright as an Alternative for Selenium to Enable deck.gl Charts in Reports #24948
Required feature flags: PLAYWRIGHT_REPORTS_AND_THUMBNAILS
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

villebro

Exciting stuff. First pass, comments mainly related to whether or not we should add the deps as required vs optional. I also have some unrelated grievances with how the current Allerts & Reports feature handles errors, but let's chat about those separately (=this is a good opportunity to think about improving this as we're refactoring the code that detects whether or not the page has rendered).

villebro · 2023-09-10T16:18:47Z

requirements/base.txt

+playwright==1.37.0
+    # via apache-superset


I think this should probably go into the dev, integration or test deps

Could you explain why? Isn't reports one of the basic flows? Or maybe I misunderstand the purpose of those files

The fact that it's marked as an optional dependency in setup.py IMO means it should not be in base.txt. Was this entry added by hand, or via pip-compile-multi? I assume we'd need to change requirements/base.in as follows to autogenerate this:

-e file:.[playwright]

villebro · 2023-09-10T16:19:41Z

setup.py

@@ -108,6 +108,7 @@ def get_git_sha() -> str:
        "pandas[performance]>=2.0.3, <2.1",
        "parsedatetime",
        "pgsanity",
+        "playwright",


Should we move this into an optional dep until selenium is deprecated?

Where are the optional deps located?

That's a good idea. @kgabryje in extras_require below.

Thanks, moved

eschutho · 2023-09-11T18:30:16Z

docker/docker-bootstrap.sh

@@ -31,6 +31,8 @@ fi
 if [ -f "${REQUIREMENTS_LOCAL}" ]; then
  echo "Installing local overrides at ${REQUIREMENTS_LOCAL}"
  pip install --no-cache-dir -r "${REQUIREMENTS_LOCAL}"
+  playwright install-deps
+  playwright install chromium


What's the correlation between the local overwrite config and installing playwright?

There isn't, it's a leftover from my tests in POC phase 🤦 updated

@eschutho Actually, now that we moved playwright to optional packages, I think we should make installing additional deps conditional. Do we have an established pattern for that?

superset/config.py

eschutho · 2023-09-13T21:45:55Z

superset/utils/machine_auth.py

@@ -23,6 +23,7 @@

 from flask import current_app, Flask, request, Response, session
 from flask_login import login_user
+from playwright.sync_api import BrowserContext


you can move this import down to the authenticate_playwright_browser_context method if you get errors on it not loading when the feature flag is not on.

I put the import behind a feature flag check - I couldn't move it to the method as it was used for typing that method

Scratch that, importing feature_flag_manager causes a circular import

eschutho · 2023-09-13T21:53:46Z

superset/utils/machine_auth.py

+        elif request.cookies:
+            cookies = request.cookies
+        else:
+            cookies = {}


should we try to dry this up from the other authenticate method?

kgabryje · 2023-09-15T12:56:27Z

/testenv up FEATURE_PLAYWRIGHT_REPORTS_AND_THUMBNAILS=true

github-actions · 2023-09-15T12:58:37Z

@kgabryje Ephemeral environment spinning up at http://18.236.98.170:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro

I think this is good to go after we decide on how to handle this new dependency.

villebro · 2023-10-02T19:43:12Z

requirements/base.txt

+playwright==1.37.0
+    # via apache-superset


The fact that it's marked as an optional dependency in setup.py IMO means it should not be in base.txt. Was this entry added by hand, or via pip-compile-multi? I assume we'd need to change requirements/base.in as follows to autogenerate this:

-e file:.[playwright]

setup.py

superset/utils/webdriver.py

villebro

I think we should be consistent in either having separate func overrides in the constructor if the authenticate methods are separate, or merge them so as to only support one screenshot mechanism.

docker/docker-bootstrap.sh

superset/utils/machine_auth.py

villebro

LGTM, thanks for all the hard work here! ❤️

eschutho

Looks great @kgabryje!

github-actions · 2023-10-04T04:52:29Z

Ephemeral environment shutdown and build artifacts deleted.

kgabryje · 2023-10-04T13:33:31Z

Thank you @villebro @eschutho for your comments and help to get this PR merged!

eschutho · 2023-10-04T22:44:46Z

@kgabryje I left a comment on a part that may break existing functionality. I'd suggest let's see if we can quickly fix forward.

…ports (apache#25247)" This reverts commit ff95d0f.

…pache#25247)

kgabryje requested review from betodealmeida, john-bodley, eschutho and villebro September 10, 2023 11:50

pull-request-size bot added the size/L label Sep 10, 2023

kgabryje mentioned this pull request Sep 10, 2023

[SIP-98] Proposal for Adding Playwright as an Alternative for Selenium to Enable deck.gl Charts in Reports #24948

Closed

villebro reviewed Sep 10, 2023

View reviewed changes

eschutho reviewed Sep 11, 2023

View reviewed changes

eschutho reviewed Sep 13, 2023

View reviewed changes

superset/config.py Show resolved Hide resolved

eschutho reviewed Sep 13, 2023

View reviewed changes

kgabryje force-pushed the feat/playwright-reports branch from beb24bf to bd9b693 Compare September 25, 2023 15:40

villebro reviewed Oct 2, 2023

View reviewed changes

kgabryje force-pushed the feat/playwright-reports branch from bd9b693 to d99cd13 Compare October 2, 2023 20:22

eschutho reviewed Oct 2, 2023

View reviewed changes

superset/utils/webdriver.py Show resolved Hide resolved

villebro reviewed Oct 3, 2023

View reviewed changes

docker/docker-bootstrap.sh Show resolved Hide resolved

superset/utils/machine_auth.py Outdated Show resolved Hide resolved

kgabryje added 12 commits October 3, 2023 10:22

Implement Playwright screenshots

377c747

Cleanup leftovers

582b6e2

Fix tests

df217a3

Fix tests

0ffbdef

Update docker-bootstrap

9d7423d

Move playwright package to optional deps

a331461

Fix import

7437640

Fix import

3afa4c4

Fix tests

cf08779

Install playwright deps only if playwright is installed

0655b5d

Update dependencies

33b543a

Remove saving the image locally

30f865f

kgabryje added 2 commits October 3, 2023 10:22

Re-add deleted dep

c195c18

Separate webdriver and browsercontext auth overrides

a39b84d

kgabryje force-pushed the feat/playwright-reports branch from 2ca0b98 to c0dcb2e Compare October 3, 2023 08:39

Support custom pixel density

e5e18dd

kgabryje force-pushed the feat/playwright-reports branch from c0dcb2e to e5e18dd Compare October 3, 2023 08:48

villebro reviewed Oct 3, 2023

View reviewed changes

superset/utils/machine_auth.py Show resolved Hide resolved

villebro reviewed Oct 3, 2023

View reviewed changes

superset/utils/machine_auth.py Outdated Show resolved Hide resolved

Un-hardcode auth cookie domain

02a0c8f

villebro approved these changes Oct 3, 2023

View reviewed changes

eschutho approved these changes Oct 3, 2023

View reviewed changes

kgabryje merged commit ff95d0f into apache:master Oct 4, 2023
29 checks passed

eschutho added the risk:breaking-change Issues or PRs that will introduce breaking changes label Oct 4, 2023

kgabryje mentioned this pull request Oct 5, 2023

fix: Breaking change in MachineAuthProvider constructor #25532

Merged

9 tasks

sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 6, 2023

Revert "feat: Implement using Playwright for taking screenshots in re…

a90ef94

…ports (apache#25247)" This reverts commit ff95d0f.

jinghua-qa added a commit to preset-io/superset that referenced this pull request Oct 10, 2023

Revert "feat: Implement using Playwright for taking screenshots in re…

107013d

…ports (apache#25247)" This reverts commit ff95d0f.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

feat: Implement using Playwright for taking screenshots in reports (a…

fd3d46c

…pache#25247)

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement using Playwright for taking screenshots in reports #25247

feat: Implement using Playwright for taking screenshots in reports #25247

kgabryje commented Sep 10, 2023 •

edited

Loading

villebro left a comment

villebro Sep 10, 2023

kgabryje Sep 13, 2023

villebro Oct 2, 2023

villebro Sep 10, 2023

kgabryje Sep 13, 2023

eschutho Sep 13, 2023

kgabryje Sep 14, 2023

eschutho Sep 11, 2023

kgabryje Sep 13, 2023

kgabryje Sep 15, 2023

eschutho Sep 13, 2023

kgabryje Sep 14, 2023

kgabryje Sep 14, 2023

eschutho Sep 13, 2023

kgabryje commented Sep 15, 2023

github-actions bot commented Sep 15, 2023

villebro left a comment

villebro Oct 2, 2023

villebro left a comment

villebro left a comment

eschutho left a comment

github-actions bot commented Oct 4, 2023

kgabryje commented Oct 4, 2023 •

edited

Loading

eschutho commented Oct 4, 2023 •

edited

Loading

feat: Implement using Playwright for taking screenshots in reports #25247

feat: Implement using Playwright for taking screenshots in reports #25247

Conversation

kgabryje commented Sep 10, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Sep 15, 2023

github-actions bot commented Sep 15, 2023

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2023

kgabryje commented Oct 4, 2023 • edited Loading

eschutho commented Oct 4, 2023 • edited Loading

kgabryje commented Sep 10, 2023 •

edited

Loading

kgabryje commented Oct 4, 2023 •

edited

Loading

eschutho commented Oct 4, 2023 •

edited

Loading