Auto reload for local mode #706

vtitor · 2018-02-08T00:38:05Z

I have implemented auto reload feature by #316 request. It doesn't have any dependencies (according to #658 remarks).
@jamesls, @stealthycoin Could you help me merge this PR as soon as possible, please?

vtitor · 2018-02-28T17:08:04Z

@jamesls, @stealthycoin Hi guys! If I have some mistakes in this pr could you please tell me about them

stealthycoin

Thanks for taking the time to make this pull request. It's been something I've been wanting for awhile as well and I'd like to see it get shipped.

I had some comments and questions. Scattered throughout. The main thing besides the below comments would be to get some tests added.

stealthycoin · 2018-03-02T00:23:27Z

.pylintrc

@@ -304,7 +304,7 @@ valid-metaclass-classmethod-first-arg=mcs

 # List of member names, which should be excluded from the protected access
 # warning.
-exclude-protected=_asdict,_fields,_replace,_source,_make


I would prefer not to reach into the _thread if at all possible. Are there other options?

No, there are no other options.

stealthycoin · 2018-03-02T00:42:23Z

chalice/cli/reload.py

+    def reload(self):
+        # type: () -> None
+        self.triggered = True
+        if sys.platform == 'win32':


We prefer to have all platform specific logic in the compat.py file in one place so we don't need to think about it here.

Sure, I'll fix this.

stealthycoin · 2018-03-02T00:44:06Z

chalice/cli/__init__.py

@@ -79,11 +80,14 @@ def cli(ctx, project_dir, debug=False):
 @click.option('--port', default=8000, type=click.INT)
 @click.option('--stage', default=DEFAULT_STAGE_NAME,
              help='Name of the Chalice stage for the local server to use.')
+@click.option('--autoreload', is_flag=True)


Seems like it should be turned on by default so people don't need to discover the functionality, it just magically works. We could flip it and let them disable it if they want to for some reason. --no-autoreload. Thoughts?

It makes sense.

stealthycoin · 2018-03-02T00:45:42Z

chalice/cli/__init__.py

    factory = ctx.obj['factory']  # type: CLIFactory
-    run_local_server(factory, host, port, stage, os.environ)
+    with Reloader(autoreload):


Seems a little bit strange to me to pass False to class called Reloader. Could we refactor this to just not use the Reloader context manager if its disabled?

You are right, I'll fix it.

stealthycoin · 2018-03-02T00:47:13Z

chalice/cli/reload.py

+    def run(self):
+        # type: () -> None
+        while True:
+            time.sleep(AUTORELOAD_INTERVAL)


This would be nice as a parameter that can be supplied to the reloader.

stealthycoin · 2018-03-02T00:48:21Z

chalice/cli/reload.py

+
+    def find_changes(self):
+        # type: () -> bool
+        for module in list(sys.modules.values()):


No need to cast to list here.

Can we scope this to just the files that are in their Chalice project?

No need to cast to list here.

Why? RuntimeError: dictionary changed size during iteration

Can we scope this to just the files that are in their Chalice project?

We can.. but I think it's better to check all modules.

stealthycoin · 2018-03-02T00:59:14Z

chalice/cli/reload.py

+            subprocess.Popen(sys.argv, close_fds=True)
+            six.moves._thread.interrupt_main()
+        else:
+            os.execv(sys.executable, [sys.executable] + sys.argv)


I haven't had the chance to try out this branch yet but I'm curious about this approach and how it compares to managing a subprocess yourself, or hot reloading the python code. For example:
https://docs.python.org/3.4/library/importlib.html#importlib.reload
https://docs.python.org/3.2/library/imp.html#imp.load_module
etc...

I'm mainly trying to think of a more testable approach that would not need any platform specific logic. Would like to hear your thoughts on why you chose this approach.

Reload python module at runtime looks like a bad idea to me (because it causes inconsistently application state)

stealthycoin · 2018-03-02T01:00:11Z

chalice/cli/reload.py

+        # type: (bool) -> None
+        super(Reloader, self).__init__()
+        self.autoreload = autoreload
+        self.triggered = False


I recognize that this is whether or not the process has ever reloaded. But can you give some clarification on why this is important to keep track of?

We need to have additional behavior when interrupting for the reloaded process in windows (raise SystemExit).

stealthycoin · 2018-03-02T01:01:31Z

chalice/cli/reload.py

+        self.triggered = True
+        if sys.platform == 'win32':
+            subprocess.Popen(sys.argv, close_fds=True)
+            six.moves._thread.interrupt_main()


Can you explain why this is needed on windows?

execv in windows is completely different than in linux. Briefly, it's necessary to close the opened file descriptors.

codecov-io · 2018-05-03T21:11:49Z

Codecov Report

Merging #706 into master will decrease coverage by 0.92%.
The diff coverage is 37.09%.

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
- Coverage   94.79%   93.86%   -0.93%     
==========================================
  Files          21       22       +1     
  Lines        3707     3767      +60     
  Branches      478      486       +8     
==========================================
+ Hits         3514     3536      +22     
- Misses        130      168      +38     
  Partials       63       63

Impacted Files	Coverage Δ
chalice/constants.py	`100% <100%> (ø)`	⬆️
chalice/cli/reload.py	`31.11% <31.11%> (ø)`
chalice/compat.py	`40.9% <42.85%> (+0.36%)`	⬆️
chalice/cli/__init__.py	`78.04% <55.55%> (-0.74%)`	⬇️

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 46cbb73...02da148. Read the comment docs.

vtitor · 2018-05-09T15:28:25Z

@jamesls, @stealthycoin Could you review my changes, please?

jamesls · 2018-05-14T19:09:02Z

Taking a look now. I haven't had a chance to look at this PR in general (thanks @stealthycoin for the feedback so far), but I think the feature itself would be good to have in chalice.

Was there a discussion about pulling in some existing library that can watch for file changes? Seems like it would be preferable to use something based on inotify/fsevents instead of polling for changes.

jamesls · 2018-05-15T17:29:28Z

Hi, I know there's been a lot of requests for this feature so I'll go ahead and pull in these changes and make a few adjustments before merging.

…icts in chalice/cli/__init__.py)

vtitor · 2018-05-15T20:35:58Z

@jamesls Thanks, I will be glad to help you. About inotify/fsevents - sounds good to me, but I guess it would be better to deliver working autoreload feature as soon as possible and then improve it in a separate pr (because as you said there are a lot of requests for this stuff).

This adds a reloader to chalice local that's enabled by default. A new library, watchdog, is pulled in as a dependency of chalice. This allows us to efficiently monitor files for changes using whatever the most efficient OS specific mechanism (FSEvents, inotify, etc). The general approach is borrowed from other frameworks such as django and flask but the implementation is somewhat different. Two processes are spun up, a worker process and a monitoring process. The monitoring process spawns the worker process and restarts it if it exits with a special RC. The worker process launches the dev server in one thread and a file monitor in a separate thread. When files are detected it shuts down the server and exits with a special RC. There's a few differences from the other approaches: * Avoid the use of sys.exit() directly in the reloader. This made the code hard to test and seems odd that a library function is calling sys.exit(). However there is a call to `sys.exit()` in the `local()` function because this is the recommended way in click for a command to exit with a specific RC (see the comments for more info). * We only monitor the project dir for changes. The other reloaders appear to monitor everything in sys.path, which seems like overkill here. The way I see it, stuff on sys.path would only be modified if you're installing new packages, in which case you'll have to modify requirements.txt as well as something in your app.py (in order to import the newly installed module). I dug around their repos for a bit and couldn't find a rationale as to why that much monitoring is needed. A note on coverage: because the reloader works by launching child processes, this isn't going to be tracked via coverage, so I expect codecov to fail. There is some support for tracking coverage across subprocesses, but it involved messing with sitecustomize.py or .pth files. See http://coverage.readthedocs.io/en/coverage-4.2/subprocess.html for more info on that. Supercedes aws#706.

jamesls · 2018-05-21T19:43:33Z

Merged via #846. This will go out in the next release. Thanks again for the original PR.

vtitor added 8 commits February 6, 2018 04:05

Add auto reloading stuff

d9a9c31

Extract reloader into separate module

641664b

Add the autoreload interval const

0e86e1d

Fix six import and naming

dcec0dd

Extend exclude-protected list in .pylintrc

7251b62

Add type annotation

811d5d9

Fix F401 (imported but unused) flake8-mistake

c547e8d

Fix context manager support for type annotation

ab52510

stealthycoin suggested changes Mar 2, 2018

View reviewed changes

stealthycoin added the changes-requested label Mar 2, 2018

Merge remote-tracking branch 'upstream/master' into reload

a563448

vtitor added 8 commits May 6, 2018 21:18

Add test for reloader

84f747a

Enable autoreloading by default

16c0ab3

Add --autoreload-interval parameter

c3f0886

Extract some reloads stuff to compat.py

18e8a80

Little fix for reloader stuff

6e27654

Fix redefining built-in 'reload'

5c0ab33

Fix py2-py3 compatibility for reloader test

9ed1a9a

Merge remote-tracking branch 'upstream/master' into reload

3383c45

Merge remote-tracking branch 'upstream/master' into reload (fix confl…

02da148

…icts in chalice/cli/__init__.py)

jamesls mentioned this pull request May 16, 2018

Automatic reload on code change (#316) #658

Closed

jamesls mentioned this pull request May 18, 2018

Automatically reload dev server when files change #846

Merged

jamesls closed this May 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto reload for local mode #706

Auto reload for local mode #706

vtitor commented Feb 8, 2018

vtitor commented Feb 28, 2018

stealthycoin left a comment

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018 •

edited

Loading

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

stealthycoin Mar 2, 2018

vtitor Mar 14, 2018

codecov-io commented May 3, 2018 •

edited

Loading

vtitor commented May 9, 2018

jamesls commented May 14, 2018

jamesls commented May 15, 2018

vtitor commented May 15, 2018

jamesls commented May 21, 2018

Auto reload for local mode #706

Auto reload for local mode #706

Conversation

vtitor commented Feb 8, 2018

vtitor commented Feb 28, 2018

stealthycoin 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

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

vtitor Mar 14, 2018 • edited Loading

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

codecov-io commented May 3, 2018 • edited Loading

Codecov Report

vtitor commented May 9, 2018

jamesls commented May 14, 2018

jamesls commented May 15, 2018

vtitor commented May 15, 2018

jamesls commented May 21, 2018

vtitor Mar 14, 2018 •

edited

Loading

codecov-io commented May 3, 2018 •

edited

Loading