-
Notifications
You must be signed in to change notification settings - Fork 191
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 #478
Logging #478
Conversation
Hey @arcavaliere thanks for working on this! Just added "[WIP]" to the title as that's our convention for PRs that are still a work in progress. About your error, you did define I think something like this would work: import Logging: min_enabled_level
min_enabled_level(logger::ModelLogger) = logger.min_level or import Logging
Logging.min_enabled_level(logger::ModelLogger) = logger.min_level and the same goes for Might also be good to PS: Looks like your second account made the commits haha. We can fix this later. |
Thanks for that! I was able to get |
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
=========================================
- Coverage 67.72% 66.83% -0.9%
=========================================
Files 69 70 +1
Lines 1952 1978 +26
=========================================
Hits 1322 1322
- Misses 630 656 +26
Continue to review full report at Codecov.
|
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 looks like a promising start.
Can I suggest that we work together on improving both Base
, MicroLogging
and the ModelLogger
you have here so that we get something really solid?
I'm willing to put in some design work, what I've been missing is a solid use case to drive the improvements which are needed in Base
.
For LogState
— that's a purely internal implementation detail of Base.CoreLogging
and you cannot change it by design. However perhaps you refer to defining custom LogLevel
s? For that, if you can read JuliaLang/julia#33418 and contribute your use cases I'd be delighted.
import Logging, Dates | ||
export ModelLogger, shouldlog, min_enabled_level, catch_exceptions, handle_message | ||
|
||
struct ModelLogger <: Logging.AbstractLogger |
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.
Yes, this is the correct way to do this :-)
# @printf("i: %04d, t: %s, Δt: %s, wmax = %.1e ms⁻¹, wall time: %s\n", | ||
# model.clock.iteration, prettytime(model.clock.time), prettytime(wizard.Δt), | ||
# wmax(model), prettytime(walltime)) | ||
@info string("i: %04d, t: %s, Δt: %s, wmax = %.1e ms⁻¹, wall time: %s\n", | ||
model.clock.iteration, prettytime(model.clock.time), prettytime(wizard.Δt), | ||
wmax(model), prettytime(walltime)) |
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.
To keep the existing format you want @sprintf
here, not string
. string
concatenates the various values:
julia> string("%04d", 10)
"%04d10"
julia> using Printf
julia> @sprintf("%04d", 10)
"0010"
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.
That explains the literal %s
's that were getting printed!
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.
BTW, @info
allows you to attach arbitrary keywords which can preserve a lot of semantic information for later analysis rather than trying to cram all that information into a string and then parse it again later.
For example:
@info "Ocean wind mixing iteration" clock=model.clock Δt=wizard.Δt wmax=wmax(model) walltime progress=model.clock.time/end_time
MicroLogging
master can print progress bars when the progress
keyword is attached.
module_name = something(_module, "nothing") | ||
file_name = something(filepath, "nothing") | ||
line_number = something(line, "nothing") | ||
msg_timestamp = Dates.format(Dates.now(), "[dd/mm/yyyy HH:MM:SS]") |
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 how the system is meant to be used :-)
As you have found out, you can get timestamps for every message in your application by customizing the logging backend rather than messing with the @info
statements themselves.
I was just looking at that issue! I have a similar desire to define some custom logging macros tied to |
src/logger.jl
Outdated
# ------------------------------------------------------------------------------------- | ||
# Custom LogStates | ||
const Diagnostic = Logging.LogLevel(-500) # Sits between Debug and Info | ||
const Setup = Logging.LogLevel(500) |
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.
Custom LogLevel
s not LogState
s!
It's interesting that you've put these at different importance levels; that would certainly be consistent with them being regarded as custom log levels 👍
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.
Whoops! I'll clean that up.
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 looks perfect. I think we will have to think a bit about exactly what custom log levels we need. I think both Setup
(for logging the model setup + construction of key model types?) and Diagnostic
are excellent. We may also want something like Simulation
: Diagnostic
would print the results of diagnostics calculations; Simulation
would print more general information about the progress of a simulation even when no diagnostics are being calculated.
On the spectrum from Debug
to Info
we could have something like
Debug < Diagnostic < Simulation < Setup < Info
This of course doesn't have to be included in the PR here, but something to think about for the future.
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'm happy to add those additional levels and structure the levels in that order so we can have a baseline.
Hi all, sorry for being late to the game, just wanted to provide some hopefully-useful input here:
@c42f this suggestion could be directed more broadly to all This PR looks really great! |
Does it make sense to encapsulate the code added by this PR into a submodule of Ref #456. |
@glwagner thanks. Indeed I think it's worthwhile pursuing good logging for all climate-machine simulations (XRef #71 (comment), CliMA/ClimateMachine.jl#134) |
Hmmm, I don't know if many log levels will be that helpful. I guess for debug messages we'll want to have the file name and line number, but for user-facing info messages maybe we should remove the file name and line number and instead a custom log level like I think one place that logging can hugely improve user experience is in setting up large models, especially on the GPU, where you could be waiting for several minutes while all of Julia, CUDA, and Oceananigans compiles, model is being set up, memory is allocated, etc. We know it takes time but most users will think that something is wrong if Julia is silent for 5 minutes. In this context info messages can be useful and reassure the user that Oceananigans is working properly. It's well known that progress indicators improve user experience, e.g.: https://www.nngroup.com/articles/progress-indicators/ |
I'm not entirely sure either. It's worth considering whether the standard For general logging I've often wondered whether it would be helpful to have a
This is basically what the standard |
Hey @arcavaliere sorry I think I messed up your PR :( I was trying to get rid of a big file that invaded our git history by mistake (#509) but apparently because it rewrites git history, open pull requests become invalid so now you have a PR with 1000+ commits and 100+ merge conflicts haha. Might be better to open a new pull request. You also need to delete the old repository and I can help with opening a new pull request. Again, sorry about ruining your pull request. |
I don't think it's quite so drastic. It's possible to have two unrelated remotes cloned within the same git repository so you certainly should be able to fetch changes even if upstream is completely rewritten. I'd probably try squashing your logging changes into a single commit and then cherry picking that across to the newly rewritten master. (I'm not entirely sure that will work, but I think it should.) PRs are based on branch and repo names, so once you've sorted things out locally you should be able to force push to |
Looks like it worked; thanks for the write up! |
You're welcome! It looks like there's still some non-logging related changes in the github diff view though? |
I think I may have missed something when I committed the sync of the upstream to my repo. Should I revert the commit and take those out? |
It's a bit hard to tell what's gone wrong here at this point. I'd do something like
(By the way, this would probably be clearer for you if you'd made the original pull request off a branch rather than your master branch. Generally that's a good idea, especially if you want to do multiple PRs) |
Moves custom levels and macros to be above logger Adds level_to_string function Changes formatting of log messages
Sorry for the delay on this! This time I think I've corrected the PR's state. @c42f Thank you for the assistance! |
Adds documentation around log severity order Changes ModelLogger message format to move metadata to the right
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 looks great! @arcavaliere do you think it's ready to be merged? We should integrate your logger and start using it and improving it.
Sorry for letting this sit around!
I also added some minor comments about style as we follow this Julia style guide: https://github.com/jrevels/YASGuide
|
||
# ------------------------------------------------------------------------------------- | ||
# Custom LogLevels | ||
_custom_log_level_docs = """ |
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.
Out of curiousity, why assign the docstring to a variable?
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 was mimicking what was done Base/Logging for the macros.
https://github.com/JuliaLang/julia/blob/46ce4d79337bdd257ee2e3d2f4bb1c55ff0a5030/base/logging.jl#L132
file_name = something(filepath, "nothing") | ||
line_number = something(line, "nothing") | ||
msg_timestamp = Dates.format(Dates.now(), "[dd/mm/yyyy HH:MM:SS]") | ||
formatted_message = "$message --- $msg_timestamp $level_name $file_name:$line_number" |
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.
🚀
Adds additional LogLevels and updates severity values Adds export logger to module exports Updates code style to match project guidelines
Thanks for the feedback and no worries on the delay! I haven't been as active as I would have liked these past couple of months 😅 |
Awesome! I'll start using it during testing first as we have a ton of Hope you don't mind if we ping you and request your review if we modify the @glwagner Any objections to merging this PR? |
Not at all! I can look into any more infrastructural work that needs doing. I don't quite understand how the models work but I'm happy to help out where I can 😄 |
elseif level == Setup | ||
"Setup" | ||
elseif level == Logging.Warn | ||
"Warning" |
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.
By the way, you will have noticed how this way of using custom levels isn't really composable, and doesn't really work well with the default ConsoleLogger
provided by Base
.
Here's my current attempt to clean up that situation (comments welcome): JuliaLang/julia#33960
I'd like to define a custom logger for the logging the progress of models (ModelLogger). It's based on
Logging.SimpleLogger
and currently attempts to format@info
calls as:[dd/mm/yyyy HH:MM:SS] module source_file:line_number: message
.Once that is in place I'd like to define some custom LogStates and have ModelLogger handle those as well. Chiefly
@diagnostic
.I wired up one of the examples to use ModelLogger rather than printf.