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.

Sign up for GitHub

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

Jump to bottom

Add test to check if run() matches Config.__init__() #1545

Merged
merged 1 commit into from Jul 2, 2022
Merged

Add test to check if run() matches Config.__init__() #1545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions tests/test_main.py
View file
Open in desktop
@@ -1,3 +1,4 @@
import inspect
import socket
from logging import WARNING

Expand Down Expand Up @@ -107,3 +108,17 @@ async def app(scope, receive, send):
with pytest.raises(SystemExit) as exit_exception:
run(app, lifespan="on")
assert exit_exception.value.code == 3


def test_run_match_config_params() -> None:
config_params = {
key: repr(value)
for key, value in inspect.signature(Config.__init__).parameters.items()
if key not in ("self", "timeout_notify", "callback_notify")
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout_notify and callback_notify are not available via run(). Should `run() support it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably good to make a constant with those two values CONFIG_ONLY_PARAMS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout_notify and callback_notify are not available via run(). Should `run() support it?

certainly not imho callback_notify it is just there to let the gunicorn know the worker is still alive iirc every timeout_notify
those can be overridden if you are subclassing UvicornWorker, I wouldn't know how to do that with the CLI nor if it makes sense

Probably good to make a constant with those two values CONFIG_ONLY_PARAMS

why not yes, not opposed having them that way too

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certainly not imho callback_notify it is just there to let the gunicorn know the worker is still alive iirc every timeout_notify
those can be overridden if you are subclassing UvicornWorker, I wouldn't know how to do that with the CLI nor if it makes sense

Right. Thanks 🙏

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much benefit on the CONFIG_ONLY_PARAMS constant for a single test function. Is there?

I'll merge this, but I don't mind creating a PR to add that. 🙏

}
run_params = {
key: repr(value)
for key, value in inspect.signature(run).parameters.items()
if key not in ("app_dir",)
}
assert config_params == run_params
12 changes: 6 additions & 6 deletions uvicorn/config.py
View file
Open in desktop
Expand Up @@ -218,9 +218,9 @@ def __init__(
http: Union[Type[asyncio.Protocol], HTTPProtocolType] = "auto",
ws: Union[Type[asyncio.Protocol], WSProtocolType] = "auto",
ws_max_size: int = 16 * 1024 * 1024,
ws_ping_interval: Optional[float] = 20,
ws_ping_timeout: Optional[float] = 20,
ws_per_message_deflate: Optional[bool] = True,
ws_ping_interval: Optional[float] = 20.0,
ws_ping_timeout: Optional[float] = 20.0,
ws_per_message_deflate: bool = True,
Kludex marked this conversation as resolved.
Show resolved Hide resolved
lifespan: LifespanType = "auto",
env_file: Optional[Union[str, os.PathLike]] = None,
log_config: Optional[Union[Dict[str, Any], str]] = LOGGING_CONFIG,
Expand All @@ -231,7 +231,7 @@ def __init__(
debug: bool = False,
reload: bool = False,
reload_dirs: Optional[Union[List[str], str]] = None,
reload_delay: Optional[float] = None,
reload_delay: float = 0.25,
Kludex marked this conversation as resolved.
Show resolved Hide resolved
reload_includes: Optional[Union[List[str], str]] = None,
reload_excludes: Optional[Union[List[str], str]] = None,
workers: Optional[int] = None,
Expand All @@ -245,7 +245,7 @@ def __init__(
backlog: int = 2048,
timeout_keep_alive: int = 5,
timeout_notify: int = 30,
callback_notify: Callable[..., Awaitable[None]] = None,
callback_notify: Optional[Callable[..., Awaitable[None]]] = None,
ssl_keyfile: Optional[str] = None,
ssl_certfile: Optional[Union[str, os.PathLike]] = None,
ssl_keyfile_password: Optional[str] = None,
Expand Down Expand Up @@ -277,7 +277,7 @@ def __init__(
self.interface = interface
self.debug = debug
self.reload = reload
self.reload_delay = reload_delay or 0.25
self.reload_delay = reload_delay
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to accept None for this. The behavior is not changed with this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's cleaner but what if someone explicitly called it with reload_delay=None? The default value won't override it. Might be worth a mention in the changelog

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIC, we shouldn't check a behavior that it's not allowed by the input, considering the type hint. I mean, reload_delay annotation changed from Optional[float] to float on this PR.

But... As you've pointed out, this is a breaking change, and I don't want to add breaking changes on this PR. 😅

I'll revert this change, and adapt the run(). 🙏

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I thought about it... Yep, you're right. I'll follow the path you suggested and add a note on the changelog about this.

self.workers = workers or 1
self.proxy_headers = proxy_headers
self.server_header = server_header
Expand Down
27 changes: 14 additions & 13 deletions uvicorn/main.py
View file
Open in desktop
@@ -1,3 +1,4 @@
import asyncio
euri10 marked this conversation as resolved.
Show resolved Hide resolved
import logging
import os
import platform
Expand Down Expand Up @@ -455,33 +456,33 @@ def main(


def run(
app: typing.Union["ASGIApplication", str],
app: typing.Union["ASGIApplication", typing.Callable, str],
*,
host: str = "127.0.0.1",
port: int = 8000,
uds: typing.Optional[str] = None,
fd: typing.Optional[int] = None,
loop: LoopSetupType = "auto",
http: HTTPProtocolType = "auto",
ws: WSProtocolType = "auto",
http: typing.Union[typing.Type[asyncio.Protocol], HTTPProtocolType] = "auto",
ws: typing.Union[typing.Type[asyncio.Protocol], WSProtocolType] = "auto",
ws_max_size: int = 16777216,
ws_ping_interval: float = 20.0,
ws_ping_timeout: float = 20.0,
ws_ping_interval: typing.Optional[float] = 20.0,
ws_ping_timeout: typing.Optional[float] = 20.0,
ws_per_message_deflate: bool = True,
lifespan: LifespanType = "auto",
interface: InterfaceType = "auto",
debug: bool = False,
reload: bool = False,
reload_dirs: typing.Optional[typing.List[str]] = None,
reload_includes: typing.Optional[typing.List[str]] = None,
reload_excludes: typing.Optional[typing.List[str]] = None,
reload_dirs: typing.Optional[typing.Union[typing.List[str], str]] = None,
reload_includes: typing.Optional[typing.Union[typing.List[str], str]] = None,
reload_excludes: typing.Optional[typing.Union[typing.List[str], str]] = None,
reload_delay: float = 0.25,
workers: typing.Optional[int] = None,
env_file: typing.Optional[str] = None,
env_file: typing.Optional[typing.Union[str, os.PathLike]] = None,
log_config: typing.Optional[
typing.Union[typing.Dict[str, typing.Any], str]
] = LOGGING_CONFIG,
log_level: typing.Optional[str] = None,
log_level: typing.Optional[typing.Union[str, int]] = None,
access_log: bool = True,
proxy_headers: bool = True,
server_header: bool = True,
Expand All @@ -493,10 +494,10 @@ def run(
limit_max_requests: typing.Optional[int] = None,
timeout_keep_alive: int = 5,
ssl_keyfile: typing.Optional[str] = None,
ssl_certfile: typing.Optional[str] = None,
ssl_certfile: typing.Optional[typing.Union[str, os.PathLike]] = None,
ssl_keyfile_password: typing.Optional[str] = None,
ssl_version: int = int(SSL_PROTOCOL_VERSION),
ssl_cert_reqs: int = int(ssl.CERT_NONE),
ssl_version: int = SSL_PROTOCOL_VERSION,
ssl_cert_reqs: int = ssl.CERT_NONE,
ssl_ca_certs: typing.Optional[str] = None,
ssl_ciphers: str = "TLSv1",
headers: typing.Optional[typing.List[typing.Tuple[str, str]]] = None,
Expand Down