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

Flask App factory PR #1 #8418

Merged
merged 48 commits into from
Nov 20, 2019
Merged

Flask App factory PR #1 #8418

merged 48 commits into from
Nov 20, 2019

Conversation

craig-rueda
Copy link
Member

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

WIP - This is the first of several PRs which addresses SIP-24

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

Thanks @craig-rueda for tackling this. I love the idea of detangling aspects of the Superset app which historically has been problematic when extending due to potential cyclical dependencies.

logger = logging.getLogger(__name__)


def create_app():
Copy link
Member

Choose a reason for hiding this comment

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

I generally like this approach as we (Airbnb) use it elsewhere. Any reason for having the SupersetAppInitializer class rather than the pattern suggested in the Flask cookie-cutter template.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, the thinking here is that an end user can simply extend the SupersetAppInitializer and override only the parts they're interested in.

Although I could just do all the work in app:create_app(), it's a bit more OO to break the task of initialization into a set of phases.

I think a good example of this need would be someone that wanted to override something specific, such as the configuration of FAB, but wanted to carry forward the rest of Superset's config. SupersetAppInitializer allows you to compose your config from reusable bits as an end user.

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if you tried to override SupersetAppInitializer in a local config, but it screams circular dependency, though maybe with all the proxying here it's not an issue anymore (!?)

module_datasource_map.update(self.config.get("ADDITIONAL_MODULE_DS_MAP"))
ConnectorRegistry.register_sources(module_datasource_map)

def configure_cache(self):
Copy link
Member

Choose a reason for hiding this comment

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

Like the cookie-cutter example one could have a method which registers all extensions.

module_datasource_map = app.config["DEFAULT_MODULE_DS_MAP"]
module_datasource_map.update(app.config["ADDITIONAL_MODULE_DS_MAP"])
ConnectorRegistry.register_sources(module_datasource_map)
app = create_app()
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this file? Having logic in __init__.py can be problematic for cyclical dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do and we don't. My goal is to clear it out completely... However, the changes required in order to facilitate this will end up being quite a large PR. If you think it's reasonable to "just do it", then I'd be happy to pull that thread.

One thing of note here is that removing app from here will REQUIRE people to register their FLASK_APP env var differently superset.app -> superset.app:create_app()



try:
# Allow user to override our config completely
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure we want to allow them to completely override the default config as there’s implicit requirements in the code that certain config variables exist.

superset/app.py Outdated
csrf.exempt(ex)

def register_blueprints(self):
for bp in self.config.get("BLUEPRINTS"):
Copy link
Member

Choose a reason for hiding this comment

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

mypy is going to barf at using config.get(...) as the return type is Optional[...]. In reference to my previous comment about the default config, config[...] should be suffice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I haven't linted yet - just getting ducks in order in terms of structure

superset/app.py Outdated
"""
pass

def init_views(self) -> None:
Copy link
Member

@john-bodley john-bodley Oct 21, 2019

Choose a reason for hiding this comment

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

Should we simply make all our views blueprints and register them accordingly? This may simplify things in the future and/or reduce the cyclical dependency issues we often run into. We (Airbnb) have found the model of using blueprints for everything has worked quite successfully and the Flask app really just glues everything together in a somewhat lightweight fashion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, definitely. I want to pull up all occurrences of things like appbuilder.add_view() to this method. Ideally, the views themselves wouldn't register themselves, but rather this init logic would compose the views we're interested in.

Again, this PR was intended to be the first step in chipping away at this pattern migration. Thoughts on going for it?? @mistercrunch ??

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure that FAB's add_view creates a blueprint for each call

Copy link
Member

@dpgaspar dpgaspar Oct 25, 2019

Choose a reason for hiding this comment

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

yap, it's ModelView, ModelRestApi class is a blueprint and it's actually created on add_view

superset/extensions.py Show resolved Hide resolved
superset/utils/manifest_processor.py Outdated Show resolved Hide resolved
superset/utils/results_backend_manager.py Outdated Show resolved Hide resolved
@mistercrunch
Copy link
Member

Read through, LGTM. This is many steps in the right direction.

setup.py Outdated
@@ -110,6 +110,7 @@ def get_git_sha():
extras_require={
"bigquery": ["pybigquery>=0.4.10", "pandas_gbq>=0.10.0"],
"cors": ["flask-cors>=2.0.0"],
"flask-testing": ["Flask-Testing==0.7.1"],
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Can we lowercase this like the other packages. Package names are case insensitive.

@codecov-io
Copy link

codecov-io commented Nov 15, 2019

Codecov Report

Merging #8418 into master will increase coverage by 0.07%.
The diff coverage is 83.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8418      +/-   ##
==========================================
+ Coverage   65.69%   65.77%   +0.07%     
==========================================
  Files         474      477       +3     
  Lines       23583    23726     +143     
  Branches     2571     2571              
==========================================
+ Hits        15494    15606     +112     
- Misses       7920     7951      +31     
  Partials      169      169
Impacted Files Coverage Δ
superset/utils/core.py 88.02% <ø> (-0.79%) ⬇️
superset/forms.py 95% <ø> (-0.46%) ⬇️
superset/tasks/celery_app.py 0% <0%> (-100%) ⬇️
superset/utils/cache_manager.py 100% <100%> (ø)
superset/tasks/schedules.py 79.89% <100%> (ø) ⬆️
superset/__init__.py 100% <100%> (+24.03%) ⬆️
superset/sql_lab.py 77.93% <100%> (ø) ⬆️
superset/tasks/cache.py 73.68% <100%> (ø) ⬆️
superset/utils/cache.py 46.15% <71.42%> (+4.48%) ⬆️
superset/cli.py 37.19% <77.55%> (+3.09%) ⬆️
... and 12 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 3b97ae3...d2f4ada. Read the comment docs.

@craig-rueda craig-rueda marked this pull request as ready for review November 15, 2019 18:14
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just a few comments.

manifest_processor,
results_backend_manager,
security_manager as ext_security_manager,
talisman as ext_talisman,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason these need to be names ext_?

Copy link
Member Author

Choose a reason for hiding this comment

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

For clarity at this point. I suppose I could have just imported extensions and then referenced each item upon assignment.

Copy link
Member

Choose a reason for hiding this comment

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

@craig-rueda what I meant was you import talisman as ext_talisman and then on line #59 there is talisman = ext_talisman and thus why not just,

from superset.extensions import talisman

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't know that was valid. Chaining imports :). I'm still a bit of a Python noob...


if __name__ == '__main__':
cli()
superset()
Copy link
Member

Choose a reason for hiding this comment

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

I think we could annex this file and have setup.py reference superset.cli:superset.

superset/utils/manifest_processor.py Outdated Show resolved Hide resolved
tests/access_tests.py Show resolved Hide resolved
appbuilder.indexview = SupersetIndexView
appbuilder.base_template = "superset/base.html"
appbuilder.security_manager_class = custom_sm
appbuilder.update_perms = False
Copy link
Member

Choose a reason for hiding this comment

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

Let FAB handle these:

  • FAB_UPDATE_PERMS defaults to True in FAB, setting superset's default to True could be done on config.py
  • FAB_INDEX_VIEW
  • FAB_BASE_TEMPLATE

We also have FAB_SECURITY_MANAGER_CLASS

Copy link
Member Author

Choose a reason for hiding this comment

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

This was in master - would prefer to tune configs in a subsequent PR

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think we need to force-set those. Superset is force-pinning FAB configurations as it operates on these assumptions.

superset/app.py Outdated Show resolved Hide resolved
superset/app.py Outdated Show resolved Hide resolved
superset/cli.py Outdated Show resolved Hide resolved

APP_DIR = os.path.dirname(__file__)

appbuilder = AppBuilder(update_perms=False)
Copy link
Member

Choose a reason for hiding this comment

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

Should we force it? set FAB_UPDATE_PERMS to False on config

Copy link
Member Author

Choose a reason for hiding this comment

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

This was in master - would prefer to tune configs in a subsequent PR

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to force it. Docs are clear about running superset init which takes care of that, and we have many other CLI commands and things where we don't want to operate against the db, create delays and flood the logs with unrelated operations.

setup.py Outdated Show resolved Hide resolved
superset/app.py Outdated Show resolved Hide resolved
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

This LGTM personally. It's a bit risky, but well overdue and a great step forward.

Let's push this forward if other committers are ok with it.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

LGTM amazing refactor!

@dpgaspar dpgaspar merged commit e490414 into apache:master Nov 20, 2019
@dpgaspar dpgaspar deleted the app_factory branch November 20, 2019 15:47
@john-bodley
Copy link
Member

@craig-rueda we ran into an issue with Celery using the configuration defined here related to running outside of an app context and need to use,

celery worker --app=superset.tasks.celery_app.app

instead. I was wondering whether you could update UPDATING.md and possibly CONTRIBUTING.md to reflect the change.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 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 size/XL 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants