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

Unify logging facilities. #3982

Merged
merged 8 commits into from
Dec 14, 2018
Merged

Unify logging facilities. #3982

merged 8 commits into from
Dec 14, 2018

Conversation

trivialfis
Copy link
Member

  • Enhance ConsoleLogger to handle different verbosity.
  • Override macros from dmlc.
  • Refactor cli_main to share common variables.

This is an attempt to unify some parameters, specifically debug_verbose and silent picked up from various places. If the method is approved, we can move on to some more dangerous and messy parameters like n_gpus and gpu_id.

@trivialfis
Copy link
Member Author

One motivation is to honor the parameter silent and debug_verbose automatically, preventing un-guarded logging.

@codecov-io
Copy link

Codecov Report

Merging #3982 into master will increase coverage by 0.2%.
The diff coverage is 71.07%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #3982     +/-   ##
===========================================
+ Coverage     56.35%   56.55%   +0.2%     
  Complexity      205      205             
===========================================
  Files           186      187      +1     
  Lines         14771    14824     +53     
  Branches        498      498             
===========================================
+ Hits           8324     8384     +60     
+ Misses         6208     6201      -7     
  Partials        239      239
Impacted Files Coverage Δ Complexity Δ
src/tree/param.h 86.34% <ø> (-0.38%) 0 <0> (ø)
src/common/math.h 100% <ø> (ø) 0 <0> (ø) ⬇️
src/data/data.cc 75.13% <ø> (ø) 0 <0> (ø) ⬇️
src/gbm/gblinear.cc 12.34% <0%> (-2.12%) 0 <0> (ø)
src/tree/updater_quantile_hist.cc 33.82% <0%> (+0.08%) 0 <0> (ø) ⬇️
src/gbm/gbtree.cc 17.05% <0%> (-1.62%) 0 <0> (ø)
src/learner.cc 26.43% <100%> (-0.58%) 0 <0> (ø)
tests/cpp/test_logging.cc 100% <100%> (ø) 0 <0> (?)
src/linear/updater_coordinate.cc 100% <100%> (ø) 0 <0> (ø) ⬇️
include/xgboost/logging.h 85% <100%> (+10%) 0 <0> (ø) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93f9ce9...1f491eb. Read the comment docs.

Copy link
Member

@RAMitchell RAMitchell left a comment

Choose a reason for hiding this comment

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

Are the changes in CLI main necessary? It will be very difficult to review and prove its correctness as I don't think we even have tests for this.

Please check the monitor class log statement is the correct type.

I think there is a change in behaviour here, before if I set debug_verbose = 1 I would get the timing information for gpu_hist and hist. Now I would have to set this at 2 to get the same information, is this correct?

src/gbm/gblinear.cc Outdated Show resolved Hide resolved
@trivialfis
Copy link
Member Author

@RAMitchell Thanks for the review. The debug_verbose actually is an un-documented internal parameter, somehow people started using it, which reminds me that I should update the doc.

Yes, separated test for cli should be added.

* Enhance `ConsoleLogger` to handle different verbosity.
* Override macros from `dmlc`.
* Don't use specialized gamma when building with GPU.
@trivialfis
Copy link
Member Author

@RAMitchell

Are the changes in CLI main necessary? It will be very difficult to review and prove its correctness as I don't think we even have tests for this.

I stripped the changes in CLI into minimum. Tests for CLI will be postponed for another PR.

Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

Thank you for improving the logging system. I like how you can set the warning level uniformly across all the program. Good job.

include/xgboost/logging.h Outdated Show resolved Hide resolved
@@ -68,13 +119,34 @@ class LogCallbackRegistry {

using LogCallbackRegistryStore = dmlc::ThreadLocalStore<LogCallbackRegistry>;

// Redefines LOG_WARNING for controling verbosity
#if defined(LOG_WARNING)
#undef LOG_WARNING
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is perfectly fine for now, but in the long term, we should port this improvement to DMLC-Core, so that we can get fine-grained warning level for all projects using DMLC-Core.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have some fears of changing dmlc-core. I looked at tvm and treelite a few times, but that's all. So when the port is needed, please let me know, I will need some help. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful to begin discussion for better logging system in TVM and MXNet. Already, there is a talk of better error handling: apache/tvm#2279.

@trivialfis
Copy link
Member Author

@hetong007 @khotilov Could you please help taking a look and deprecating silent in R package?

@trivialfis
Copy link
Member Author

@CodingCat I'm deprecating the silent parameter, currently still working, but could you help looking at the JVM package? I will take care of python.

@trivialfis trivialfis merged commit e0a2791 into dmlc:master Dec 14, 2018
@trivialfis trivialfis deleted the unify-logging branch December 14, 2018 12:37
@trivialfis trivialfis mentioned this pull request Dec 15, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants