add logging interface #60
Conversation
src/learn.jl
Outdated
| """ | ||
| log_plot!(logger, field::AbstractString, value) | ||
|
|
||
| Log a scalar value `value` to `field`. |
There was a problem hiding this comment.
Not sure if this has to be a scalar value? I think what it supports might depend on the logging backend...
There was a problem hiding this comment.
yeah, I think it is fair to remove the term "scalar" here, since it is a plot, not sure if we can consider it a scalar or not!
There was a problem hiding this comment.
ah rename to log_value!, again I do not think the word scalar allows us the flexibility we will probably have, we should update the LighthouseWandb docs accordingly as well
There was a problem hiding this comment.
I removed the word scalar!
There was a problem hiding this comment.
...I don't suppose there's a non-breaking way to rename LearnLogger to be TBLogger (or something that indicates its tb-ness)?
As in one of the comments below, I think we could make the LearnLogger deprecation path more obvious if we relocate all LearnLoggers in a single file (or single region), but that's up to you.
...ditto the comment about logging the evaluation row, that's up to you!
src/learn.jl
Outdated
| Logs a string event given by `value` to `logger`. | ||
| """ | ||
| log_event!(logger, value) | ||
|
|
||
| function log_event!(logger::LearnLogger, value) |
There was a problem hiding this comment.
Hrm, maybe we should move all of the LearnLogger-specific functions to their own file? Would make it extra clear what is specific to LearnLogger, and would make the "remove LearnLogger" PR as easy as "delete a file"....
There was a problem hiding this comment.
My thought is maybe we could do something like
function log_event!(logger, value)Because LWB is dispatching this for a different type of logger, and it makes sense for lighthouse to provide the fallback methods for the different logging actions
|
|
||
| See also [`log_line_series!`](@ref). | ||
| """ | ||
| log_plot!(logger, field::AbstractString, plot, plot_data) |
There was a problem hiding this comment.
...I really really wish we didn't have to log both the plot and the data in this function; that was an addition that we made as a tb workaround (we were plotting an image of the plotted data, but weren't otherwise serializing the data used to do the plotting) and in hindsight that should have been a separate function call.
If there's a way to back out of it now, we should consider it...could this be, e.g., an args... situation where LearnLogger can have additional args but other usage need not?
| log_plot!(logger, field::AbstractString, plot, plot_data) | |
| log_plot!(logger, field::AbstractString, plot, args...) |
There was a problem hiding this comment.
I think this runs the risk of switching loggers and then existing code breaking (since it relies on being able to pass some number of args); ideally, you should be able to switch loggers without any code changes. I agree about log_plot! not being a nice function- I think we should deprecate it, pointing towards log_line_series! instead, and then delete it in the next breaking release.
There was a problem hiding this comment.
yeah I agree with this, especially as we build out more plot logging functionalities we will probably want to deprecate
There was a problem hiding this comment.
Yeah, sounds good to me---log_line_series! and/or log_image!.
| """ | ||
| step_logger!(logger) = nothing | ||
|
|
||
|
|
||
| """ |
There was a problem hiding this comment.
okay, i'm not "allowed" to comment below this point, but I think we should specialize the below implementation of
log_evaluation_row! on LearnLogger so that special-casing spearman correlation doesn't have to happen by default---that is a very tb-specific logged item. E.g.,
function log_evaluation_row!(logger, field::AbstractString, metrics)
metrics_plot = evaluation_metrics_plot(metrics)
metrics_dict = _evaluation_row_dict(metrics)
log_plot!(logger, field, metrics_plot, metrics_dict)
return metrics_plot
end
or, better:
function log_evaluation_row!(logger, field::AbstractString, metrics)
metrics_plot = evaluation_metrics_plot(metrics)
log_plot!(logger, field, metrics_plot) # if we make the plot_data field optional
# optionally, could also then log each field of `metrics_plot` with `log_values!`
return metrics_plot
end
and
function log_evaluation_row!(logger::LearnLogger, field::AbstractString, metrics)
metrics_plot = evaluation_metrics_plot(metrics)
metrics_dict = _evaluation_row_dict(metrics)
log_plot!(logger, field, metrics_plot, metrics_dict)
if haskey(metrics_dict, "spearman_correlation")
sp_field = replace(field, "metrics" => "spearman_correlation")
log_value!(logger, sp_field, metrics_dict["spearman_correlation"].ρ)
end
return metrics_plot
end
There was a problem hiding this comment.
I don't really get why spearman is TB-specific; shouldn't either all loggers want it or none?
There was a problem hiding this comment.
I filed #64 to track this
src/learn.jl
Outdated
| """ | ||
| log_plot!(logger, field::AbstractString, value) | ||
|
|
||
| Log a scalar value `value` to `field`. |
There was a problem hiding this comment.
ah rename to log_value!, again I do not think the word scalar allows us the flexibility we will probably have, we should update the LighthouseWandb docs accordingly as well
src/learn.jl
Outdated
| Logs a string event given by `value` to `logger`. | ||
| """ | ||
| log_event!(logger, value) | ||
|
|
||
| function log_event!(logger::LearnLogger, value) |
There was a problem hiding this comment.
My thought is maybe we could do something like
function log_event!(logger, value)Because LWB is dispatching this for a different type of logger, and it makes sense for lighthouse to provide the fallback methods for the different logging actions
Co-authored-by: Hannah Robertson <hannahilea@users.noreply.github.com>
Closes #55
This does not change the default logger, it just documents the existing interface, and adds one more method to it:
step_logger!. I'll file a new issue for removing the default logger.