Assertions etc. #78
Comments
|
GLOG (we have used it in our code) supports |
|
Mm. Can you tell me what it will look like in user code, to do assertions,
errors, etc.?
…On Fri, Jul 31, 2020 at 5:14 PM Haowen Qiu ***@***.***> wrote:
GLOG (we have used it in our code) supports Severity Level (INFO, WARNING,
ERROR, and FATAL). However, I think maybe we should wrap it into a custom
class as we may need to handle some exceptions from other frameworks (e.g.
PyTorch, TensorFlow) or pass errors (from our assertions) as
exceptions/error_code to other frameworks?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-667022918>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO4VFJVTNZD3M7RFTTDR6KDRFANCNFSM4PPR77MQ>
.
|
|
and sorry, by "user code" I really mean k2 c++ code
…On Fri, Jul 31, 2020 at 11:22 PM Daniel Povey ***@***.***> wrote:
Mm. Can you tell me what it will look like in user code, to do
assertions, errors, etc.?
On Fri, Jul 31, 2020 at 5:14 PM Haowen Qiu ***@***.***>
wrote:
> GLOG (we have used it in our code) supports Severity Level (INFO, WARNING,
> ERROR, and FATAL). However, I think maybe we should wrap it into a
> custom class as we may need to handle some exceptions from other frameworks
> (e.g. PyTorch, TensorFlow) or pass errors (from our assertions) as
> exceptions/error_code to other frameworks?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/danpovey/k2/issues/78#issuecomment-667022918>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZFLO4VFJVTNZD3M7RFTTDR6KDRFANCNFSM4PPR77MQ>
> .
>
|
|
Well, with https://github.com/kaldi-asr/kaldi/blob/master/src/base/kaldi-error.h#L164-L165 |
|
It seems to be an open issue in glog
google/glog#555
and google/glog#534 has a suggestion to resolve
it.
OK, what I'll do for now is write code like what you suggested and hope for
a fix later on.
But in kernels I'll continue to use assert(), as I'm guessing glog won't
work in kernels.
I'm not sure if asserts give line numbers but I suppose we can fix it
later.
…On Sat, Aug 1, 2020 at 12:20 PM Haowen Qiu ***@***.***> wrote:
Well, with GLOG, we can write code like this CHECK_GE(arc.label, -1) <<
"Arc label must be greater than -1";, i.e. assertion with logs. But we
definitely need to wrap it to add more informations (e.g. file name,
function name, line number, etc.), maybe some assertion mechanism like
Kaldi does?
https://github.com/kaldi-asr/kaldi/blob/master/src/base/kaldi-error.h#L164-L165
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-667466298>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOYYUZNOKEHHRZ5WLLTR6OJ2LANCNFSM4PPR77MQ>
.
|
|
Not sure whether the debug macros from https://github.com/NVlabs/cub/blob/master/cub/util_debug.cuh are useful. |
|
Mm, might be in CUDA code. But we need a more generic mechanism that is
usable in code that has nothing to do with CUDA.
…On Sat, Aug 1, 2020 at 2:59 PM Fangjun Kuang ***@***.***> wrote:
Not sure whether the debug macros from
https://github.com/NVlabs/cub/blob/1.8.0/cub/util_debug.cuh are useful.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-667484887>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOZRRIHLGU2NVEROBGLR6O4LTANCNFSM4PPR77MQ>
.
|
|
In that case, the following files from bazel might be a good start: |
|
Mm. It seems plausible to me (although I don't know what IsOk(..) is ...
perhaps some generic template that checks whether objects are OK?
I might not want to use that, seems too automagical for my taste.
Any thoughts, Haowen or Meixu?
…On Sat, Aug 1, 2020 at 3:26 PM Fangjun Kuang ***@***.***> wrote:
In that case, the following files from bazel might be a good start:
-
https://github.com/bazelbuild/bazel/blob/master/src/main/cpp/util/logging.h
-
https://github.com/bazelbuild/bazel/blob/master/src/main/cpp/util/logging.cc
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-667488088>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOYADJFYMMICNLCIAFDR6O7STANCNFSM4PPR77MQ>
.
|
|
We do not need |
|
Hi guys, I would like to do this. |
|
Great!
I guess we'll have something like K2_CHECK_EQ and so on? For less
problematic integration with other software?
Google style guide requires macros to have a project-specific prefix like
that.
…On Sat, Aug 1, 2020 at 4:47 PM Meixu Song ***@***.***> wrote:
Hi guys, I would like to do this.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-667497300>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO5JFZ642ZQ7TW7ALPDR6PJCLANCNFSM4PPR77MQ>
.
|
Whether or not the API is like this, is related to how we do this. With glog, keep the same API is necessary, to make use another customized logger. The pytorch and tf both implement a warpper of glog (to be more convenient or detailed). https://github.com/pytorch/pytorch/blob/2912390662c3c50732add54c94ac9b98d17e1cba/c10/util/Logging.h#L24 They have glog as one preferred logger, and a customized one with same API as glog. One is chosen based on macros (related to the platform, glog availability, ..) Thoughts:
|
|
The pytorch link doesn't work. So you are saying the interface should be
like CHECK_EQ and so on?
I'm concerned that this will increase the risk of difficulties if we later
need to #include external toolkits, like we will do for PyTorch.
…On Sat, Aug 1, 2020 at 7:58 PM Meixu Song ***@***.***> wrote:
K2_CHECK_EQ
Whether or not the API is like this, is related to how we do this. With
glog, the same API is needed to keep a customized logger optional.
------------------------------
The pytorch and tf both implement a warpper of glog (to be more convenient
or detailed).
https://github.com/pytorch/pytorch/blob/2912390662c3c50732add54c94ac9b98d17e1cbac10/util/Logging.h#L24
https://github.com/tensorflow/tensorflow/blob/44597e39fb9e6eeb614d2119d516c0d4ede084cc/tensorflow/core/platform/logging.h#L22
They have glog as one preferred logger, and a customized one with same API
as glog. One is chosen based on macros (related to the platform, glog
availability, ..)
------------------------------
Thoughts:
-
To avoid the macro or version conflict that glog may introduce, and
consider the lightweight needing currently (w/o file rotation, concurrence,
performance), I would advise a customized one like KALDI_ASSERT
implementation, or lightweight/header-only ones (e.g. pytorch/tf
customization, or https://github.com/gabime/spdlog).
-
And in our case that is to make a cpp extension for pytorch, why not
just use what we need from the customized logging header of pytorch for
now. By this way, we keep glog around, and could extend it in two similar
customized headers:
- one for cover the common API of glog
https://github.com/pytorch/pytorch/blob/2912390662c3c50732add54c94ac9b98d17e1cba/c10/util/logging_is_not_google_glog.h
- the other to check a macro, include glog, and extension API
independent with the macro.
https://github.com/pytorch/pytorch/blob/2912390662c3c50732add54c94ac9b98d17e1cba/c10/util/Logging.h
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-667520359>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOZ2WP4DWSKHNDZVKKTR6P7P3ANCNFSM4PPR77MQ>
.
|
|
Yes. Anyway, let me give a PR first to talk about it. |
|
Hi, there is a PR #80 for this issue. Please review it. To be clear, I want to explain why to add a new logger as an alternative?
|
|
Is loguru popular and used by many other projects? I may agree with fangjun here as I feel it's too heavy to involve two log libraries in our project. We can support mobile platforms later if we find we really need to support it (of course we need to enclose third-party log libraries into interfaces with names like |
|
I agree that probably glog is enough and loguru not needed.
Incidentally, we will need a solution for checks inside CUDA kernels. cub
has its own macros for that, I may use that although IDK if it
provides a stack trace. What you can access from CUDA is limited, although
printf does work.
…On Tue, Aug 4, 2020 at 1:00 PM Haowen Qiu ***@***.***> wrote:
Is loguru <https://github.com/emilk/loguru> popular and used by many
other projects? I may agree with fangjun here as I feel it's too heavy to
involve two log libraries in our project. We can support mobile platforms
later if we find we really need to support it (of course we need to enclose
third-party log libraries into interfaces with names like K2_, so that we
can change the log implementations easily in the future, without changing
the calling code).
Anyway, I think writing some code based on glog to support some extra
information (e.g. filename, function name, line-number, etc.) would be good
enough to our current requirements.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-668380130>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLOZRBDLIFHXPT5SZV6TR66IWTANCNFSM4PPR77MQ>
.
|
Do these already in the logging of glog? |
|
Mm. It may not even be possible to get a stack trace with line numbers all
the way down. (IDK). Anyway prob. not necessary.
Doesn't have to be perfect, anyway.
Actually I'm OK to use CUB asserts in any CUDA code. It may not be ideal
but it's simple and should work fine.
…On Tue, Aug 4, 2020 at 3:55 PM Meixu Song ***@***.***> wrote:
e.g. filename, function name, line-number, etc.
Do these already in the logging of glog?
Glog doesn't support is output these in the stack trace. But the exact
position is not important I think, as the module name is enough to locate
the problem(?).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-668443349>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO5PTQ7T2XL7XKJIC6DR665IHANCNFSM4PPR77MQ>
.
|
|
Emm, the problem just is glog is more than enough, as it introduce too much than needed. |
|
ah OK.
…On Tue, Aug 4, 2020 at 3:58 PM Meixu Song ***@***.***> wrote:
Emm, the problem just is glog is more than enough, as it introduce too
much than needed.
Loguru can act as a minimal glog. Which is also customized implemented by
many projects to prevent glog problems(the real heavy thing).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-668444918>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO633A7TL7XR4WT3HT3R665UFANCNFSM4PPR77MQ>
.
|
I think this is not a problem, we just can choose what we need and use only those functions/Macros. Compared with |
Is this a problem for K2? If so, what problems are there of using glog in K2? How does loguru solve them? I've never heard of loguru. Could you please list some open source projects using loguru? |
|
My feeling is (sorry Meixu!) that we can wait till we actually have a
problem and at that time do the loguru stuff, but for now just using glog
would
be fine... with some wrappers to make it K2_CHECK_EQ etc. for future
compatibility.
…On Tue, Aug 4, 2020 at 5:21 PM Fangjun Kuang ***@***.***> wrote:
Emm, the problem just is glog is more than enough, as it introduce too
much than needed.
Is this a problem for K2?
If so, what problems are there of using glog in K2?
How does loguru solve them?
------------------------------
I've never heard of loguru. Could you please list some open source
projects using loguru?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/danpovey/k2/issues/78#issuecomment-668486166>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFLO42CLIJY335FTFYZP3R67HIZANCNFSM4PPR77MQ>
.
|
Guys, I am fine but have to make my point after investigating it. Sorry for my poor expression😅. Add a K2_* alias define of glog doesn't help. I explain here: Emm, I feel it's worthy to face glog thing this earlier. I have encounter glog problems at previous engineering work. More disaster is protobuf, which is itself version incompatible. |
Guys,
For assertions I'm just using assert(...), but I'm not sure if this is a good practice?
I think we should decide how to do assertions and so on. It might make sense to have different level of assert
for things that are inside loops vs. things that won't slow the code down. And maybe errors and warnings too?
Any suggestions?
The text was updated successfully, but these errors were encountered: