Skip to content
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

Merged
merged 12 commits into from
Jan 24, 2021

Conversation

StrikerRUS
Copy link
Collaborator

Centralize all logging logic in one place.
Fixed #3805.
Related #3156.

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

Can these two logger calls (this is the all logs we have from Dask so far) be replaced with new _log() function?

logger = logging.getLogger(__name__)

logger.warning('Parameter tree_learner not set. Using "data" as default')

logger.warning('Parameter tree_learner set to %s, which is not allowed. Using "data" as default' % tree_learner)

Please check this: dmlc/xgboost#6609 (comment).

The logging module should be preferred over the simple print, since Dask has a good integration with the logging module. See related discussion at rapidsai/cuml#3350.

Copy link
Collaborator

@jameslamb jameslamb left a 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.

Comment on lines 62 to 67
def _log(msg, is_warning=False):
"""Output LightGBM's logs."""
if is_warning:
_LOGGER.warning(msg)
else:
_LOGGER.info(msg)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameslamb

yes I think this can and should be used by the code in dask.py as well.

Done in 48c4058.

instead of using a flag for the log level, I'd prefer functions _log_warn() and _log_info()

Done in 7247790.

Copy link
Collaborator

@jameslamb jameslamb left a 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

@StrikerRUS StrikerRUS merged commit b7ccdaf into master Jan 24, 2021
@StrikerRUS StrikerRUS deleted the log_python branch January 24, 2021 20:37
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging to Python Log
3 participants