-
Notifications
You must be signed in to change notification settings - Fork 124
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
Allow to set loglevel #765
Allow to set loglevel #765
Conversation
I'm not completely convinced about taking over full log level responsibility into these methods. |
I do not really understand the point here. The idea is that in general in CLIMADA one can set the log level as desired. Except here in the unsequa module, where we decided to suppress by default more of the logging. Here I just want to give the user and developer the capabilities to set the log level to info or debug as it is possible for the rest of CLIMADA. But if there is a good reason not to let developers adjust the log level we can also remove this feature. |
| Here I just want to give the user and developer the capabilities to set the log level to info or debug as it is possible for the rest of CLIMADA. Yes, I can see that. What's bugging me is that the setting of log levels would not be done in the same way as in the rest of CLIMADA: with a method argument and not with a context manager. Too many cases for me /: |
I strongly disagree with the proposal in this PR. Users can set their own (local) log level with from climada.util import log_level
with log_level("ERROR"):
# Calculate uncertainty Setting custom log levels within your code circumvents what you actually want to achieve. If your code produces too many log messages by default, it might sense to reduce their severity instead (e.g., from log to debug or even to trace). |
Ok, I see. I am a bit surprised that this sparks such strong opinions :D @peanutfun : note that this is NOT the case for the unsequa module because we set the log level inside of the method directly. This overwrites your context manager. That is the reason to add the option in this function only. We could also remove entirely the setting of the log level. However, dealing with the logger is rather more advanced functionality, so we opted at the time to support the user with a default option to not log everything. This is reasonable for the unsequa module imho. Otherwise, one would almost for all cases need to use the logging context manager when running the unsequa module, which I think is sub-optimal. Note also that the logs are NOT created by the unsequa module, but the underlying hazard, exposures, impact functions, impact calc and cost benefit modules. |
@chahank with that in mind, I see how this PR came about ✌️ However, I dislike the decision of using an internal context manager for the log messages.
Then the log levels are not chosen correctly. Your task as programmer is to decide which message has which severity, keeping in mind that users do not want to be overwhelmed at the "info" level. The users' task is to decide which log level they prefer. So I suggest that this PR removes the internal context manager and overhauls the log levels of the unsequa module instead. Notice also that if you run into trouble with consistent logging for all levels, you have the option of querying the log level, and choosing different log formats depending on the current level. I did a similar thing in DORiE, see e.g. https://gitlab.dune-project.org/dorie/dorie/-/blob/master/dune/dorie/model/richards/richards.cc#L230 |
Thanks for the update! So, the logging comes not from unsequa, but from all the downstream modules which are called several 1000 times easily. The logging IS appropriate for say the I found it more convenient to give access to the user to change the logging level if desired, but I see that this is not unanimous. So, I will simply close this pull request. |
Changes proposed in this PR:
PR Author Checklist
develop
)PR Reviewer Checklist