Support running superset via pex #1713

yolken · 2016-11-30T00:52:22Z

Context

Pex is a packaging system and format for Python apps that combines the code for an app and its dependencies in a single file. This change includes some minor updates to superset to support running it via a pex. Everything should be backwards-compatible.

Content

Support running gunicorn without going through gunicorn command-line; this requires bypassing the cli.manager and having some wrapper code in superset/bin/superset so that the app forks cleanly in this case
Support loading a local config that isn't in the python path. In the pex case, the pythonpath is isolated and immutable, so this is the easiest way to bring in an external config.

xrmx · 2016-11-30T09:54:06Z

superset/cli.py

@@ -46,6 +46,9 @@ def runserver(debug, address, port, timeout, workers):
            threaded=True,
            debug=True)
    else:
+        print("DEPRECATED: Please run gunicorn via the 'runserver_gunicorn' command")


Do we really need the two commands? What does change if we switch to the new way of loading gunicorn?

Nothing should change from the user perspective when switching to the new command. I'm keeping the old one in there for now for backwards compatibility. At least inside Airbnb, it's hard for us to simultaneously switch both the app code and the command used to run the app (set via chef).

Once this change is in, I can come back in a few days and delete the old gunicorn command.

Let me know if this doesn't sound ok.

bkyryliuk · 2016-11-30T17:44:41Z

superset/bin/superset

+    subparsers = parser.add_subparsers()
+    gunicorn_parser = subparsers.add_parser('runserver_gunicorn')
+
+    gunicorn_parser.add_argument('-a', '--address', type=str, default='0.0.0.0')


what about making 127.0.0.1 as a default?

bkyryliuk · 2016-11-30T22:40:49Z

superset/cli.py

@@ -46,6 +46,9 @@ def runserver(debug, address, port, timeout, workers):
            threaded=True,
            debug=True)
    else:
+        print("DEPRECATED: Please run gunicorn via the 'runserver_gunicorn' command")


codeclimate complains: print("DEPRECATED: Please run gunicorn via the 'runserver_gunicorn' command")

bkyryliuk · 2016-11-30T22:41:45Z

LGTM, @mistercrunch - how does it look for you? You have way more experience with gunicorn and python in general.

mistercrunch · 2016-12-01T00:53:17Z

superset/bin/superset

+import gunicorn.app.base
+
+
+class GunicornSupersetApplication(gunicorn.app.base.BaseApplication):


the cli logic should live in cli.py as we're trying to keep this file to a bare minimum since it can't be imported easily (no .py extension). You should also use flask_script the same way that the rest of the CLI is written as opposed to good old argparse

I tried that but got lots of DB errors due to forking problems. :-( The issue is that, with the way things are set up now, you can't import superset or any of its subpackages before gunicorn forks (i.e., in the load function). The underlying cause is that sqlalchemy (initialized in superset/__init__.py) is not fork-safe.

We ran into similar issues when pex-ifying knowledge repo. The way around it is to refactor the code so that cli.py can be loaded without initializing the flask app and its associated DB connections in the gunicorn case. Looking at the code, this is doable, but would require a significant amount of refactoring.

Given these limitations, can we keep the gunicorn wrapper in the superset script for now and leave the refactoring as a future TODO? Apologies that it isn't as clean as it would be ideally.

Oh sorry I now realize I skipped reading the PR's message which explained all this. We'll need to update our chef recipe when merging this into production.

Sure! The old version of runserver will still work until we deprecate it, so nothing should break immediately.

mistercrunch · 2016-12-01T00:53:48Z

superset/bin/superset

+        return app
+
+
+def run_gunicorn():


move to cli.py, use flask_script

mistercrunch · 2016-12-01T19:35:21Z

LGTM

yolken · 2016-12-01T22:31:15Z

@bkyryliuk @mistercrunch Refactored the runserver flow a bit as we discussed earlier today. In particular, both the debug and gunicorn cases are now handled consistently, so we won't have to deprecate anything in the future. Can you take another look? Thanks!

mistercrunch · 2016-12-01T23:18:43Z

darabos · 2017-01-09T16:50:38Z

superset/bin/superset

+        '-a', '--address', type=str, default='127.0.0.1',
+        help='Specify the address to which to bind the web server')
+    gunicorn_parser.add_argument(
+        '-p', '--port', type=int, default=8088,


This pull request has changed the default port number from config.get("SUPERSET_WEBSERVER_PORT") to 8088. Effectively the SUPERSET_WEBSERVER_ADDRESS, SUPERSET_WEBSERVER_PORT, SUPERSET_WORKERS, and SUPERSET_WEBSERVER_TIMEOUT config variables have been removed. (They have no effect now.)

Is this intentional? These settings are still mentioned in the docs.

+cc @szmate1618

I hope it's not :) Care to open a PR?

I hope it's not :) Care to open a PR?

The solution is not easy it seems. The idea here appears to be to avoid importing superset before the Gunicorn application is loaded. So at the point where the flag defaults are set we cannot access the Superset config file yet.

I think it may be fine to retire these settings. Gunicorn can be configured via a config file. Superset could have a separate Gunicorn config file instead that would allow setting these web-server-specific parameters in one logical place. Plus it would give the user a chance to set many other Gunicorn settings that cannot be controlled now, such as the SSL certificate.

What do you think?

i don't know, i kinda liked the one config file to rule them all approach. Let's ping the airbnb guys @yolken @mistercrunch

BTW could we please move the discussion to #1939 ?

mistercrunch · 2017-01-11T16:13:41Z

We've found other ways of pex-ing for gunicorn to address this problem (gunicorn forking the process while sqlalchemy is not fork-safe), we'll revert most of this PR as soon as the alternative solution is up.

Support running superset via pex

a5c35a5

xrmx reviewed Nov 30, 2016

View reviewed changes

bkyryliuk reviewed Nov 30, 2016

View reviewed changes

Benjamin Yolken added 4 commits November 30, 2016 10:29

[superset] Update default port in superset/bin/superset

61f077e

Fix codeclimate line length issues

ac0a94c

Fix another line length issue, in config.py

4efc6f8

Merge branch 'master' into byolken/support_pex

19fb0af

bkyryliuk reviewed Nov 30, 2016

View reviewed changes

Benjamin Yolken added 2 commits November 30, 2016 15:06

Merge branch 'master' into byolken/support_pex

2876df9

Add trivial utils test to increase test coverage

4c57caf

mistercrunch reviewed Dec 1, 2016

View reviewed changes

mistercrunch requested changes Dec 1, 2016

View reviewed changes

superset/bin/superset

return app

def run_gunicorn():

Copy link

Member

mistercrunch Dec 1, 2016

Choose a reason for hiding this comment

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

move to cli.py, use flask_script

mistercrunch approved these changes Dec 1, 2016

View reviewed changes

Benjamin Yolken added 3 commits December 1, 2016 11:39

Merge branch 'master' into byolken/support_pex

31c66a7

Merge branch 'master' into byolken/support_pex

4eaeac4

Clean up runserver handling

b192578

mistercrunch merged commit 50da4f8 into master Dec 1, 2016

mistercrunch deleted the byolken/support_pex branch December 1, 2016 23:18

yolken restored the byolken/support_pex branch December 2, 2016 00:44

yolken deleted the byolken/support_pex branch December 2, 2016 00:44

darabos reviewed Jan 9, 2017

View reviewed changes

plumbeo mentioned this pull request Jan 10, 2017

Configuration variables in superset_config.py ignored after PR #1713 #1939

Closed

mistercrunch added a commit to mistercrunch/superset that referenced this pull request Jan 12, 2017

Reverting CLI changes in apache#1713

d1e2786

mistercrunch added a commit that referenced this pull request Jan 13, 2017

Reverting CLI changes in #1713 (#1964)

99b84d2

SalehHindi pushed a commit to SalehHindi/superset that referenced this pull request Jun 9, 2017

Reverting CLI changes in apache#1713 (apache#1964)

9666878

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support running superset via pex #1713

Support running superset via pex #1713

yolken commented Nov 30, 2016

xrmx Nov 30, 2016

yolken Nov 30, 2016

bkyryliuk Nov 30, 2016

yolken Nov 30, 2016

bkyryliuk Nov 30, 2016

bkyryliuk commented Nov 30, 2016 •

edited

Loading

mistercrunch Dec 1, 2016 •

edited

Loading

yolken Dec 1, 2016 •

edited

Loading

mistercrunch Dec 1, 2016

yolken Dec 1, 2016

mistercrunch Dec 1, 2016

mistercrunch commented Dec 1, 2016

yolken commented Dec 1, 2016

mistercrunch commented Dec 1, 2016

darabos Jan 9, 2017 •

edited

Loading

xrmx Jan 9, 2017

darabos Jan 10, 2017

xrmx Jan 11, 2017

mistercrunch commented Jan 11, 2017

		import gunicorn.app.base


		class GunicornSupersetApplication(gunicorn.app.base.BaseApplication):

Support running superset via pex #1713

Support running superset via pex #1713

Conversation

yolken commented Nov 30, 2016

Context

Content

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

bkyryliuk commented Nov 30, 2016 • edited Loading

mistercrunch Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

yolken Dec 1, 2016 • edited Loading

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

mistercrunch commented Dec 1, 2016

yolken commented Dec 1, 2016

mistercrunch commented Dec 1, 2016

darabos Jan 9, 2017 • edited Loading

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

mistercrunch commented Jan 11, 2017

bkyryliuk commented Nov 30, 2016 •

edited

Loading

mistercrunch Dec 1, 2016 •

edited

Loading

yolken Dec 1, 2016 •

edited

Loading

darabos Jan 9, 2017 •

edited

Loading