-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
change print to logging #457
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just an author reminder #282 (comment) :)
@@ -214,17 +214,17 @@ def __dataloader(self, train): | |||
|
|||
@pl.data_loader | |||
def train_dataloader(self): | |||
print('training data loader called') | |||
self.logger.info('training data loader called') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not very saved with self.logger = None
in LightningModule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah indeed. in that case exposing the logger is becoming much harder 🌚
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may try change self.logger = logging.getLogger()
'Early stopping conditioned on metric `%s` ' | ||
'which is not available. Available metrics are: %s' % | ||
(self.monitor, ','.join(list(logs.keys()))), RuntimeWarning) | ||
warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warnings.warn
and logging.warning
are a completely different stream with different consequences so please be sure that you want this behaviour... You may consider also raise it as Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I'm not familiar with this, but I noticed that the previous code used warnings.warn a lot, I thought this was kind of best-practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that it was related to using print
, in such case, it is better to warnings
I do not say that is wrong, in this case it is good, just to be aware of it... :]
@@ -142,7 +143,7 @@ def on_epoch_end(self, epoch, logs=None): | |||
|
|||
def on_train_end(self, logs=None): | |||
if self.stopped_epoch > 0 and self.verbose > 0: | |||
logging.info('Epoch %05d: early stopping' % (self.stopped_epoch + 1)) | |||
logging.info(f'Epoch {self.stopped_epoch + 1:05d}: early stopping') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not directly to this line, but we shall be sure that all this new logging is tested (covered by tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This did occur to me, but I didn't know how to test this logging, any idea on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually tested that the message does not crash the code...
A summary of this PR:
oh and btw, I thought it might be better to leave 'custom logging function' for some other day in the future. At this moment, I don't feel like adding a property to trainer indicating logging.info . The logging conventions in the project seems not yet settled down. |
amazing! @Borda let's do a different PR/issue for test coverage. |
Before submitting
What does this PR do?
Fixes #282 (issue).
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃
Note
I changed
print
tologging.info
. And added some simple support for Custom Logger to override the logging to some custom logging functions.However, many logging functions dont have access to trainer.logger at this moment, so I leaved them untouched.(for example the callback has test_tube_logger, some mixin functions have unclear direct access to logger. In these cases, I let them use logging.info directly instead of using a more robust self.logger.info)