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

Do not configure python logging #1503

Closed
rmrao opened this issue Apr 15, 2020 · 10 comments · Fixed by #1718
Closed

Do not configure python logging #1503

rmrao opened this issue Apr 15, 2020 · 10 comments · Fixed by #1718
Assignees
Labels
bug Something isn't working help wanted Open to be worked on

Comments

@rmrao
Copy link
Contributor

rmrao commented Apr 15, 2020

🐛 Bug

pytorch-lightning right now configures the python logging module (here). This is generally not recommended when writing a library as it makes it difficult for users to modify logging format (see python docs, Stack Overflow post). I would suggest deleting the configuration line.

@rmrao rmrao added bug Something isn't working help wanted Open to be worked on labels Apr 15, 2020
@hadim
Copy link
Contributor

hadim commented Apr 15, 2020

I agree. Logging configuration should be disabled by default. We could still set up the logging configuration manually just after importing PL.

@Borda
Copy link
Member

Borda commented Apr 15, 2020

we had the discussion here #1267 (comment) @jeremyjordan ^^

@rmrao
Copy link
Contributor Author

rmrao commented Apr 15, 2020

Hmm... ok. In that case, could I potentially suggest that the call to basicConfig be placed in Trainer's __init__? Otherwise you require the user to configure logging before importing pytorch lightning. There are a couple problems with this:

  1. imports should generally be placed at the top of a file. Configuring logging before importing pytorch-lightning means users have to violate PEP8 at some point.
  2. It's even more difficult when using DDP, since it's not possible to configure the logging for a DDP process before importing pytorch lightning.

@jeremyjordan
Copy link
Contributor

We could still set up the logging configuration manually just after importing PL.

@hadim isn't that what we do now? we configure logging when pytorch-lightning is imported

could I potentially suggest that the call to basicConfig be placed in Trainer's init?

@rmrao i'm not sure this would help you since we import Trainer in the project's root __init__.py

however, if you configure logging as you wish in your project's root __init__.py, by the time lightning is imported the root handler will already be configured and basicConfig will not run.

tagging @williamFalcon since we had this discussion here already. would prefer not to keep going back and forth on how we configure logging for the project.

@Borda
Copy link
Member

Borda commented Apr 16, 2020

We could still set up the logging configuration manually just after importing PL.

@hadim isn't that what we do now? we configure logging when pytorch-lightning is imported

now it sets the level for all logging not just for this lightning logger, that is what I was talking before...

@jeremyjordan
Copy link
Contributor

just doing something like

logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)

doesn't actually set up a handler to send the log messages.

however, we could just simply add a line to configure a stream handler

logger = logging.getLogger(__name__)
logger.addHandler(logging.StreamHandler())
logger.setLevel(logging.INFO)

@rmrao
Copy link
Contributor Author

rmrao commented Apr 28, 2020

@jeremyjordan my suggestion is actually to put basicConfig inside of the Trainer's __init__ function. This would only call basicConfig when an instance of Trainer is created (not when it's imported). You could even include the log level as an argument to Trainer.

@jeremyjordan
Copy link
Contributor

@rmrao ah i see, i misunderstood your suggestion. i think that's a fine solution

@fjhheras
Copy link
Contributor

fjhheras commented May 1, 2020

I was having problems because of this. It would duplicate my log messages.

I understand this will be solved soon, but in case anyone has the same problem, this is how I circumvented it. When first defining the root logger, I do:

    logger = logging.getLogger()
    # if there is already a handler, remove it.
    if logger.handlers:
        logger.handlers.pop()

(logging is the python logging package, not pl's)

@FrancescoSaverioZuppichini

same here, how do I use a logger from logging?

lucmos added a commit to grok-ai/nn-template that referenced this issue Jan 8, 2022
Flegyas pushed a commit to grok-ai/nn-template that referenced this issue Jan 8, 2022
* Disable hydra logging auto-configuration

* Add logging pretty printing

* Force logging configuration before pytorch lightning

See Lightning-AI/pytorch-lightning#1503

* Enable exceptions pretty printing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants