-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Hot reload #362
Conversation
9a48f6c
to
8955a92
Compare
@plotly/dash Ready for reviews! |
if pattern and not pattern.search(f): | ||
continue | ||
path = os.path.join(current, f) | ||
info = os.stat(path) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used to trigger a soft reload.
https://github.com/plotly/dash-renderer/blob/71a42e2f63880ec8ce2cbc01a8e18e582857f97d/src/components/core/Reloader.react.js#L35
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, _): |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
A few things I ran into:
It seems like the developer-facing API is small (just the
|
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:
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
There's also
Could do that by disabling the old stylesheets and append the new one with the modified timestamp in the head. |
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. |
I solved it by including the keys of 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. |
@chriddyp I added that functionality. |
a47c5d7
to
6736cc7
Compare
Updated the rc versions:
|
I'm having trouble with running this. When I start my app, it goes into an endless loop with the following error messages:
|
@mbkupfer thanks for reporting, I think I know what's wrong, will post update soon. |
@mbkupfer Released dash==0.29.0rc4, should fix the json issue on py3. |
Had to remove the resources cache, a proper cache bust check was more expansive than remapping them. |
Amazing work @T4rk1n ! Is there is an issue logged for the documentation items I mentioned in #362 (comment)? |
@chriddyp There is an issue for the dev tools https://github.com/plotly/dash-docs/issues/196 |
Available with the following versions:
dash==0.29.0rc4
dash-renderer==0.15.0rc2
Reload the browser when files are changed.
Dev tools params (in
app.run_server
orapp.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.