Use `config[]` not `config.get()` #8454

willbarrett · 2019-10-25T21:21:03Z

SUMMARY

As pointed out here https://github.com/apache/incubator-superset/pull/8409/files#diff-6a3371457528722a734f3c51d9238c13R482 we no longer want to use the config.get format, but instead assume that all configuration options are set. This PR alters all existing usages of config.get to the config[] syntax. Note that this will cause errors if those configuration options are not provided. In order to get the system running again, I had to set the following configuration options locally:

# Stuff that needs defaulting
ENABLE_CHUNK_ENCODING = False
FEATURE_FLAGS = {}
EVENT_LOGGER = None
SQLALCHEMY_CUSTOM_PASSWORD_STORE = None
QUERY_LOGGER = None
FAB_ADD_SECURITY_VIEWS = True
SUPERSET_LOG_VIEW = True
HTTP_HEADERS = {}

I'm somewhat concerned about the effect of merging this in terms of stability, so I'm looking for feedback from the community, especially @villebro who clued me in to the preferred approach in the first place.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Run CI
Click around locally
Request feedback from the community

ADDITIONAL INFORMATION

REVIEWERS

@villebro @mistercrunch @dpgaspar

villebro

@john-bodley can probably chime in here, too, but I think in general there are lots of good changes here, i.e. removing default settings that are scattered across the codebase that should be defined only in config.py. Also the fact that you were able to find eight (!) missing config parameters by doing this change goes to show that this change makes the codebase more robust. Some small changes proposed, but apart from that LGTM.

CONTRIBUTING.md

superset/__init__.py

villebro · 2019-10-26T09:46:02Z

superset/common/query_object.py

+        relative_start: str = app.config["DEFAULT_RELATIVE_START_TIME"],
+        relative_end: str = app.config["DEFAULT_RELATIVE_END_TIME"],


This is a good example of why this change is good; no point in having secondary default values spread across the codebase, as these should only be defaulted in one place.

villebro · 2019-10-26T09:48:00Z

superset/sql_lab.py

-stats_logger = config.get("STATS_LOGGER")
-SQLLAB_TIMEOUT = config.get("SQLLAB_ASYNC_TIME_LIMIT_SEC", 600)
+stats_logger = config["STATS_LOGGER"]
+SQLLAB_TIMEOUT = config["SQLLAB_ASYNC_TIME_LIMIT_SEC"]
 SQLLAB_HARD_TIMEOUT = SQLLAB_TIMEOUT + 60


Out of scope, but I wonder if SQLLAB_HARD_TIMEOUT should be defined in config.py, too?

I don't think sql lab hard timeout needs to be definable in the config as well, the purpose for it is to provide a hard cutoff in celery if a soft timeout fails (or isn't implemented). The time between soft and hard timeout shouldn't be much, so I don't think allowing configuration gets much here. Happy to reconsider if you think there's a good reason to make it configurable though!

Not feeling especially opinionated on this one, but in general I have an aversion to hard coding parameters, especially in the context of distributed/async problems which are extra horrible to debug. So, at the risk of cluttering the config file, I would still vote for introducing the config parameter SQLLAB_HARD_TIMEOUT_ADDON or similar with a concise comment that might save someone down the line a few hours of grief.

But: this ended up sounding much more dramatic than it probably is, so happy either way!

superset/sql_lab.py

superset/config.py

john-bodley

Thanks for tackling this. You mentioned there were a number of variables you had to set locally. Any reason they haven’t been added to config.py?

superset/config.py

etr2460 · 2019-10-27T05:32:09Z

+1 to @john-bodley's point, if switching from get to [] causes errors because the variables weren't defined in the config, then I think we should define defaults in the config.py file.

Also, in terms of typing, once we move to python 3.8, I think defining a Config type as a TypedDict (https://www.python.org/dev/peps/pep-0589/) should be really helpful here, as it'll catch any config vars not set.

I think this is an awesome change overall though, thanks for taking it on!

willbarrett · 2019-10-28T17:22:30Z

Thank you @villebro, @john-bodley, and @etr2460 for the feedback. The only reason the additional configuration wasn't added to config.py was that I'm new to the repo and wasn't aware of the correct place to put it :). Your guidance was helpful. You should find the missing configuration options added to config.py.

…/incubator-superset into wbarrett/config-not-config-get

mistercrunch · 2019-10-29T00:41:57Z

It'd be nice if we could raise a PresetConfigError instead of a KeyError if the key is missing, but that may be tricky since Flask manages the config class.

willbarrett · 2019-10-29T18:39:06Z

@mistercrunch yes, throwing a custom configuration error would be a nice improvement. I'd like to attempt that in a separate PR once this guy goes through - it may take a little doing to separate out Superset config from Flask config or extend Flask's config object to provide that functionality.

codecov-io · 2019-10-29T18:41:05Z

Codecov Report

Merging #8454 into master will decrease coverage by 0.01%.
The diff coverage is 82.41%.

@@            Coverage Diff             @@
##           master    #8454      +/-   ##
==========================================
- Coverage   66.58%   66.57%   -0.02%     
==========================================
  Files         449      449              
  Lines       22525    22576      +51     
  Branches     2364     2367       +3     
==========================================
+ Hits        14999    15030      +31     
- Misses       7388     7408      +20     
  Partials      138      138

Impacted Files	Coverage Δ
superset/examples/random_time_series.py	`20% <ø> (ø)`	⬆️
superset/views/log/views.py	`100% <ø> (ø)`	⬆️
superset/examples/unicode_test_data.py	`100% <ø> (ø)`	⬆️
superset/views/log/api.py	`100% <ø> (ø)`	⬆️
superset/examples/multiformat_time_series.py	`15.38% <ø> (ø)`	⬆️
superset/examples/world_bank.py	`100% <ø> (ø)`	⬆️
superset/common/query_object.py	`50% <ø> (ø)`	⬆️
superset/examples/birth_names.py	`100% <ø> (ø)`	⬆️
superset/db_engine_specs/presto.py	`22.43% <0%> (ø)`	⬆️
superset/migrations/env.py	`0% <0%> (ø)`	⬆️
... and 28 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 a757b43...4a71552. Read the comment docs.

willbarrett · 2019-10-29T20:00:08Z

Hey there @villebro @etr2460 @john-bodley - I think this is ready for another look when you have time. Thanks!

villebro

LGTM apart from a few non-blocking comments.

villebro · 2019-10-29T21:19:46Z

superset/sql_lab.py

-stats_logger = config.get("STATS_LOGGER")
-SQLLAB_TIMEOUT = config.get("SQLLAB_ASYNC_TIME_LIMIT_SEC", 600)
+stats_logger = config["STATS_LOGGER"]
+SQLLAB_TIMEOUT = config["SQLLAB_ASYNC_TIME_LIMIT_SEC"]
 SQLLAB_HARD_TIMEOUT = SQLLAB_TIMEOUT + 60


Not feeling especially opinionated on this one, but in general I have an aversion to hard coding parameters, especially in the context of distributed/async problems which are extra horrible to debug. So, at the risk of cluttering the config file, I would still vote for introducing the config parameter SQLLAB_HARD_TIMEOUT_ADDON or similar with a concise comment that might save someone down the line a few hours of grief.

But: this ended up sounding much more dramatic than it probably is, so happy either way!

superset/views/core.py

tests/celery_tests.py

willbarrett · 2019-10-30T21:43:47Z

Hi @john-bodley - I think you have the remaining review here. Please forgive the reminder :)

* Typo fix in CONTRIBUTING.md * Alter references to config.get('FOO') to use preferred config['FOO'] * Set missing configuration constants in superset/config.py * Misc. CI fixes * Add type annotation for FEATURE_FLATGS

Will Barrett added 2 commits October 25, 2019 10:19

Typo fix in CONTRIBUTING.md

40bf18a

Alter references to config.get('FOO') to use preferred config['FOO']

8a9cdba

pull-request-size bot added the size/L label Oct 25, 2019

willbarrett changed the title ~~Wbarrett/config not config get~~ Use config[] not config.get() Oct 25, 2019

villebro requested changes Oct 26, 2019

View reviewed changes

john-bodley reviewed Oct 26, 2019

View reviewed changes

superset/config.py Outdated Show resolved Hide resolved

willbarrett requested review from villebro, john-bodley and etr2460 October 28, 2019 17:22

Set missing configuration constants in superset/config.py

a4b91f0

willbarrett force-pushed the wbarrett/config-not-config-get branch from 09072af to a4b91f0 Compare October 28, 2019 17:23

Will Barrett added 3 commits October 28, 2019 10:25

Merge branch 'master' into wbarrett/config-not-config-get

ff97e07

Misc. CI fixes

0b19330

Merge branch 'wbarrett/config-not-config-get' of github.com:preset-io…

ba0bee6

…/incubator-superset into wbarrett/config-not-config-get

Add type annotation for FEATURE_FLATGS

4a71552

villebro approved these changes Oct 29, 2019

View reviewed changes

etr2460 approved these changes Oct 30, 2019

View reviewed changes

john-bodley approved these changes Oct 30, 2019

View reviewed changes

mistercrunch merged commit e4ca44e into apache:master Oct 30, 2019

willbarrett deleted the wbarrett/config-not-config-get branch November 6, 2019 16:10

dpgaspar added the preset label Nov 13, 2019

mistercrunch added the preset-io label Nov 26, 2019

mistercrunch removed the preset label Nov 26, 2019

dpgaspar added v0.35 and removed v0.35 labels Dec 20, 2019

etr2460 mentioned this pull request Apr 28, 2020

[fix] Support APP_ICON_WIDTH configuration parameter in SPA menu #9666

Merged

12 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `config[]` not `config.get()` #8454

Use `config[]` not `config.get()` #8454

willbarrett commented Oct 25, 2019

villebro left a comment

villebro Oct 26, 2019

villebro Oct 26, 2019

etr2460 Oct 27, 2019

villebro Oct 29, 2019 •

edited

Loading

john-bodley left a comment

etr2460 commented Oct 27, 2019

willbarrett commented Oct 28, 2019

mistercrunch commented Oct 29, 2019 •

edited

Loading

willbarrett commented Oct 29, 2019

codecov-io commented Oct 29, 2019 •

edited

Loading

willbarrett commented Oct 29, 2019

villebro left a comment

villebro Oct 29, 2019 •

edited

Loading

willbarrett commented Oct 30, 2019

		relative_start: str = app.config["DEFAULT_RELATIVE_START_TIME"],
		relative_end: str = app.config["DEFAULT_RELATIVE_END_TIME"],

Use config[] not config.get() #8454

Use config[] not config.get() #8454

Conversation

willbarrett commented Oct 25, 2019

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

villebro left a comment

Choose a reason for hiding this comment

villebro Oct 26, 2019

Choose a reason for hiding this comment

villebro Oct 26, 2019

Choose a reason for hiding this comment

etr2460 Oct 27, 2019

Choose a reason for hiding this comment

villebro Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

john-bodley left a comment

Choose a reason for hiding this comment

etr2460 commented Oct 27, 2019

willbarrett commented Oct 28, 2019

mistercrunch commented Oct 29, 2019 • edited Loading

willbarrett commented Oct 29, 2019

codecov-io commented Oct 29, 2019 • edited Loading

Codecov Report

willbarrett commented Oct 29, 2019

villebro left a comment

Choose a reason for hiding this comment

villebro Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

willbarrett commented Oct 30, 2019

Use `config[]` not `config.get()` #8454

Use `config[]` not `config.get()` #8454

villebro Oct 29, 2019 •

edited

Loading

mistercrunch commented Oct 29, 2019 •

edited

Loading

codecov-io commented Oct 29, 2019 •

edited

Loading

villebro Oct 29, 2019 •

edited

Loading