Config cleanup #761

alexcjohnson · 2019-06-07T03:05:17Z

closes #746

Some breaking changes to the `dash.Dash` API

Constructor / config breaking changes:

Removed two obsolete constructor kwargs: static_folder (assets_folder is much smarter) and components_cache_max_age (all files have cache-busting URLs)
Removed the very old misspelled supress_callback_exceptions fallback
Removed the unused resources.config.infer_from_layout
Revamped app.config: ALL constructor args are now stored in config (and none are stored in any other instance attributes anymore), with three exceptions:
- server - app.init_app(server) is the right way to set this
- index_string - it has a special setter method
- plugins - You could add more plugins by calling my_plugin.plug(app) but not by modifying a config entry

Dev tools breaking changes:

Changed hot_reload_interval from msec to seconds, for consistency with hot_reload_watch_interval
When called from enable_dev_tools, debug=True by default. It's still False by default from run_server.

Non-breaking changes:

The expanded app.config prevents you from adding new (ie misspelled or obsolete) items and makes some items read-only - the ones that will not take effect if changed after the constructor (and init_app, since that can happen in the constructor) have finished.
The default assets_url_path is now just 'assets', not '/assets'. Since in actual usage we always strip the leading '/' and prepend the requests prefix, it's misleading as well as harder to explain with the slash there.
New constructor kwarg serve_locally - default True as per serve_locally by default #722 - sets both JS and CSS config
Added a docstring for the dash.Dash constructor and improved docstrings for the run_server and enable_dev_tools methods

Not implemented:

I looked into the name parameter and seems to me we should NOT make it mandatory, but would be interested to hear what others think about this. The only time this seems to matter is if the file that was run (python run.py in our common usage, or the gunicorn equivalent) is in a different directory from the one that defines app (let's call it app.py). All it seems to affect is where Flask looks for other files specified by relative paths, the key one being assets_folder.

Our default, '__main__', says these are relative to run.py.
Our recommended usage, name=__name__, says they're relative to app.py
You can specify something else with a package path (dotted, like package.sub.path as in import package.sub.path, and it will be resolved exactly as that import would be)
AFAICT it does NOT matter what the working dir was, relative to run.py, when you ran Python

Most of the time this doesn't matter, as run.py and app.py, if they're even separate files, are usually in the same dir. __name__ is nice in that you get the same assets folder no matter where you import app.py from, but on the other hand I suspect the more likely situation is that you set up your project and then at some point decide to move app.py into a subdirectory, without moving assets or run.py. In that case __name__ would cause problems, and the current default '__main__' is probably correct. That to me is a sufficient reason to leave the default behavior as it is.

Contributor Checklist

I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR
I have added entry in the CHANGELOG.md
If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow
- this github #PR number updates the dash docs
- here is the show and tell thread in plotly dash community

it's more confusing to describe with it present - and we always strip it off in practice

also a fix for debug logic re: env vars

except server, index_string, and plugins

… use lock as context manager

byronz · 2019-06-07T03:19:50Z

dash/CHANGELOG.md

@@ -1,5 +1,13 @@
 ## Unreleased
 ### Changed
+- [#761](https://github.com/plotly/dash/pull/761) Several breaking changes to the `dash.Dash` API:


a 💥 here?

byronz · 2019-06-07T03:35:37Z

dash/dash.py

@@ -86,106 +86,223 @@ class _NoUpdate(object):
 # pylint: disable=too-many-instance-attributes
 # pylint: disable=too-many-arguments, too-many-locals
 class Dash(object):
+    """


😸 nice docstring.

tangential question, do we have an agreed reference to follow? I feel like it's not very helpful to use super-long-detailed-sphinx-style; chris is also building sth in the house.

but from our users' perspective, data scientists might feel this more familiar https://sphinxcontrib-napoleon.readthedocs.io/en/latest/

I'd be up for changing to a cleaner style, I was just copying the style we already had for run_server and enable_dev_tools.

Feels to me like we should have a decent level of detail here, and then use this to generate the corresponding page in dash-docs. Let's keep this format until we decide how we want to do that, so we only need to alter it once. @chriddyp I'm not aware if you're working on something else to fill this gap.

byronz · 2019-06-07T03:42:49Z

dash/dash.py

+    Dash is a framework for building analytical web applications.
+    No JavaScript required.
+
+    If a parameter can be set by an environment variable, that is listed too.


that is listed with env: prefix?

byronz · 2019-06-07T03:46:47Z

dash/dash.py

+    :type server: boolean or flask.Flask
+
+    :param assets_folder: a path, relative to the current working directory,
+        for extra files to be used in the browser. Default ``'assets'`` By


the Default 'asset' By default needs a better split.

byronz · 2019-06-07T03:48:36Z

dash/dash.py

+        Default ``'assets'``.
+    :type asset_url_path: string
+
+    :param assets_ignore: A regexp, as a string to pass to ``re.compile``, for


regexp is js flavor, pythonic name should be regex

byronz · 2019-06-07T03:53:35Z

dash/dash.py

+        be called after the Flask server is attached.
+    :type plugins: list of objects
+
+    :param **kwargs: Do not use. Gives error messages for obsolete arguments.


would it be more intuitive if we add an example section e.g. man lsof for less experienced users?

You mean rather than the errors on providing old kwargs? I was thinking of those just as helpers for experienced users of old dash versions. This documentation of **kwargs is just intended for people who see **kwargs in the function signature and wonder why it's there. Perhaps I can omit this documentation and just rename it to **obsolete, and assume that's sufficiently self-documenting?

Sorry for the confusion, I meant for all the docstring. Just happened to leave the comment at this line. Nothing wrong with kwargs.

Got it. I feel like once people get here they've seen examples already elsewhere in the docs, so this docstring is just about enumerating all the options.

Anyway I will change **kwargs to **obsolete and remove it from the docstring.

byronz · 2019-06-07T03:57:30Z

tests/integration/test_render.py

@@ -1164,7 +1164,7 @@ def test_removing_component_while_its_getting_updated(self):
            ),
            html.Div(id='body')
        ])
-        app.config.supress_callback_exceptions = True
+        app.config.suppress_callback_exceptions = True


predicted merge conflicts here

byronz · 2019-06-11T03:00:45Z

tests/integration/test_integration.py

@@ -870,6 +871,40 @@ def failure2(children):
            context.exception.args[0]
        )

+    def test_callback_dep_types(self):


@alexcjohnson do you want to migrate this with new style?

not too hard to migrate -> 4052507

byronz · 2019-06-11T03:06:35Z

dash/testing/browser.py

        return self._wait_for(
            EC.presence_of_element_located,
            ((By.CSS_SELECTOR, selector),),
            timeout,
-            "timeout {} => waiting for selector {}".format(timeout, selector),
+            "timeout {}s => waiting for selector {}".format(_time, selector),


nice fix on the timeout
🐄 minor, I would use more pythonic way format (timeout if timeout else self._wait_timeout, selector)

🐄 in 55274be

byronz

💃 nice job! the two new comments are good to have, feel free to merge without the change

byronz · 2019-06-11T16:11:36Z

tests/integration/test_integration.py

+    input_.send_keys('xyz')
+    dash_duo.wait_for_text_to_equal('#input', 'initial inputxyz')
+
+    # callback1 runs 4x (initial page load and 3x through send_keys)


I would put the comment as an assert msg, so you can see that directly in the assertion exception. the comment does show up too with the code snippet.

byronz · 2019-06-11T16:12:55Z

tests/integration/test_integration.py

-        with self.assertRaises(InvalidCallbackReturnValue):
-            multi('aaa')
+    btn_elements = [
+        dash_duo.find_element('#' + x) for x in btns


minor: prefer btn than x here

byronz · 2019-06-11T16:19:59Z

tests/integration/test_integration.py

-        app = Dash(__name__)
+    btn = dash_duo.find_element('#btn')
+    for _ in range(10):
+        btn.click()


you can use this API dash_duo.multiple_click(). yep the API reference is coming soon

for _ in range(clicks): self.find_element(css_selector).click()

byronz · 2019-06-11T16:24:51Z

tests/integration/test_integration.py

+    ])
+
+    with pytest.raises(IncorrectTypeException):
+        @app.callback([[Output('out', 'children')]],  # extra nesting


comment can be set as message='' inside raises

byronz · 2019-06-11T16:28:31Z

tests/integration/test_integration.py

+    # Remove those
+    comment_regex = r'<!--[^\[](.*?)-->'  # noqa: W605
+
+    # Somehow the html attributes are unordered.


this permutation was a bad idea, I did similar clean up with beautiful soup parser for the initial state test case

dash/tests/integration/renderer/test_due_diligence.py

Lines 58 to 70 in 3a9c3cf

# Note: this .html file shows there's no undo/redo button by default

with open(

os.path.join(

os.path.dirname(__file__), "initial_state_dash_app_content.html"

)

) as fp:

expected_dom = BeautifulSoup(fp.read().strip(), "lxml")

fetched_dom = dash_duo.dash_outerhtml_dom

assert (

fetched_dom.decode() == expected_dom.decode()

), "the fetching rendered dom is expected"

yeah I noticed that - BeautifulSoup is a nice idea, I'll try it here.

alexcjohnson added 21 commits June 4, 2019 13:10

remove old misspelled supress->suppress fallback

64dc0c9

remove Dash **kwargs, unused except to warn about csrf removal

3be5dc6

remove static_folder kwarg - assets_folder is better

e48b12c

remove components_cache_max_age kwarg - we have cache-busting urls

50527e1

remove unused resources.config.infer_from_layout

f0282f8

new Dash kwarg serve_locally - sets Scripts and Css config

ae685ee

helpful errors if old kwargs are used in Dash constructor

7c76583

remove leading '/' from default assets_url_path

37cf546

it's more confusing to describe with it present - and we always strip it off in practice

docstring for Dash class

b4185e2

set default debug=True if enable_dev_tools is called directly

1ef7046

change hot_reload_interval from msec to sec to match watch_interval

2f737cd

update docstrings for enable_dev_tools and run_server

e32914e

simplify and DRY dev_tools setup

77d42a4

also a fix for debug logic re: env vars

simpler AttributeDict usage, and typo fix

e579d34

include all constructor args in app.config

2eab141

except server, index_string, and plugins

fix some callback deps validation edge cases

3d749f1

linting / style improvements

d4a09ba

linting, encapsulate hot reload state & helpers in AttributeDict, and…

a4146d3

… use lock as context manager

prevent changing some config keys or adding any new ones

e75de81

oops, who knew, reload is a builtin

d898546

test_utils import order fix

080ac2b

alexcjohnson added the dash 1.0 label Jun 7, 2019

alexcjohnson requested a review from byronz as a code owner June 7, 2019 03:05

alexcjohnson requested a review from chriddyp June 7, 2019 03:07

changelog for dash.Dash API changes PR

0056298

byronz reviewed Jun 7, 2019

View reviewed changes

docstring updates as suggested by @byronz

cbd48b0

alexcjohnson added 7 commits June 8, 2019 09:01

self-documenting **obsolete

02b31cd

boom in changelog

519fdc9

Merge branch 'master' into config-cleanup

bc50899

re-remove supress fallback after merging test refactor

9f20298

improve testing messages for default timeout

b7b71af

re-convert hot_reload_interval to seconds in tests

dbc1981

try to robustify callbacks_generating_children

7fe4b52

byronz reviewed Jun 11, 2019

View reviewed changes

byronz approved these changes Jun 11, 2019

View reviewed changes

alexcjohnson added 4 commits June 11, 2019 07:19

collapse _time intermediate

55274be

evaluate wait_cond before any wait in until(_not)

6c93e60

convert test_integration.py to pytest

4052507

Merge branch 'master' into config-cleanup

571a710

byronz reviewed Jun 11, 2019

View reviewed changes

add some tcids

62a18d3

byronz reviewed Jun 11, 2019

View reviewed changes

BZ-proof test_integration

b389bfb

byronz merged commit 558047f into master Jun 11, 2019

alexcjohnson deleted the config-cleanup branch June 11, 2019 17:59

HammadTheOne pushed a commit to HammadTheOne/dash that referenced this pull request May 28, 2021

Handle Windows-style paths in webpack config rules. (plotly#761)

c7909e3

HammadTheOne pushed a commit that referenced this pull request Jul 23, 2021

Handle Windows-style paths in webpack config rules. (#761)

5988aab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config cleanup #761

Config cleanup #761

alexcjohnson commented Jun 7, 2019 •

edited

Loading

byronz Jun 7, 2019

byronz Jun 7, 2019

alexcjohnson Jun 8, 2019

byronz Jun 7, 2019

byronz Jun 7, 2019

byronz Jun 7, 2019

byronz Jun 7, 2019

alexcjohnson Jun 8, 2019

byronz Jun 8, 2019

byronz Jun 8, 2019

alexcjohnson Jun 8, 2019

byronz Jun 7, 2019

byronz Jun 7, 2019

byronz Jun 11, 2019

alexcjohnson Jun 11, 2019

byronz Jun 11, 2019

alexcjohnson Jun 11, 2019

byronz left a comment

byronz Jun 11, 2019

byronz Jun 11, 2019

byronz Jun 11, 2019

byronz Jun 11, 2019

byronz Jun 11, 2019 •

edited

Loading

alexcjohnson Jun 11, 2019

	# Note: this .html file shows there's no undo/redo button by default
	with open(
	os.path.join(
	os.path.dirname(__file__), "initial_state_dash_app_content.html"
	)
	) as fp:
	expected_dom = BeautifulSoup(fp.read().strip(), "lxml")

	fetched_dom = dash_duo.dash_outerhtml_dom

	assert (
	fetched_dom.decode() == expected_dom.decode()
	), "the fetching rendered dom is expected"

Config cleanup #761

Config cleanup #761

Conversation

alexcjohnson commented Jun 7, 2019 • edited Loading

Some breaking changes to the dash.Dash API

Dev tools breaking changes:

Non-breaking changes:

Not implemented:

Contributor Checklist

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byronz left a 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

Choose a reason for hiding this comment

byronz Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Jun 7, 2019 •

edited

Loading

Some breaking changes to the `dash.Dash` API

byronz Jun 11, 2019 •

edited

Loading