-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[python] Allow to register custom logger in Python-package #3820
Conversation
b125f0d
to
e7e59d0
Compare
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 these two logger calls (this is the all logs we have from Dask so far) be replaced with new LightGBM/python-package/lightgbm/dask.py Line 28 in ac706e1
LightGBM/python-package/lightgbm/dask.py Line 260 in ac706e1
LightGBM/python-package/lightgbm/dask.py Line 263 in ac706e1
Please check this: dmlc/xgboost#6609 (comment).
|
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 really like this! To answer your question...yes I think this can and should be used by the code in dask.py
as well.
I left a suggestion for the a change to the API, though.
python-package/lightgbm/basic.py
Outdated
def _log(msg, is_warning=False): | ||
"""Output LightGBM's logs.""" | ||
if is_warning: | ||
_LOGGER.warning(msg) | ||
else: | ||
_LOGGER.info(msg) |
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.
instead of using a flag for the log level, I'd prefer functions _log_warn()
and _log_info()
. I think that makes the code easier to understand, and is easier to extend to other log levels (error, fatal, and debug). That is how most loggers are structured.
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 don't think that adding new arguments like is_debug
, is_fatal
is harder and less clear than writing new functions. I believe it's more a matter of taste. But I don't have any strong preferences, so I'm going to implement your suggestion.
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.
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.
looks great! Really nice addition
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Centralize all logging logic in one place.
Fixed #3805.
Related #3156.