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

Hot reload #362

Merged
merged 29 commits into from
Nov 14, 2018
Merged

Hot reload #362

merged 29 commits into from
Nov 14, 2018

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Aug 31, 2018

Available with the following versions:

  • dash==0.29.0rc4
  • dash-renderer==0.15.0rc2

Reload the browser when files are changed.

  • Use the flask debug reloader to reset the hash on python file change.
  • Watch the assets files and change the hash on assets modified.
  • Add a route to serve the reload hash.

Dev tools params (in app.run_server or app.enable_dev_tools

  • dev_tools_hot_reload, bool set to true to enable hot reload (default=False).
  • dev_tools_hot_reload_interval, int, interval at which the renderer will request the reload hash (default=3000).
  • dev_tools_hot_reload_watch_interval, float, The delay between each walk of the assets folder to detect file changes.
  • dev_tools_silence_routes_logging, disable the route logging.

Hot reload is activated by default when debug=True !!

Need plotly/dash-renderer#73.

Resolves #66.
Fix #341.

@T4rk1n T4rk1n force-pushed the hot-reload branch 2 times, most recently from 9a48f6c to 8955a92 Compare September 5, 2018 17:18
@T4rk1n T4rk1n changed the title [WIP] Hot reload Hot reload Sep 5, 2018
@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 11, 2018

@plotly/dash Ready for reviews!

if pattern and not pattern.search(f):
continue
path = os.path.join(current, f)
info = os.stat(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

os.path.getmtime might be cleaner here. I thought there was platform dependence issue at first (since otherwise why would they even implement os.path.getmtime if you can just os.stat(path).st_mtime, but it turns out thats exactly what os.path.getmtime does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, some methods in that module seems to be wrapper around os.stat. I'm used to os.stat so I just do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍, either way is good, just seemed a bit strange, seems to break "There should be one-- and preferably only one --obvious way to do it".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the os module predates the PEP's.


def watch(folders, on_change, pattern=None, sleep_time=0.1):
pattern = re.compile(pattern) if pattern else None
watched = collections.defaultdict(lambda: -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat, didn't know you could do this.

dash/dash.py Outdated
self._watch_thread = threading.Thread(
target=lambda: _watch.watch([self._assets_folder],
self._on_assets_change,
sleep_time=0.5))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't sleep_time be hot_reload_interval / 1000? I see that hot_reload_interval is passed to the front-end via config, but I'm not sure if it is intended to be the same on the back-end. If not (e.g. back-end should have a fixed constant refresh interval regardless of hot_reload_interval value) I think you should make this a constant set in the _watch file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sleep time is the poll time for the assets watch thread and hot_reload_interval is for the client requests interval. I don't think the user will need to change the sleep time so it can be a constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to make these two times the same, or at least proportional. With this there is a .5 second lower bound on the hot reload interval, and with large intervals (e.g. 10 seconds) you would be making a lot of calls to watch that aren't really necessary, if a user has a huge app that takes a non-trivial amount of computation to traverse the asset tree, this might affect performance.

I think it makes sense to make sleep_time = (hot_reload_interval / 1000) / d where d is some constant. Then in the worst case after making a change, a user would need to wait hot_reload_interval + (hot_reload_interval / d) time for the page to change. d = 2 or d = 4 seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it matter much, it just the timer for when to run the check in the back end and should not be linked in any way to the request timer that can be changed. It should be a constant that work goods in the conditions. We don't want run that check too often because of the GIL and it should still be low so you can get the reload a short while after making changes. So I think a check every half a second is good with this in mind.

In my asyncio based implementation it was a really low number like yield from asyncio.sleep(0.000001) and it didn't make a difference in the performance of the app.

dash/dash.py Outdated
self._watch_thread.start()

if not self.server.debug:
self.server.debug = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would get over-ridden when somebody calls app.run_server(debug=False). Should that happen? I think in run_server you can just say debug = debug or self.hot_reload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will remove that from here and add it in the enable method from #369.

dash/dash.py Outdated
def serve_reload_hash(self):
return flask.jsonify({
'reloadHash': self._reload_hash,
'hard': self._hard_reload
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like the 'hard' variable is used at all in the front-end. Can we just remove it here, in _on_assets_change and in __init__?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay, I see. So is this variable just set to true on the first asset change? I see it gets set to True in _on_assets_change but don't see it getting set back anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, need to reset it after serving the response.

dash/dash.py Outdated
@@ -949,6 +986,12 @@ def get_asset_url(self, path):
self.config.routes_pathname_prefix,
path)

def _on_assets_change(self, _):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pass the argument here? Can't we just call on_change()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The watch function I got here is from another project of mine that I adapted to run on threads instead of asyncio. When a file changed in the other project, I would log the filename thus giving the changed filename as argument in the callback. But we don't have a logging system in-place so I just put a pass param. I guess I could remove the options we're not gonna use here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay, didn't know thats what the argument was. I think we should keep it to add logging in the future, something like File <file_name> was changed at <time>. Maybe put a comment that says the _ is the file name in case another dev makes this change.

Copy link
Contributor

@rmarren1 rmarren1 left a comment

Choose a reason for hiding this comment

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

Looks good! These are mostly questions, and I think there are a few things you can delete

@chriddyp
Copy link
Member

chriddyp commented Sep 14, 2018

This is looking really great!
hot reloading demo

A few things I ran into:

  • If I add a new component library in layout, the JS bundle doesn't get served. Use case: start with an empty app.layout = html.Div([]) and then incrementally build the app. When I added dcc.Graph, it didn't appear until I refreshed.
  • Similarly, if I created a new CSS file, I had to restart the app before I saw it.

It seems like the developer-facing API is small (just the hot_reload tag), so we should be free to change the backend implementation in the future, right? In particular, I would be interested in exploring (in the future):

  • A websocket based approach instead of front-end timers.
  • An approach where CSS files are reloaded separately without a full page reload. Use case:
    • User could click through the application, getting it to a certain state where certain elements are being shown, and then style those elements with CSS without changing the state.
    • CSS updates could happen a lot quicker than the Python updates, since it doesn't (in theory) necessarily require a full refresh of the underling python code

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 14, 2018

If I add a new component library in layout, the JS bundle doesn't get served.

That's a hard one. Since it's a soft reload, it won't reload the index and the new resources doesn't get served.

The solution I am thinking of:

  1. Keep a list of the dependencies in a list in the store, available in a API.requestComponentsLib.
  2. Check the components libs scripts tags are all included in the requestComponentLib response, if not append it to the footer and wait for it to be loaded before triggering a soft reload.

if I created a new CSS file, I had to restart the app before I saw it.

I think that is related to #341. The assets folder is walked only once during initialization, my initial fix was to walk every time the index is served. But then I thought it would be better to integrate the watch of hot-reload and add it to the assets resources since that fix is not needed for deployments and incur a performance cost.

It seems like the developer-facing API is small (just the hot_reload tag),

There's also hot_reload_interval, and maybe watch_interval that can be added.

An approach where CSS files are reloaded separately without a full page reload.

Could do that by disabling the old stylesheets and append the new one with the modified timestamp in the head.

@chriddyp
Copy link
Member

If I add a new component library in layout, the JS bundle doesn't get served.

That's a hard one. Since it's a soft reload, it won't reload the index and the new resources doesn't get served.

Another approach might be to solve it by solving plotly/dash-renderer#46. I think that issue will be a lot more challenging to solve though.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 14, 2018

@chriddyp

I solved it by including the keys of dash.registered_paths in the reload request and checking that the length and content is the same, if not hard reload. Was easier than I thought.

For plotly/dash-renderer#46, I think we would have to add a method to register the packages externally from the layout collection so they can be included even if not in the initial layout.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Sep 17, 2018

An approach where CSS files are reloaded separately without a full page reload

@chriddyp I added that functionality.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Oct 3, 2018

Updated the rc versions:

dash==0.29.0rc3
dash-renderer==0.15.0rc2

@chriddyp
Copy link
Member

chriddyp commented Oct 3, 2018

This latest version is looking really great. The new CSS pathway is especially nice 🎉
hot-reload-demo

@mbkupfer
Copy link

mbkupfer commented Oct 3, 2018

I'm having trouble with running this. When I start my app, it goes into an endless loop with the following error messages:


  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\app.py", line 1718, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\_compat.py", line 35, in reraise
    raise value
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\app.py", line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\app.py", line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\dash\dash.py", line 336, in serve_reload_hash
    'files': list(changed)
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\json\__init__.py", line 321, in jsonify
    dumps(data, indent=indent, separators=separators) + '\n',
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\json\__init__.py", line 179, in dumps
    rv = _json.dumps(obj, **kwargs)
  File "C:\Users\mkupfer\AppData\Local\Programs\Python\Python36-32\Lib\json\__in
it__.py", line 238, in dumps
    **kw).encode(obj)
  File "C:\Users\mkupfer\AppData\Local\Programs\Python\Python36-32\Lib\json\enco
der.py", line 201, in encode
    chunks = list(chunks)
  File "C:\Users\mkupfer\AppData\Local\Programs\Python\Python36-32\Lib\json\enco
der.py", line 430, in _iterencode
    yield from _iterencode_dict(o, _current_indent_level)
  File "C:\Users\mkupfer\AppData\Local\Programs\Python\Python36-32\Lib\json\enco
der.py", line 404, in _iterencode_dict
    yield from chunks
  File "C:\Users\mkupfer\AppData\Local\Programs\Python\Python36-32\Lib\json\enco
der.py", line 437, in _iterencode
    o = _default(o)
  File "C:\Users\mkupfer\Desktop\MYDOCU~1\01_VIS~1\PAY-RA~1\venv\lib\site-packag
es\flask\json\__init__.py", line 81, in default
    return _json.JSONEncoder.default(self, o)
  File "C:\Users\mkupfer\AppData\Local\Programs\Python\Python36-32\Lib\json\enco
der.py", line 180, in default
    o.__class__.__name__)
TypeError: Object of type 'dict_keys' is not JSON serializable
 

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Oct 3, 2018

@mbkupfer thanks for reporting, I think I know what's wrong, will post update soon.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Oct 3, 2018

@mbkupfer Released dash==0.29.0rc4, should fix the json issue on py3.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 14, 2018

Had to remove the resources cache, a proper cache bust check was more expansive than remapping them.

@T4rk1n T4rk1n merged commit f590166 into master Nov 14, 2018
@T4rk1n T4rk1n deleted the hot-reload branch November 14, 2018 21:06
@chriddyp
Copy link
Member

Amazing work @T4rk1n ! Is there is an issue logged for the documentation items I mentioned in #362 (comment)?

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 15, 2018

@chriddyp There is an issue for the dev tools https://github.com/plotly/dash-docs/issues/196

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants