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.

Sign up for GitHub

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

add a logging alternative #80

Closed
wants to merge 8 commits into from
Closed

add a logging alternative #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3839c3a
add a logging alternative
megazone87 Aug 3, 2020
84e93ce
Merge remote-tracking branch 'Dan/master' into mx
megazone87 Aug 3, 2020
b291ed3
fix style as cpplint advise
megazone87 Aug 3, 2020
154364e
continue above commit
megazone87 Aug 3, 2020
9ab00fb
changes after @csukuangfj review
megazone87 Aug 4, 2020
07a7774
cpplint style: change header guard
megazone87 Aug 4, 2020
882f44b
make cxx 11 as required
megazone87 Aug 4, 2020
ead061a
changes after reviews
megazone87 Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ cmake_minimum_required(VERSION 3.5 FATAL_ERROR)
project(k2)

set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON) # require c++ standard >= 11

option(BUILD_SHARED_LIBS "Whether to build shared or static lib" ON)

Expand All @@ -33,11 +35,16 @@ if(WIN32 AND BUILD_SHARED_LIBS)
set(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE)
endif()

# build options
option(K2_USE_GLOG "Flag that chooses logger from [GLOG, NON-GLOG (loguru)]" OFF)
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.


enable_testing()

list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake)
include(cpplint)
include(glog)
if(K2_USE_GLOG)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if(K2_USE_GLOG)
  include(glog)
endif()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed.

include(glog)
endif()
include(googletest)
include(pybind11)

Expand Down
46 changes: 24 additions & 22 deletions k2/csrc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,32 @@
add_subdirectory(util)

# please sort the source files alphabetically
add_library(fsa
arcsort.cc
aux_labels.cc
connect.cc
determinize.cc
fsa.cc
fsa_equivalent.cc
fsa_renderer.cc
fsa_util.cc
intersect.cc
properties.cc
rmepsilon.cc
topsort.cc
util.cc
weights.cc
)
set(FSA_SOURCES
arcsort.cc
aux_labels.cc
connect.cc
determinize.cc
fsa.cc
fsa_equivalent.cc
fsa_renderer.cc
fsa_util.cc
intersect.cc
properties.cc
rmepsilon.cc
topsort.cc
util.cc
weights.cc)

add_library(fsa ${FSA_SOURCES})

target_include_directories(fsa PUBLIC ${CMAKE_SOURCE_DIR})
if(CMAKE_VERSION VERSION_LESS 3.8)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

if(K2_USE_GLOG)
target_compile_definitions(fsa PUBLIC K2_USE_GLOG)
target_link_libraries(fsa PUBLIC glog)
else()
target_compile_features(fsa PUBLIC cxx_std_11)
target_link_libraries(fsa PUBLIC non-glog)
endif()
target_link_libraries(fsa PUBLIC glog)

function(k2_add_fsa_test name)
add_executable(${name} "${name}.cc")
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/arcsort.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util/logging.h"

namespace k2 {
void ArcSorter::GetSizes(Array2Size<int32_t> *fsa_size) const {
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/arcsort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util/logging.h"

namespace k2 {
/**
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/util/logging.h"

namespace k2 {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/aux_labels.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/fsa_util.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util/logging.h"

namespace {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/connect.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@
#include <unordered_map>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/fsa_util.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace k2 {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/dense_fsa.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace k2 {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/determinize.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/fsa_util.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace k2 {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/determinize.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/determinize_impl.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"
#include "k2/csrc/weights.h"

namespace k2 {
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/fsa.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/array.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you were going to get rid of the directory util/?

Copy link
Collaborator

Choose a reason for hiding this comment

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

mm.. there is more code than I realized, now wondering whether we should keep util/

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 expect more subdirs would be here, as k2 surely needs more function ((IDK) IO, bin ..) that should distribute more organized.

And I think the dlpack related codes under python/csrc/ may be better under csrc/dlpack as dlpack is not for pybind essentially (dlpack gives a way to transfer between data structures, as here we use it transfer k2.Tensor with torch.Tensor). Pytorch put it as from torch.utils.dlpack import to_dlpack in py API. I think we would need the class k2.Tensor from current python/csrc/tensor.h.

Anyway, I hold no opinion about the right project structure🤗. Changes happen fast. So, util/ is OK for now?


namespace k2 {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/fsa_equivalent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
#include <cstdint>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util/logging.h"
#include "k2/csrc/weights.h"

#ifndef K2_CSRC_FSA_EQUIVALENT_H_
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/fsa_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/connect.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/intersect.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/intersect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util/logging.h"

namespace k2 {
/**
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/rmepsilon.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
#include <unordered_map>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/rmepsilon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/determinize_impl.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util/logging.h"
#include "k2/csrc/weights.h"

namespace k2 {
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/topsort.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
#include <unordered_map>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/properties.h"
#include "k2/csrc/util.h"
#include "k2/csrc/util/logging.h"

namespace k2 {
void TopSorter::GetSizes(Array2Size<int32_t> *fsa_size) {
Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/topsort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
#include <utility>
#include <vector>

#include "glog/logging.h"
#include "k2/csrc/fsa.h"
#include "k2/csrc/util/logging.h"

namespace k2 {

Expand Down
2 changes: 1 addition & 1 deletion k2/csrc/util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

#include <stdlib.h>

#include "glog/logging.h"
#include "k2/csrc/util/logging.h"

namespace k2 {

Expand Down
15 changes: 15 additions & 0 deletions k2/csrc/util/CMakeLists.txt
View file
Open in desktop
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()
55 changes: 55 additions & 0 deletions k2/csrc/util/logging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// k2/csrc/util/logging.h

// Copyright (c) 2020 Xiaomi Corporation ( authors: Meixu Song )

// See ../../LICENSE for clarification regarding multiple authors

#include "k2/csrc/util/logging.h"

#ifdef K2_USE_GLOG
// 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.
// (e.g. Torch).
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's better if you add the corresponding code link of PyTorch? (and I don't know what Torch here is, sorry)

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) {
if (*argc == 0)
return true;

loguru::init(*argc, argv);
return true;
}

void UpdateLoggingLevelsFromFlags() {}
} // namespace k2
#endif // !K2_USE_GLOG
33 changes: 33 additions & 0 deletions k2/csrc/util/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// k2/csrc/util/logging_is_google_glog.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

filename wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This filename is referred from pytorch, and I think it's clear (TF use default/logging.h (customized minimal glog) and google/build_config/logging.h(glog) to distinguish each other.).


// Copyright (c) 2020 Xiaomi Corporation (authors: Meixu Song)

// See ../../LICENSE for clarification regarding multiple authors

#ifndef K2_CSRC_UTIL_LOGGING_H_
#define K2_CSRC_UTIL_LOGGING_H_

// Choose one logging implementation.
// All have the same common API of google/glog
#ifdef K2_USE_GLOG
#include "k2/csrc/util/logging_is_google_glog.h"
#else
#include "k2/csrc/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_CSRC_UTIL_LOGGING_H_
Loading