add a logging alternative #80
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| add_subdirectory(util) | ||
|
There was a problem hiding this comment. please put it into csrc. No need to create a new directory. There was a problem hiding this comment. agreed RE no need for new directory. |
||
| add_subdirectory(csrc) | ||
| add_subdirectory(python) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
|
|
||
| #include <vector> | ||
|
|
||
| #include "k2/util/Logging.h" | ||
| #include "k2/csrc/fsa.h" | ||
|
|
||
| namespace k2 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include "k2/util/Logging.h" | ||
|
|
||
| namespace k2 { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
|
|
||
| #include <vector> | ||
|
|
||
| #include "k2/util/Logging.h" | ||
| #include "k2/csrc/fsa.h" | ||
|
|
||
| namespace k2 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
|
|
||
| #include <stdlib.h> | ||
|
|
||
| #include "k2/util/Logging.h" | ||
|
|
||
| namespace k2 { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
|
|
||
| #include "k2/python/csrc/tensor.h" | ||
|
|
||
| #include "k2/util/Logging.h" | ||
|
|
||
| namespace k2 { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
|
|
||
| #--------------------------------- non-glog (loguru) ------------------------------- | ||
| if(NOT K2_USE_GLOG) | ||
| add_library(non-glog | ||
| Logging.cc | ||
| loguru/loguru.cc) | ||
|
|
||
| target_include_directories(non-glog PUBLIC ${CMAKE_SOURCE_DIR}) | ||
|
|
||
| find_package(Threads) | ||
| target_link_libraries(non-glog ${CMAKE_THREAD_LIBS_INIT}) # For pthreads | ||
| if(NOT WIN32) | ||
| target_link_libraries(non-glog dl) # For ldl | ||
| endif() | ||
| endif() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // $FILE_PATHNAME $HEADER_FILENAME $HEADER_PATHNAME | ||
|
There was a problem hiding this comment. please replace this line with the one used by other k2 files. |
||
|
|
||
| // Copyright (c) 2020 Xiaomi Corporation ( authors: Meixu Song ) | ||
| // | ||
|
|
||
| // See ../../LICENSE for clarification regarding multiple authors | ||
|
|
||
| #include "k2/util/Logging.h" | ||
|
|
||
| #ifdef K2_USE_GLOG | ||
|
There was a problem hiding this comment. Why not to use glog directly? K2 is a training framework and I don't think There was a problem hiding this comment. Actually, k2 would be used in inference as well. There was a problem hiding this comment. For mobile is one thing, the other more important reason is about glog itself actually. After investigations, I found glog is not beautiful as I thought. Even the logging system itself is a trade-off/confusing concept, that would easily be noisy, expose too much global flags/macro, takes too much responsibility (exception/error handling). So in conclusion, I found there isn't the best practice. However, glog with one minimal glog-like logging is preferred by many great projects. A minimal glog could be very simple to just support minimal functions of glog. And loguru turn out to be the expected one. So, with loguru, the two-logger thought is implemented as this PR. |
||
| // Google glog's api does not have an external function that allows one to check | ||
| // if glog is initialized or not. It does have an internal function - so we are | ||
| // declaring it here. This is a hack but has been used by a bunch of others too. | ||
|
There was a problem hiding this comment. RE |
||
| namespace google { | ||
| namespace glog_internal_namespace_ { | ||
| bool IsGoogleLoggingInitialized(); | ||
| } // namespace glog_internal_namespace_ | ||
| } // namespace google | ||
|
|
||
| namespace k2 { | ||
| bool InitK2Logging(int* argc, char** argv) { | ||
| if (*argc == 0) | ||
| return true; | ||
| #if !defined(_MSC_VER) | ||
| // This trick can only be used on UNIX platforms | ||
| if (!::google::glog_internal_namespace_::IsGoogleLoggingInitialized()) | ||
| #endif | ||
| { | ||
| ::google::InitGoogleLogging(argv[0]); | ||
| #if !defined(_MSC_VER) | ||
| // This is never defined on Windows | ||
| ::google::InstallFailureSignalHandler(); | ||
| #endif | ||
| } | ||
| UpdateLoggingLevelsFromFlags(); | ||
| return true; | ||
| } | ||
|
|
||
| void UpdateLoggingLevelsFromFlags() { | ||
| // TODO(meixu): set some default FLAGS/gflags here | ||
| } | ||
| } // namespace k2 | ||
| #else // !K2_USE_GLOG | ||
| namespace k2 { | ||
| bool InitK2Logging(int *argc, char **argv) { | ||
| // When doing InitCaffeLogging, we will assume that caffe's flag parser has | ||
|
There was a problem hiding this comment. Why does the comment contain There was a problem hiding this comment. the argc,argv stuff may not be necessary as I don't anticipate using k2 much in command line programs. There was a problem hiding this comment. Emm, let me explain this. Before coding, I investigated pytorch, tf, bazel, and other projects that make use of glog, and many alternative logging methods. To find one best practice for logging, some common thought is used here. I try to keep minimal and non-specific to some project codes here. As here, it declares an internal function of glog, which is a common hack as it said, so I leave it as it is. However, this comment shouldn't be here. As the loguru here does parse the arg. The comment is removed now. To @danpovey : |
||
| // already finished. | ||
| if (*argc == 0) | ||
| return true; | ||
|
|
||
| loguru::init(*argc, argv); | ||
|
There was a problem hiding this comment. Name of functions should be in Uppercase. There was a problem hiding this comment. I guess the name There was a problem hiding this comment. Yes, it is from loguru. |
||
| return true; | ||
| } | ||
|
|
||
| void UpdateLoggingLevelsFromFlags() {} | ||
| } // namespace k2 | ||
| #endif // !K2_USE_GLOG | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // | ||
|
There was a problem hiding this comment. the first line of every file should be the path to the file. |
||
| // Created by songmeixu (songmeixu@outlook.com) on 2020/8/3. | ||
|
There was a problem hiding this comment. Please follow the header style of other files. There was a problem hiding this comment. @songmeixu And I think it's better if you declare your copyright under Xiaomi Corporation, as Dan and I did. There was a problem hiding this comment. Sorry, it's from my IDE old setting, and I forget it in some files. Changes have been made referring to the existing style. |
||
| // Copyright (c) 2020 Xiaomi Inc. All rights reserved. | ||
| // | ||
|
|
||
| #ifndef K2_UTIL_LOGGING_H_ | ||
| #define K2_UTIL_LOGGING_H_ | ||
|
|
||
| // Choose one logging implementation. | ||
| // All have the same common API of google/glog | ||
| #ifdef K2_USE_GLOG | ||
| #include "k2/util/logging_is_google_glog.h" | ||
| #else | ||
| #include "k2/util/logging_is_not_google_glog.h" | ||
| #endif | ||
|
|
||
| namespace k2 { | ||
|
|
||
| // Functions that we use for initialization. | ||
| bool InitK2Logging(int* argc, char** argv); | ||
| void UpdateLoggingLevelsFromFlags(); | ||
|
|
||
| constexpr bool IsUsingGoogleLogging() { | ||
| #ifdef K2_USE_GLOG | ||
| return true; | ||
| #else | ||
| return false; | ||
| #endif | ||
| } | ||
|
|
||
| } // namespace k2 | ||
| #endif // K2_UTIL_LOGGING_H_ | ||
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.
Why is this off by default? Since we install gtest by default, doesn't that already require glog?
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.
@songmeixu
when to enable this option?
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.
OFF is set for test, and help figure out what we need for logging that is outside a minimal one.
The gtest doesn't depend on glog, the former is for unit test, while the latter is for logging. Though the implementations are similar and very simple for some cases (*_EQ), I think there is a design principle to isolate these two.