-
Notifications
You must be signed in to change notification settings - Fork 454
Logger Destination Refactor #1416
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
Conversation
Why does the before have empty algo charts? |
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 a massive lift -- huge kudos! No comments on the actual implementation -- it looks the same as the last PR / previous review? so lgtm. Put a few minor comments otherwise.
I'm happy to approve post-tests looking good. Definitely want to make sure we test all the loggers before rolling it out. Maybe add some research folks who use the loggers more often to validate the usage pictures look good? Besides that, this looks good to merge for me.
As a side, it looks like the diff is weird and showing things from another PR I merged into dev. Maybe because this PR hasn't been merged up or something...
these were the traces that were getting logged as metrics |
I'll be OOO, but this lgtm. Not approving because some lint stuff is off, but the logic looks great. Thanks for dealing with some nasty merges |
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.
Thanks, a significant improvement in our logging API. I am OK with the documentation deferring to a different PR.
But had one significant concern wrt the API (noted in comment below), perhaps we can close offline.
fixed: error caused by passing step explicitly to
strange isort issue, where I have to manually reorder the imports in |
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.
LGTM. Can we open Jira for progressbarlogger logs?
I suspect this will have some issues we'll discover with internal testing, but no reason to hold this off any longer.
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.
As this is already reviewed, accepting to unblock.
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.
Made some comments here -- looking good! Using Request Changes to hold this PR until #1419 is merged.
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.
Approving to unblock now that #1419 is merged
🙌 |
This PR:
log_data
fromLoggerDestination
and its subclasseslog_metrics
, andlog_hyperparameters
toLoggerDestination
toFileLogger
,InMemoryLogger
,ProgressBarLogger
,WandBLogger
, andTensorboardLogger
log_traces
toFileLogger
data
,data_fit
,data_epoch
, anddata_batch
fromLogger
log_traces
,log_metrics
, andlog_hyperparameters
toLogger
log_data
calls withlog_metrics
,log_hyperparameters
, orlog_traces
callsdata_fit
calls withlog_hyperparameters
data_epoch
anddata_batch
calls withlog_metrics
LogLevel
stuff except for inlog_artifacts
Reformatting Examples
FileLogger
FileLogger
before:FileLogger
after:ProgressBarLogger
ProgressBarLogger
log to console before:ProgressBarLogger
log to console after:ProgressBarLogger
locally before:ProgressBarLogger
locally after (no change?):ProgressBarLogger
in a notebook before:ProgressBarLogger
in a notebook after (no change?):ProgressbarLogger
from remote logs:ProgressBarLogger
in remote interactive session:ProgressBarLogger
log_to_console
remote:WandBLogger
beforeWandBLogger
afterWandBLogger
when run on clusterTensorboardLogger
before:TensorboardLogger
after:Solves JIRA Issues:
CO-585
CO-916
CO-207
CO-838