posit-dev / py-shiny

Shiny for Python
https://shiny.posit.co/py/
MIT License
1.31k stars 79 forks source link

Enable standard python logging #727

Open dstumbo opened 1 year ago

dstumbo commented 1 year ago

In closed issue #305, @jcheng5 advised against using standard python logging (see below). He comments that "we need to fix up our logging to use logging everywhere". Has this been done, can we now use standard logging? If not, perhaps we should open this as an issue so it can be prioritized and tracked. Sorry if this is already in process or done. Thanks!

         OK, I missed the fact that you were calling `logging.basicConfig`. I'd stop doing that for now--we need to fix up our logging to use `logging` everywhere, but for now you should just do this instead, as early as possible:
import sys
logfile = open('myapp.log', 'a', 1)
sys.stdout = logfile
sys.stderr = logfile

I'd do this before importing any other packages, so import errors will be logged.

Originally posted by @jcheng5 in https://github.com/posit-dev/py-shiny/issues/305#issuecomment-1214673651

hedsnz commented 1 month ago

As a simple reprex, with four log levels from the logging module:

from shiny import App, ui
import logging

logging.debug("DEBUG log")
logging.info("INFO log")
logging.warning("WARNING log")
logging.error("ERROR log")

app_ui = ui.page_fluid()

def server(input, output, session):
    pass

app = App(app_ui, server)

Starting the app via the terminal, only the ERROR and WARNING logs are output, regardless of the selected log level. That is, the --log-level argument of shiny run doesn't have any effect on logs that are generated by the application, rather than shiny/uvicorn.

shiny run app.py --log-level debug

I think ultimately this comes from the default uvicorn logging configuration. If I had to guess, perhaps this could be fixed by updating the log_config argument of uvicorn.run: https://github.com/posit-dev/py-shiny/blob/main/shiny/_main.py#L365.

davetapley commented 1 month ago

I'd love this and am happy to do a PR if maintainers agree?

Assuming it's just replacing all print with a log.info then I don't think it would be much work at all. Once you filter out docs, tests, etc. it's only 10 files.

Then uvicorn already uses logging, e.g.: https://github.com/encode/uvicorn/blob/fe3910083e3990695bc19c2ef671dd447262ae18/uvicorn/lifespan/on.py#L48

So could either allow passing a log config here: https://github.com/posit-dev/py-shiny/blob/023de32a1b9fb545bd8f38a54fe69c0e79746088/shiny/_main.py#L321

Or ensure uvicorn.config.LOGGING_CONFIG is set before calling run().

Thoughts?