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

Restructure of CLI and application state #2295

Merged
merged 23 commits into from Nov 7, 2021
Merged

Restructure of CLI and application state #2295

merged 23 commits into from Nov 7, 2021

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Oct 30, 2021

Closes #2169
Replaces #2237


Purpose

This PR started out as a way to add some additional running defaults via the CLI. It has grown in scope a little and is taking on some refactoring of the Sanic class (still much more is needed).

The big change (although we strive for backwards compat here, so please LMK if you see anything that is breaking) is to move some properties from the Sanic instance to a new ApplicationState instance. After v21.12, there is a lot more refactoring to be done.

Implementation

Flag Mode Tracebacks Logging Access logs Reload Max workers
--debug DEBUG yes DEBUG yes ^1
PROD no INFO ^2 ^3
--dev DEBUG yes DEBUG yes yes
--fast yes
  • ^1 --debug to deprecate auto-reloading and remove in 22.3
  • ^2 After 22.3 this moves to WARNING
  • ^3 After 22.3: no

Additions

The new features are:

  • --fast: default to the maximum allowed workers
  • --verbosity: allows us to hide some logging noise
  • Pretty MOTD display

image

Cleanup

There is also a bit of refactoring of __main__ to move it into a module. The single function was getting a bit messy to maintain. While I did not touch the bulk of the logic (saved for another time), it does make it a little easier to handle the addition of arguments. And, a side benefit is that they are grouped together a little more nicely.

$ sanic -h
usage: sanic [-h] [--version] [--factory] [-s] [-H HOST] [-p PORT] [-u UNIX] [--cert CERT] [--key KEY] [--tls DIR] [--tls-strict-host]
             [-w WORKERS | --fast] [--access-logs | --no-access-logs] [--debug] [-d] [-r] [-R PATH] [--motd | --no-motd] [-v]
             [--noisy-exceptions | --no-noisy-exceptions]
             module

   ▄███ █████ ██      ▄█▄      ██       █   █   ▄██████████
  ██                 █   █     █ ██     █   █  ██
   ▀███████ ███▄    ▀     █    █   ██   ▄   █  ██
               ██  █████████   █     ██ █   █  ▄▄
  ████ ████████▀  █         █  █       ██   █   ▀██ ███████

 To start running a Sanic application, provide a path to the module, where
 app is a Sanic() instance:

     $ sanic path.to.server:app

 Or, a path to a callable that returns a Sanic() instance:

     $ sanic path.to.factory:create_app --factory

 Or, a path to a directory to run as a simple HTTP server:

     $ sanic ./path/to/static --simple

Required
========
  Positional:
    module                         Path to your Sanic app. Example: path.to.server:app
                                   If running a Simple Server, path to directory to serve. Example: ./

Optional
========
  General:
    -h, --help                     show this help message and exit
    --version                      show program's version number and exit

  Application:
    --factory                      Treat app as an application factory, i.e. a () -> <Sanic app> callable
    -s, --simple                   Run Sanic as a Simple Server, and serve the contents of a directory
                                   (module arg should be a path)

  Socket binding:
    -H HOST, --host HOST           Host address [default 127.0.0.1]
    -p PORT, --port PORT           Port to serve on [default 8000]
    -u UNIX, --unix UNIX           location of unix socket

  TLS certificate:
    --cert CERT                    Location of fullchain.pem, bundle.crt or equivalent
    --key KEY                      Location of privkey.pem or equivalent .key file
    --tls DIR                      TLS certificate folder with fullchain.pem and privkey.pem
                                   May be specified multiple times to choose multiple certificates
    --tls-strict-host              Only allow clients that send an SNI matching server certs

  Worker:
    -w WORKERS, --workers WORKERS  Number of worker processes [default 1]
    --fast                         Set the number of workers to max allowed
    --access-logs                  Display access logs
    --no-access-logs               No display access logs

  Development:
    --debug                        Run the server in debug mode
    -d, --dev                      Currently is an alias for --debug. But starting in v22.3, 
                                   --debug will no longer automatically trigger auto_restart. 
                                   However, --dev will continue, effectively making it the 
                                   same as debug + auto_reload.
    -r, --reload, --auto-reload    Watch source directory for file changes and reload on changes
    -R PATH, --reload-dir PATH     Extra directories to watch and reload on changes

  Output:
    --motd                         Show the startup display
    --no-motd                      No show the startup display
    -v, --verbosity                Control logging noise, eg. -vv or --verbosity=2 [default 0]
    --noisy-exceptions             Output stack traces for all exceptions
    --no-noisy-exceptions          No output stack traces for all exceptions

@ahopkins ahopkins mentioned this pull request Oct 30, 2021
@ahopkins
Copy link
Member Author

ahopkins commented Oct 30, 2021

@Tronic We discussed making --dev be INFO and not DEBUG. That does not really make sense with this implementation since (eventually) --dev and --debug will mean something different: --dev will be --debug + --auto-reload.

To compensate for that, I buried some of the noise with --verbosity. I think this is a more elegant solution and allows some decoupling of the running mode with the output logs.

@Tronic
Copy link
Member

Tronic commented Oct 30, 2021

Could you make the mode line display: mode: production, single worker or mode: production, goin' fast with {N} workers or something alike, avoiding the separate workers line? Maybe also auto-reload on the same line.

Also, the graphic will look bad on MacOS but I guess there is not much to be done about that.

@Tronic
Copy link
Member

Tronic commented Oct 30, 2021

And actually, replace the host and port lines with URLs and avoid the separate goin' fast message we have been printing.

Or a URL, for now... Or an UNIX socket. I think at some point we have to allow serving multiple IPs (like IPv4 and IPv6) from the same instance, meaning that there could be more than one URL.

@ahopkins
Copy link
Member Author

ahopkins commented Oct 31, 2021

@Tronic I was planning on changing the host/port like like you suggested because I knew I needed to support unix socket alternative. I agree about the multiple IPs, but that is a much larger PR since we really should think about how to have 1+ Sanic instances to 1+ bindings.

I incorporated your idea about bringing workers into the mode line. I think auto-reload should still be its own because I want to also enumerate and additional directories that are added. Perhaps the line is just omitted if there is no auto-reload.

Also, the graphic will look bad on MacOS but I guess there is not much to be done about that.

I plan to play around with it on Mac and Windows. If it looks terrible, then we can fallback to text based.

@ahopkins
Copy link
Member Author

ahopkins commented Oct 31, 2021

A couple more additions:

  • Displays versions of some Sanic libs if they are installed in the environment
  • Includes an API for adding arbitrary data to MOTD (Might be helpful for both app and plugin developers)

image

And the non-tty version:

[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] Sanic v21.12.0dev
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] Goin' Fast @ http://127.0.0.1:8000
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] mode: debug, goin' fast w/ 12 workers
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] server: sanic
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] python: 3.9.7
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] platform: Linux-5.14.12-arch1-1-x86_64-with-glibc2.33
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] auto-reload: enabled
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] sanic-routing: 0.7.1
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] sanic-testing: 0.7.0b2
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] sanic-ext: 21.9.0
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] foo: bar
[2021-10-31 11:10:42 +0200] [1097041] [DEBUG] hello: adam

@Tronic
Copy link
Member

Tronic commented Oct 31, 2021

Great! Now just drop one of the Goin' fast so that it is not needlessly repeated :)

I did not yet look at the verbosity changes but maybe still make (some of) these messages INFO rather than DEBUG?

@ahopkins
Copy link
Member Author

Great! Now just drop one of the Goin' fast so that it is not needlessly repeated :)

The "goin' fast" in the mode only shows up when you are using --fast. Also, I consolidated the packages to a single item:

packages: sanic-routing==0.7.1, sanic-testing==0.7.0b2, 
          sanic-ext==21.9.0                             

I did not yet look at the verbosity changes but maybe still make (some of) these messages INFO rather than DEBUG?

I am assuming you mean the non-TTY version. I am not sure about how best to handle this. On the one hand you are likely in a prod container. On the other, it is still nice to have the output in your logs. Maybe I'll leave these as INFO for now and we can think about that at a later point.

For now, the verbosity changes I made only touch the signals. But there probable are some other points we could use it.

@ahopkins
Copy link
Member Author

FYI - I confirmed on my Windows laptop that the color looks correct using:

I will test on OSX later today

@ahopkins ahopkins marked this pull request as ready for review November 1, 2021 09:36
@ahopkins ahopkins requested review from a team as code owners November 1, 2021 09:36
@ahopkins ahopkins marked this pull request as draft November 1, 2021 09:36
@ahopkins ahopkins changed the title Initial work on restructure of application state Restructure of CLI and application state Nov 1, 2021
@ahopkins ahopkins marked this pull request as ready for review November 3, 2021 21:55
sanic/app.py Show resolved Hide resolved
sanic/app.py Outdated Show resolved Hide resolved
sanic/app.py Outdated Show resolved Hide resolved
sanic/app.py Show resolved Hide resolved
sanic/app.py Show resolved Hide resolved
sanic/app.py Show resolved Hide resolved
tests/test_exceptions_handler.py Show resolved Hide resolved
@prryplatypus
Copy link
Member

Also, seem to be missing tests for the warning when setting config.LOGO.

@prryplatypus prryplatypus mentioned this pull request Nov 7, 2021
@ahopkins ahopkins requested review from prryplatypus and a team November 7, 2021 19:26
@codeclimate
Copy link

codeclimate bot commented Nov 7, 2021

Code Climate has analyzed commit fc40d78 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 86.3% (86% is the threshold).

This pull request will bring the total coverage in the repository to 86.6% (-0.1% change).

View more on Code Climate.

@ahopkins ahopkins merged commit 392a497 into main Nov 7, 2021
@ahopkins ahopkins deleted the sanic-state branch November 7, 2021 19:39
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
* Initial work on restructure of application state

* Updated MOTD with more flexible input and add basic version

* Remove unnecessary type ignores

* Add wrapping and smarter output per process type

* Add support for ASGI MOTD

* Add Windows color support ernable

* Refactor __main__ into submodule

* Renest arguments

* Passing unit tests

* Passing unit tests

* Typing

* Fix num worker test

* Add context to assert failure

* Add some type annotations

* Some linting

* Line aware searching in test

* Test abstractions

* Fix some flappy tests

* Bump up timeout on CLI tests

* Change test for no access logs on gunicornworker

* Add some basic test converage

* Some new tests, and disallow workers and fast on app.run
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.

Production and development modes (default workers)
3 participants