Skip to content

[WiP] Thumbnails! #6601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mistercrunch wants to merge 2 commits into from
Closed

[WiP] Thumbnails! #6601

mistercrunch wants to merge 2 commits into from

Conversation

@mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jan 6, 2019 •
edited
Loading

screen shot 2019-01-24 at 10 58 20 pm

todo

  • test emails
  • split the caching configuration
  • cli command to prime thumbnails
  • put behind feature flag(s)
  • tests
  • test different drivers

@mistercrunch mistercrunch force-pushed the selenium branch 11 times, most recently from 2e1c514 to 7a6edf8 Compare January 7, 2019 05:48
@codecov-io
Copy link

codecov-io commented Jan 7, 2019 •
edited
Loading

Codecov Report

Merging #6601 into master will decrease coverage by 8.72%.
The diff coverage is 82.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6601      +/-   ##
==========================================
- Coverage   64.89%   56.16%   -8.73%     
==========================================
  Files         428      525      +97     
  Lines       20934    23219    +2285     
  Branches     2330     2772     +442     
==========================================
- Hits        13585    13042     -543     
- Misses       7225     9767    +2542     
- Partials      124      410     +286
Impacted Files Coverage Δ
superset/config.py 93.46% <ø> (-0.33%) ⬇️
superset/sql_lab.py 76.75% <100%> (+0.38%) ⬆️
superset/viz.py 71.89% <100%> (+0.35%) ⬆️
superset/views/schedules.py 59.45% <100%> (+0.36%) ⬆️
superset/views/base.py 69.27% <100%> (+0.16%) ⬆️
superset/views/__init__.py 100% <100%> (ø) ⬆️
superset/views/thumbnails.py 42.85% <42.85%> (ø)
superset/models/core.py 83.72% <50%> (+0.16%) ⬆️
superset/utils/json.py 82% <82%> (ø)
superset/views/core.py 75.05% <83.33%> (+2.15%) ⬆️
... and 348 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5db20e3...fac70c0. Read the comment docs.

@mistercrunch
Copy link
Member Author

mistercrunch commented Jan 10, 2019

@mahendra any chance you can test this in your environment?

@mahendra
Copy link

mahendra commented Jan 11, 2019 via email

mahendra
mahendra suggested changes Jan 14, 2019
View reviewed changes
superset/utils/selenium.py Outdated
Copy link

@mahendra mahendra Jan 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may need this at 30. This is because the JS based rendering takes a while to finish.

Copy link
Member Author

@mistercrunch mistercrunch Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I meant to check in while-type loop here..

Copy link
Member Author

@mistercrunch mistercrunch Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I switch to checking 30 times each 2 seconds instead of a few times every 30 seconds...

@mahendra
Copy link

mahendra commented Jan 14, 2019

@mistercrunch I got this error while testing the feature. (not had time to debug yet).

[2019-01-14 09:05:07,665: ERROR/ForkPoolWorker-1] Task email_reports.send[7070358d-d91f-4b11-b23c-6cc9cd46a937] raised unexpected: InvalidCookieDomainException('Document is cookie-averse', None, None)
Traceback (most recent call last):
  File "/Users/mahendram/work/incubator-superset/.venv/lib/python3.6/site-packages/celery/app/trace.py", line 382, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/Users/mahendram/work/incubator-superset/.venv/lib/python3.6/site-packages/celery/app/trace.py", line 641, in __protected_call__
    return self.run(*args, **kwargs)
  File "/Users/mahendram/work/incubator-superset/superset/tasks/schedules.py", line 252, in schedule_email_report
    deliver_dashboard(schedule)
  File "/Users/mahendram/work/incubator-superset/superset/tasks/schedules.py", line 118, in deliver_dashboard
    url, window, ['grid-container'], user, config.get('EMAIL_REPORTS_WEBDRIVER'))
  File "/Users/mahendram/work/incubator-superset/superset/utils/selenium.py", line 78, in get_png_from_url
    driver = create_webdriver(user, webdriver)
  File "/Users/mahendram/work/incubator-superset/superset/utils/selenium.py", line 72, in create_webdriver
    driver.add_cookie(info)
  File "/Users/mahendram/work/incubator-superset/.venv/lib/python3.6/site-packages/selenium/webdriver/remote/webdriver.py", line 894, in add_cookie
    self.execute(Command.ADD_COOKIE, {'cookie': cookie_dict})
  File "/Users/mahendram/work/incubator-superset/.venv/lib/python3.6/site-packages/selenium/webdriver/remote/webdriver.py", line 321, in execute
    self.error_handler.check_response(response)
  File "/Users/mahendram/work/incubator-superset/.venv/lib/python3.6/site-packages/selenium/webdriver/remote/errorhandler.py", line 242, in check_response
    raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.InvalidCookieDomainException: Message: Document is cookie-averse

bolkedebruin
bolkedebruin suggested changes Jan 15, 2019
View reviewed changes
@mistercrunch mistercrunch force-pushed the selenium branch 3 times, most recently from 8a0465d to 847facc Compare January 18, 2019 06:41
@mistercrunch mistercrunch changed the title Refactor selenium-related methods under utils/selenium.py [WiP] Thumbnails! Jan 25, 2019
@mistercrunch mistercrunch force-pushed the selenium branch 3 times, most recently from 487cc84 to d8c3865 Compare January 26, 2019 18:15
@mistercrunch mistercrunch mentioned this pull request Jun 17, 2019
Closed
6 tasks
mistercrunch added a commit to mistercrunch/superset that referenced this pull request Jun 17, 2019
@mistercrunch
Backend for thumnail computation + caching + endpoints
cd31db6 
This PR is the backend subset of WiP PR apache#6601. It includes

* refactor of the Selenium abstractions used for email schedules,
  generalized to also work with thumbs
* new dependency on PIL, the common way of doing image processing in
  python, we use it to resize selenium screenshots into thumbs
* CLI utilities to compute-thumbnails
* the addition of an extra caching backend for thumbnail, in most cases
  we assume it should point to the same backend as the one used for
  chart JSON caching
* 2 new endpoints to retrieve dashboard and chart thumbs
@stale
Copy link

stale bot commented Jul 31, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jul 31, 2019
@stale stale bot closed this Aug 7, 2019
mistercrunch added a commit to mistercrunch/superset that referenced this pull request Aug 9, 2019
@mistercrunch
Backend for thumnail computation + caching + endpoints
cef4673 
This PR is the backend subset of WiP PR apache#6601. It includes

* refactor of the Selenium abstractions used for email schedules,
  generalized to also work with thumbs
* new dependency on PIL, the common way of doing image processing in
  python, we use it to resize selenium screenshots into thumbs
* CLI utilities to compute-thumbnails
* the addition of an extra caching backend for thumbnail, in most cases
  we assume it should point to the same backend as the one used for
  chart JSON caching
* 2 new endpoints to retrieve dashboard and chart thumbs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@mahendra mahendra mahendra requested changes

@bolkedebruin bolkedebruin bolkedebruin requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

inactive Inactive for >= 30 days

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mistercrunch @codecov-io @mahendra @bolkedebruin @kristw