Sleep in Reloader loop to conserve CPU resources

[eirikrye]

Fixes #2566

The Reloader class already has an unused interval property, this pull request simply adds the missing sleep.

[codecov]

Codecov Report

Base: 88.036% // Head: 88.012% // Decreases project coverage by -0.024% ⚠️

Coverage data is based on head (357944b) compared to base (f891995).
Patch coverage: 91.666% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #2567       +/-   ##
=============================================
- Coverage   88.036%   88.012%   -0.025%     
=============================================
  Files           71        71               
  Lines         5333      5339        +6     
  Branches       987       894       -93     
=============================================
+ Hits          4695      4699        +4     
  Misses         461       461               
- Partials       177       179        +2     

Impacted Files	Coverage Δ
sanic/mixins/startup.py	91.705% <66.666%> (-0.194%)	⬇️
sanic/config.py	97.368% <100.000%> (+0.017%)	⬆️
sanic/worker/reloader.py	97.101% <100.000%> (+0.131%)	⬆️
sanic/server/websockets/impl.py	37.115% <0.000%> (-0.237%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ahopkins]

Good catch. Thanks!

[ahopkins]

This really should be configurable, and TBH I will accept it as an oversight by myself.

https://github.com/sanic-org/sanic/blob/main/sanic/mixins/startup.py#L820

☝️ This line should be something like:

Reloader(..., primary.config.RELOADER_INTERVAL, ...)

And then also have a default value, and type annotation.

Would you be willing to add that to this PR?

[eirikrye]

This is now configurable through AUTO_RELOAD_INTERVAL.

The second commit is an attempt to make it the interval configurable through the CLI, however since I am not very familiar with this code base I am not entirely sure if I did it "right". While the code works, the setting is not actually visible in app.config at runtime.

[eirikrye]

I am not sure about how good this CLI API feels, both in naming of the new arguments, and its verbosity:

~# sanic main:app --auto-reload --auto-reload-interval 5
~# sanic main:app -r -i 5
~# sanic main:app --reload --interval 5

Should setting the interval through the CLI implicitly enable the reloader, so that you only need to call it like this?

~# sanic main:app --auto-reload-interval 5

This works fine, but some ambiguity ("what interval?") would be introduced when using the short forms:

~# sanic main:app --interval 5
~# sanic main:app -i 5

Alternatively, instead of two separate arguments, maybe -r, --reload, --auto-reload could accept the float argument directly (using nargs="?"), and set the default value (1.0) if no value is provided, otherwise use the passed argument as the interval?

This would result in this API:

~# sanic main:app --reload 5.0

AUTO_RELOAD=True, AUTO_RELOAD_INTERVAL=5.0

~# sanic main:app --reload

AUTO_RELOAD=True, AUTO_RELOAD_INTERVAL=1.0 (default)

~# sanic main:app

AUTO_RELOAD=False, AUTO_RELOAD_INTERVAL=1.0 (default)

[ahopkins]

Should setting the interval through the CLI implicitly enable the reloader, so that you only need to call it like this?

Yeah, I like this.

This works fine, but some ambiguity ("what interval?") would be introduced when using the short forms:

I am not 100% sold on the short form. I suppose if we are allowing it to act implicit -i is okay since it is one character. Maybe we scrap --interval altogether. Thoughts?

Alternatively...

This is a nice idea. Make sure to keep --auto-reload as an option though. We could make them a mutually exclusive group. Example.

Of all the options, I like this the best.

sanic ... --auto-reload
sanic ... --reload
sanic ... --reload=4.0
sanic ... --r 4.0

[Tronic]

IMO this needlessly bloats app.run and CLI API. Couldn't you put that in app.config instead?

[eirikrye]

IMO this needlessly bloats app.run and CLI API. Couldn't you put that in app.config instead?

With the suggested API of rolling it into the existing --reload command (e.g. --reload to enable reloading and default interval, --reload 3.0 to enable reloading and set custom interval), I don't think its bloated at all.

It's supported in app.config as part of this pull request, but I personally feel that things like enabling debug mode, auto-reload and configuring the reload interval are things that you want to set ephemerally instead of in the more static app configuration (which would likely be committed to version control and so on)

[ahopkins]

This pull request has been mentioned on Sanic Community Discussion. There might be relevant details there:

https://community.sanicframework.org/t/on-intel-mac-the-sanic-dev-mode-uses-99-cpu/1077/2

[Tronic]

IMO this needlessly bloats app.run and CLI API. Couldn't you put that in app.config instead?

With the suggested API of rolling it into the existing --reload command (e.g. --reload to enable reloading and default interval, --reload 3.0 to enable reloading and set custom interval), I don't think its bloated at all.

It's supported in app.config as part of this pull request, but I personally feel that things like enabling debug mode, auto-reload and configuring the reload interval are things that you want to set ephemerally instead of in the more static app configuration (which would likely be committed to version control and so on)

You can set config also by environment variables, e.g.

SANIC_RELOAD_INTERVAL=3 sanic your.app --reload

All CLI options show up in CLI help, and thus I am vary of adding anything extra there. The app.run and CLI interfaces are intended to be kept in sync as far as possible, and while on the former you could simply pass a number instead of True to override the default without so much bloating the API, the same doesn't apply to CLI.

You could also always set the interval via config, but it would only have an effect when the reloader is enabled. I suppose you wouldn't normally have use to ephemerally configure for different instantiations. In reality, I would expect there to be very little reason to ever change the value but I see that there could be some use to have it configurable, the same as all other timeouts and various other parameters are configurable. As an added benefit, you could export SANIC_RELOAD_INTERVAL=0.1 and avoid repeating it on each command/app, if you always need it super fast on anything you run on that shell.

[eirikrye]

All CLI options show up in CLI help, and thus I am vary of adding anything extra there.

I mean, the current changes in the pull request adds two lines to the CLI help, out of a total of 98 lines. The suggested change above would add zero lines to the CLI help (it'd just rewrite the help for the existing --reload option).

On the other hand, I do agree that there's likely very few reasons someone would want to set the reload interval to a non-default value. For really large large projects stored on a slow storage medium (e.g. HDDs), the default interval may not be enough for the code to check all the module files it is monitoring, but for those people configuring it through config or environment variables would probably suffice.

I'd be happy to revert the CLI changes and just keeping the AUTO_RELOAD_INTERVAL (or RELOAD_INTERVAL?) config setting. The primary motivation for this pull request was just to prevent the auto reloader from killing my CPU.

[Tronic]

I mean, the current changes in the pull request adds two lines to the CLI help, out of a total of 98 lines. The suggested

This is an unfortunate consequence of there already being a lot of options on app.run that have then been ported to CLI for parity.

On the other hand, I do agree that there's likely very few reasons someone would want to set the reload interval to a non-default value. For really large large projects stored on a slow storage medium (e.g. HDDs), the default interval may not be enough for the code to check all the module files it is monitoring, but for those people configuring it through config or environment variables would probably suffice.

I didn't look at your implementation yet, but ideally the delay should add to the time it takes to do the scan (i.e. sleep for that duration between scans): if scanning takes 1 second and the setting is 3 seconds, it should run the scan every four seconds. As you point out, this adapts to different systems better and avoids ending up in a busy loop (or worse, multiple scans running at the same time) if the scanning actually takes longer than the setting is.

As has been mentioned before, ideally there would also be an implementation to watch for modification rather than poll for them, if the OS and the filesystem support watching (not always the case even on Linux with network filesystems that may not watch for remote changes).

I'd be happy to revert the CLI changes and just keeping the AUTO_RELOAD_INTERVAL (or RELOAD_INTERVAL?) config setting. The primary motivation for this pull request was just to prevent the auto reloader from killing my CPU.

I like the shorter names the better. Less writing is always good, especially on command line where you need the SANIC_ prefix too.

I should also clarify that I intend to use no authority on this matter, just weighing in on my personal opinion. @ahopkins usually has a good idea of development and knows Sanic better than anyone, so I would like to hear what he thinks too.

[ahopkins]

I am going to leave this for now and we can come back to making a better decision on this for LTS. In #2595 I implemented the original strategy so we can release a patch as intended and then finish this.

[ahopkins]

So, I am thinking that maybe this should just be a class variable. We have a few different values like this that are really low-level and would cloud the config space. Yet on the other hand we want to maintain some ability to control without ugly patches. Therefore, I propose someone that wants to override do the following:

from sanic.worker.reloader import Reloader

Reloader.INTERVAL = 5.0

[ahopkins]

Closing in favor of #2633