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

Production and development modes (default workers) #2169

Closed
Tronic opened this issue Jun 17, 2021 · 14 comments · Fixed by #2295
Closed

Production and development modes (default workers) #2169

Tronic opened this issue Jun 17, 2021 · 14 comments · Fixed by #2295

Comments

@Tronic
Copy link
Member

Tronic commented Jun 17, 2021

While it is possible to specify these individual features by hand, I would rather see a simple mode flag to get sensible defaults:

  • Production: no access logs, workers=os.cpu_count()
  • Development: debug, access logs, auto-reloader

Sanic server by default runs with a single worker process and doesn't enable features that would be useful in development. Many people accidentally use these default settings for benchmarking and production but the performance is poor due to lack of multiprocessing and due to logging if app.run was used. The debug mode cannot be enabled by default because running it in production is a potential security risk, so making the production mode the new default seems a better option.

Notably the reloader setting by default already follows the debug flag. I believe this should done with the access log setting too (currently app.run(debug=True) and app.run(debug=False) both enable access logs, while sanic CLI disables logs irregardless of the --debug flag). Alike, the number of workers could depend on the debug flag, keeping the single worker default in debug mode, but using the number of CPUs by default.

By these changes we get production mode by default, and adding debug=True or sanic --debug would accomplish full switch of all relevant settings into development mode.

This is potentially a breaking change with application servers that run multiple Sanic instances on different ports with the intention that each Sanic instance stays a single process. If this is a problem, specifying workers=1 in these installs is a simple and backwards-compatible fix.

Relevant app.run parts shown with changes:

    def run(
        debug: bool = False,
        auto_reload: Optional[bool] = None,  # already follows debug flag if not set
        workers: Optional[int] = None,   # instead of: workers: int = 1
        access_log: Optional[bool] = None,  # was already this but doesn't work as expected
    ) -> None:
        # New logic
        if workers is None:
            workers = 1 if debug else os.cpu_count()
        if access_log is None:
            access_log = debug

I'd like to hear your thoughts before implementing this. This can probably be done better, or perhaps I overlooked something.

@ahopkins
Copy link
Member

This lines up with another thought that I had related to some discussions recently, and that is to give more teeth to debug mode. We are pretty light on debug logs, and that could probably be turned up a bit.

As for default workers=os.cpu_count(), I am not sold on this yet. I would be curious to see the impact of this on multi-pod k8s installations with single worker instances. This is something that has been on my to-do list for a while. Maybe now is a good time to prioritize that.


With that said, I do like the idea of rethinking our defaults, the project has grown and matured a lot and we have not changed these. As for production mode by default... 🤔 My reservation on that is it may be a bit confusing for someone kicking the tires. This could be remedied with an example that explicitly enables debug mode, but it is worth mentioning.

The other thing that might be helpful here is a little more information in the startup logs. Fairly recently I added a little more:

[2021-06-17 20:55:11 +0300] [930550] [INFO] Goin' Fast @ http://127.0.0.1:9999
[2021-06-17 20:55:11 +0300] [930550] [DEBUG] Sanic auto-reload: enabled
[2021-06-17 20:55:11 +0300] [930550] [DEBUG] Sanic debug mode: enabled
[2021-06-17 20:55:11 +0300] [930550] [INFO] Starting worker [930550]

@eric-spitler
Copy link

eric-spitler commented Jun 17, 2021

I would be opposed to defaulting to more than one worker due simply to the fact that workers cannot spawn child processes due to the daemonic nature.

Even doing a ProcessPoolExecutor gets mucked up when you have multiple Sanic workers.

Setting this default would be a breaking change to any applications currently using child processes.

@ashleysommer
Copy link
Member

ashleysommer commented Jun 18, 2021

I agree with @ahopkins and @eric-spitler, we should definitely default to a single worker even in Production mode. Multi-workers should always be something a user specifically enables manually, for the reasons outlined above.

And just for some context, around why sanic has such weird defaults to begin with. Its because Sanic's app.run() method was designed to match with Flask's app.run(), and have the same defaults. The goal was for Flask users to move over to Sanic and need to change very little app code, like a drop-in performance upgrade.

Of course Sanic has evolved a lot since then, and moved away from its Flask-mimic heritage, so I agree we're overdue to change the defaults.

On a personal note, I'm not a fan of setting auto_reload=True by default in debug mode. I know Sanic already does that, and I always disable it. It messes up my debugging workflow in PyCharm for three reasons:

  1. The main process becomes a file-watcher, and a worker process (with a separate PID) is created to run the app. PyCharm's debugger can't catch breakpoints from a process with a different PID than what it launched with.
  2. PyCharm auto-saves at odd times. If I'm debugging an app and making changes to the files while running the app, it can be jarring for the app to unexpectedly restart while I'm editing a file.
  3. Similar to 2, when debugging a program, I might be writing editing code in a different unrelated python file in the same project, but the app restarts when the change is made because it detects that change.

For those reasons, I always use app.run("localhost", 8080, debug=True, auto_reload=False) in my applications. But I understand auto_reload something that a lot of people really like, enough that it should remain the default behaviour in debug mode.

@ahopkins
Copy link
Member

What if we add a --workers=full arg? It might also be nice to have --mode=debug|default|prod, and then it's a matter of deciding what each of those means. I'll post some more coherent thoughts a little later after some coffee.

@Tronic
Copy link
Member Author

Tronic commented Jun 18, 2021

Okay, looks like there are legitimate concerns about multiple workers by default. Can we still disable access logs by default in app.run as they are disabled by default in CLI?

How about this:

Revert debug to being a stand-alone feature (that won't enable auto-reload), and implement a separate --dev flag on CLI that is a shorthand for --debug --access-log --reload for those who want all three. Then add --fast to have workers match the CPU count (and potentially add other optimisations in the future). Awesome CLI sanic --fast goin.app

@ahopkins
Copy link
Member

This is sounding much more the direction I'd like to go. Several config options to be done separately, and then bundled together with smart defaults.

  • debug logging (perhaps even with levels of verbosity)
  • access logs
  • worker optimization
  • motd
  • deprecation notices

👍 to --dev and --fast (-D and -F)?

@Tronic
Copy link
Member Author

Tronic commented Jun 18, 2021

The names are short enough without single-letter versions IMO. Also worth noting is that these two are not contradictory modes since --dev --fast is a perfectly valid combination. (unless --fast also implies disabling access logs)

@ahopkins
Copy link
Member

ahopkins commented Jun 18, 2021

I think it does. "fast" should be disable access logs and enable max workers (unless you specifically enable them separately), and set the sanic loggers appropriately. Maybe disable all loggers except the error logger.

@ahopkins
Copy link
Member

ahopkins commented Jun 18, 2021

Somewhat related...

What about spitting out OS, and platform details along with Sanic installation (this last one I think is in one of the recent PRS I made) at debug startup for easy copy/paste on support queries?

@robd003
Copy link
Contributor

robd003 commented Jun 26, 2021

When checking to see the number of CPUs available can we do an OS check and if the system is Linux then return len(os.sched_getaffinity(0)) as the number of CPUs. This way if the Sanic process is scheduled via Kubernetes with less CPU cores than available on the system it won't be over-provisioned.

Explanation: https://docs.python.org/3/library/os.html#os.sched_getaffinity

@ahopkins
Copy link
Member

That seems reasonable.

@ahopkins
Copy link
Member

ahopkins commented Sep 11, 2021

Here is my proposal:

FLAG Tracebacks Logging level Access logging Reload Workers to CPU MOTD
--debug yes DEBUG yes ^ yes
(DEFAULT) no INFO ^^ yes
--prod no WARNING no no
--dev DEBUG yes yes
--fast no yes
  • ^ --debug to deprecate auto-reloading and remove in 22.3
  • ^^ default will still do access logs until 22.3

@Tronic
Copy link
Member Author

Tronic commented Sep 11, 2021

Quite many modes, and a lot of differences that are not entirely obvious. The table also does not say whether tracebacks are shown on error pages (debug mode can be a security risk).

@ahopkins
Copy link
Member

ahopkins commented Sep 11, 2021

Not really.... it is three modes: debug/default/prod, only one of which is new: prod. To be honest, that is the one I am not sure about because the only thing it really changes is the logging level. Perhaps we have some other performance enhancements in the future, but I am not a huge fan of adding features we are not really using yet.

--dev and --fast are more like modifiers (exactly as your proposal). The goal here is to thread the line between adding functionality and not breaking anything (yet).

$ sanic -h
...
  --mode [{debug,prod}]          Set the mode
  -d, --debug                    Shortcut to set to DEBUG mode
  --prod                         Shortcut to set to PROD mode
  --dev                          Shortcut to set to DEBUG mode, and enables auto-reload

Yes, existing tracebacks would not change. I will update the table to reflect that

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

Successfully merging a pull request may close this issue.

5 participants