change print to logging #457
Conversation
Borda
left a comment
Borda
left a comment
There was a problem hiding this comment.
just an author reminder #282 (comment) :)
| @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.
this is not very saved with self.logger = None in LightningModule
There was a problem hiding this comment.
ah indeed. in that case exposing the logger is becoming much harder 🌚
There was a problem hiding this comment.
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.
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.
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.
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... :]
| 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.
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.
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.
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. |
3 participants
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
printtologging.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)