Skip to content
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.

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

[cli] Preparing to deprecate wrapping of external commands #4451

Merged
merged 2 commits into from Mar 30, 2018

Conversation

john-bodley
Copy link
Member

This PR indicates to users that the Superset CLI commands which wrap external commands such as gunicorn, celery, and flower are deprecated prior to removing the functionality at a later stage.

The reason for the change is:

  1. All these commands can be run via the delegated application.
  2. The current approach only exposes a limited number of command line arguments.
  3. It provides unnecessary dependencies which aren't core to the application.

to: @fabianmenges @mistercrunch

@xrmx
Copy link
Contributor

xrmx commented Feb 18, 2018

This looks like painful for newcomers coming from outside the python world who have no clue on how to deploy a wsgi app though. The fact that superset looked like an http service while it's just a flask application it's handy to me, also quite "cloud native" :) Would it be possible to keep the dependencies and just remove the wrappers? Anyway if we remove the wrapper we have to provide the equivalent of superset runserver in the docs if we don't want to get swamped with github issues please.

@john-bodley john-bodley changed the title [cli] Deprecating wrapping of external commands [cli] Preparing to deprecate wrapping of external commands Feb 18, 2018
@mistercrunch
Copy link
Member

Having some docker / docker-compose predefined setup(s) could help compensate in a better-than-its-now kind of way.

Personally cannot commit time to this but I think that'd be the modern alternative for good and easy setup.

@john-bodley john-bodley force-pushed the john-bodley-doc-cli-changes branch 2 times, most recently from fddf5b9 to 59179fb Compare February 20, 2018 19:17
@codecov-io
Copy link

codecov-io commented Mar 15, 2018

Codecov Report

Merging #4451 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4451      +/-   ##
==========================================
- Coverage   71.88%   71.85%   -0.04%     
==========================================
  Files         204      204              
  Lines       15329    15317      -12     
  Branches     1177     1177              
==========================================
- Hits        11020    11006      -14     
- Misses       4306     4308       +2     
  Partials        3        3
Impacted Files Coverage Δ
superset/cli.py 46.83% <0%> (-0.91%) ⬇️
superset/config.py 92.24% <100%> (ø) ⬆️
superset/utils.py 88.05% <0%> (-0.17%) ⬇️
superset/connectors/sqla/views.py 71.56% <0%> (ø) ⬆️
superset/connectors/druid/views.py 68.02% <0%> (ø) ⬆️

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 d49a0e7...625efa5. Read the comment docs.

@@ -521,6 +513,10 @@ have the same configuration.

CELERY_CONFIG = CeleryConfig

To start a Celery worker to leverage the configuration run: ::

celery worker --app=superset.sql_lab:celery_app -Ofair
Copy link
Contributor

Choose a reason for hiding this comment

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

For celery the gevent flag is -P gevent

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I added --pool=gevent. I've used the long names as they're more descriptive.

so you'll want to craft your own `gunicorn` command in your production
environment. Here's an **async** setup known to work well: ::
application in a way that works well in your environment. Here's an **async**
setup known to work well in production: ::

 gunicorn \
Copy link
Contributor

Choose a reason for hiding this comment

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

We are running gunicorn with gevent which make superset more efficient handling requests -k gevent

Copy link
Member Author

Choose a reason for hiding this comment

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

@fabianmenges the -k gevent argument is specified.

@john-bodley
Copy link
Member Author

@fabianmenges are you ok with the changes I made per your request?

Copy link
Contributor

@jeffreythewang jeffreythewang left a comment

Choose a reason for hiding this comment

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

I do somewhat agree with @xrmx that it may be painful for newcomers, at least for the gunicorn wrapper. I still (and will probably still) use the superset cli wrapper as a shortcut for quickly spinning up an app locally.

superset runserver

# To start a development web server, use the -d switch
# To start a development web server on port 8088, use -p to bind to anothe port
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo

SUPERSET_WORKERS = 2
SUPERSET_CELERY_WORKERS = 32
SUPERSET_WORKERS = 2 # deprecated
SUPERSET_CELERY_WORKERS = 32 # deprecated

SUPERSET_WEBSERVER_ADDRESS = '0.0.0.0'
SUPERSET_WEBSERVER_PORT = 8088
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: would these configs (and the rest of the config vars that are only used in cli.py also be tagged as # deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeffreythewang I added SUPERSET_WEBSERVER_TIMEOUT. Am I missing other config variables from the commands which are slated to be deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response. I primarily meant it as adding a # deprecated and [DEPRECATED] comment, both in cli.py and the sample config.py, to the environment variables that are deprecated. I might be wrong, but I only see SUPERSET_WEBSERVER_PORT, SUPERSET_WEBSERVER_ADDRESS, and FLASK_USE_RELOAD used in cli.py.

@john-bodley john-bodley force-pushed the john-bodley-doc-cli-changes branch 2 times, most recently from 2052f3e to 5179d10 Compare March 21, 2018 04:49
@john-bodley john-bodley merged commit b3442a7 into apache:master Mar 30, 2018
@john-bodley john-bodley deleted the john-bodley-doc-cli-changes branch March 30, 2018 16:28
john-bodley added a commit to john-bodley/superset that referenced this pull request Apr 10, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants