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

Add default log_config on uvicorn.run() #1541

Merged
merged 1 commit into from
Jun 24, 2022
Merged

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jun 24, 2022

I broke the logs because of this. 😞

I don't know how I didn't catch this... It's too simple...

Anyway, moment of shame here. This application doesn't show any logs:

import uvicorn
from fastapi import FastAPI

app = FastAPI() 
@app.get("/")
def index(): 
    return {"home": "index"}

if __name__==  "__main__": 
    uvicorn.run("main:app", port=8100)

The reason is that I've set the default of log_config to None, instead of LOGGING_CONFIG.

EDIT: The funny thing is that I noticed this behavior, and I thought the problem was the computer (mac)... 🤦

@Kludex Kludex requested review from euri10 and adriangb June 24, 2022 18:14
@adriangb
Copy link
Member

adriangb commented Jun 24, 2022

Was this the previous default? Where (in what commit) was it changed?

@@ -478,7 +478,7 @@ def run(
reload_delay: float = 0.25,
workers: typing.Optional[int] = None,
env_file: typing.Optional[str] = None,
log_config: typing.Optional[typing.Union[dict, str]] = None,
log_config: typing.Optional[typing.Union[dict, str]] = LOGGING_CONFIG,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log_config: typing.Optional[typing.Union[dict, str]] = LOGGING_CONFIG,
log_config: typing.Optional[typing.Union[typing.Mapping[str, Any], str]] = LOGGING_CONFIG,

At least this way we'll get an error from the type checker if we try to mutate the default argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to merge #1539 after this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it wouldn't make any difference for here...

@Kludex
Copy link
Member Author

Kludex commented Jun 24, 2022

Default values were added on: #1423

@adriangb
Copy link
Member

I see, so before we were just setting it to this if it wasn't in kwargs:

"log_config": LOGGING_CONFIG if log_config is None else log_config,

So I guess we just got it wrong in #1423

This is an easy 👍 then.

@adriangb
Copy link
Member

One last thing: should we remove Optional then? You either use the default mapping or you pass in a mapping. Or does None have a meaning now?

@Kludex
Copy link
Member Author

Kludex commented Jun 24, 2022

None always had a meaning. Which is to not set what is in the LOGGING_CONFIG i.e. just not set anything for loggers...

The mistake here was to set None as default on the run().

@adriangb
Copy link
Member

But before if you passed None you got LOGGING_CONFIG so None had not meaning:

log_config=LOGGING_CONFIG if log_config is None else log_config,

@Kludex
Copy link
Member Author

Kludex commented Jun 24, 2022

Via CLI it was never supposed to accept None. But via code, it was always allowed:

if self.log_config is not None:

@adriangb
Copy link
Member

Hmm okay ✅

@Kludex Kludex merged commit 0954e56 into master Jun 24, 2022
@Kludex Kludex deleted the fix-broken-log-config branch June 24, 2022 18:49
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Kludex added a commit that referenced this pull request Oct 29, 2022
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.

2 participants