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

logging basic configuration level: INFO vs. WARNING (usability with W&B) #710

Closed
colehurwitz opened this issue Jan 19, 2020 · 12 comments · Fixed by #1015
Closed

logging basic configuration level: INFO vs. WARNING (usability with W&B) #710

colehurwitz opened this issue Jan 19, 2020 · 12 comments · Fixed by #1015
Labels
good first issue Good for newcomers logger Related to the Loggers
Milestone

Comments

@colehurwitz
Copy link
Contributor

colehurwitz commented Jan 19, 2020

Thanks for the amazing package! I am having a great time using it.

Issue

Recently, I have been playing around with the weights and biases (W&B) logging functionality and I noticed that I was getting a lot of logging messages in my jupyter notebook while training my model (every epoch I got new messages).

When I looked into logging.__init__.py, the logging basic configuration was set with:

logging.basicConfig(level=logging.INFO)

However, that means a lot of INFO messages are being printed from W&B's use of the logger (e.g. when the RunManager modifies files).

Potential Solution

To disable this, I switched the logging basic configuration in logging.__init__.py to:

logging.basicConfig(level=logging.WARNING)

Would this be a useful addition in general to the package or should I just keep this change local?

@neggert
Copy link
Contributor

neggert commented Jan 21, 2020

Honestly, best practice is for libraries not to configure logging at all. That way the user can set it up however they want. When libraries configure logging, you get weird things like this.

logging.basicConfig(level=logging.DEBUG)
import pytorch_lightning.logging
# now log level is INFO :(

@williamFalcon thoughts on leaving logging configuration to the users?

@Borda
Copy link
Member

Borda commented Jan 21, 2020

the issue with logging is that it has to be sett by logging.basicConfig before fist usage, another option is to get the logger during a run and change the level... or we may think about using another logging package... let's have look at https://github.com/Delgan/loguru

@williamFalcon
Copy link
Contributor

@neggert i agree that we should leave up to user but this is something people will have to remember... so maybe we set a default one if there's no config already set @Borda ?

@Borda
Copy link
Member

Borda commented Jan 24, 2020

I will need to check it...

@Borda Borda self-assigned this Jan 24, 2020
@williamFalcon williamFalcon added this to the 0.6.1 milestone Feb 11, 2020
@Borda Borda removed their assignment Feb 18, 2020
@Borda Borda added the good first issue Good for newcomers label Feb 18, 2020
@neggert
Copy link
Contributor

neggert commented Feb 28, 2020

Okay, spent a little bit of time thinking about this. I think it's fine to setup the lightning logger for the user, but we shouldn't be touching other loggers (e.g. WandB). To give a little more visibility to the user, what do we think about adding another arg to Trainer?

Arg:
    log_level (str, optional): Log level for the pytorch_lightning logger. Should be one of
    "DEBUG", "INFO", "WARNING", "ERROR", or `None`. If set to `None`, no logging
    configuration will be done; the user must configure logging if they want something
    other than Python defaults. Defaults to "INFO".

Then in the code, we do something like

if log_level is not None:
    logging.getLogger("pytorch_lighting").setLevel(log_level)

@Borda @williamFalcon Any thoughts? Only downside I see is yet another argument to Trainer.

@cmpute
Copy link
Contributor

cmpute commented Feb 29, 2020

I think the best practice is actually letting the package use dedicated logger, just as @neggert wrote: use logging.getLogger("pytorch_lightning"). Messing up with root logger has created much trouble in my case...

@cmpute
Copy link
Contributor

cmpute commented Feb 29, 2020

It's fine if default level of dedicated logger is set, since it won't change the level of other loggers, while users can still change the logger level if they want

And also the loggers under /loggers package should use subloggers (like logging.getLogger('pytorch_lightning.neptune') or maybe shorter like logging.getLogger('lightning.neptune'). Thus the log_level can be inherited.

@Borda
Copy link
Member

Borda commented Mar 2, 2020

yeah, having different logging level per logger sounds good... on the other hand, experiment-like logger does not save any logging messages aka terminal...
We can freely remove setting logging level from package init I guess. just trying to remember what was the reason we place it there...
I think that the case was that the logging was used somewhere in lightning package in plane code (not inside a function) and then touching this logging without proper setting basicConfing caused that no logging messages were shown...

@Borda
Copy link
Member

Borda commented Mar 2, 2020

And also the loggers under /loggers package should use subloggers (like logging.getLogger('pytorch_lightning.neptune') or maybe shorter like logging.getLogger('lightning.neptune'). Thus the log_level can be inherited.

since #825 we support multiple loggers... The common practice is to have a WARNING level in a terminal and DEBUG in a file... What we could do is add a logger which would catch all logging messages like info/debug and write them to a special file... @cmpute is it something you are interested in?

@cmpute
Copy link
Contributor

cmpute commented Mar 4, 2020

And also the loggers under /loggers package should use subloggers (like logging.getLogger('pytorch_lightning.neptune') or maybe shorter like logging.getLogger('lightning.neptune'). Thus the log_level can be inherited.

since #825 we support multiple loggers... The common practice is to have a WARNING level in a terminal and DEBUG in a file... What we could do is add a logger which would catch all logging messages like info/debug and write them to a special file... @cmpute is it something you are interested in?

By "logging" I mean maybe the text logging (with logging python module) should go under certain namespace instead of using root logger, for better text logging management

@Borda
Copy link
Member

Borda commented Mar 4, 2020

Sounds interesting, @cmpute mind submit PR with your suggestion?

@cmpute
Copy link
Contributor

cmpute commented Mar 4, 2020

@Borda sure, I can have a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers logger Related to the Loggers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants