areusch commented on a change in pull request #9012:
URL: https://github.com/apache/tvm/pull/9012#discussion_r710243501
##########
File path: include/tvm/runtime/logging.h
##########
@@ -409,6 +416,68 @@ inline bool DebugLoggingEnabled() {
return state == 1;
}
+/*! \brief Helpers for \p VerboseLoggingEnabled. Exposed for unit testing
only. */
+std::unordered_map<std::string, int> ParseTvmLogDebugSpec(const char*
opt_spec);
+bool VerboseEnabledInMap(const std::string& filename, int level,
+ const std::unordered_map<std::string, int>& map);
+
+/*!
+ * \brief Returns true if a VLOG statement in \p filename is enabled by the \p
TVM_LOG_DEBUG
+ * environment variable for logging at verbosity \p level.
+ *
+ * Filenames are canonicalized to be w.r.t. the src/ dir of the TVM tree.
(VLOG's should not
+ * appear under include/).
+ *
+ * To enable file \p relay/foo.cc up to level 2 and \p ir/bar.cc for level 0
only set:
Review comment:
maybe a little confusing to say 2 here but then have `TVM_LOG_DEBUG="1`
below
##########
File path: include/tvm/runtime/logging.h
##########
@@ -409,6 +416,68 @@ inline bool DebugLoggingEnabled() {
return state == 1;
}
+/*! \brief Helpers for \p VerboseLoggingEnabled. Exposed for unit testing
only. */
+std::unordered_map<std::string, int> ParseTvmLogDebugSpec(const char*
opt_spec);
+bool VerboseEnabledInMap(const std::string& filename, int level,
+ const std::unordered_map<std::string, int>& map);
+
+/*!
+ * \brief Returns true if a VLOG statement in \p filename is enabled by the \p
TVM_LOG_DEBUG
+ * environment variable for logging at verbosity \p level.
+ *
+ * Filenames are canonicalized to be w.r.t. the src/ dir of the TVM tree.
(VLOG's should not
+ * appear under include/).
+ *
+ * To enable file \p relay/foo.cc up to level 2 and \p ir/bar.cc for level 0
only set:
+ * \code
+ * TVM_LOG_DEBUG="1;relay/foo.cc=2;ir/bar.cc=0;"
+ * \endcode
+ *
+ * To enable all files up to level 3 but disable \p ir/bar.cc set:
+ * \code
+ * TVM_LOG_DEBUG="1;*=2;ir/bar.cc=-1;"
Review comment:
@huajsj i agree this is a bit more complex, but it is
backwards-compatible. i also don't think it's especially complex given what it
does.
it seems like a more structured, frontend-agnostic interface might be a
PackedFunc like:
```
TVM_REGISTER_GLOBAL("set_vlog_level").set_body_typed([](const std::string&
file_name, int level) {
my_global_map[file_name] = level
}
```
I'm totally open to adding this. However, I'd like to point out a couple of
advantages of the environment variable route @mbs-octoml proposes here.
- use of a single variable or config string: it's very easy to determine
exactly the VLOG configuration of TVM. you just `echo ${TVM_LOG_DEBUG}`. if
multiple variables are used, the command becomes more complex: `export | grep
TVM_LOG_DEBUG_LEVEL_`
- use of environment variable: We make heavy use of `multiprocessing` or
`PopenPool`, and have many different frontends. i'd argue that VLOG level
configuration is a TVM developer-facing interface. If we use a command-line
flag or script to configure logging, then every time a program is launched with
a different entry point, a parser would need to be written to convert some
command-line flag into a set of `get_function("set_vlog_level")(file_name,
level)` calls. Each script may have a different way to parse out the various
vlog levels, so if a TVM C++ developer is trying to help a user debug a problem
in `libtvm.so`, they first would need to determine how to configure TVM VLOG
levels. `TVM_LOG_DEBUG` is a more foolproof route. Additionally, we would need
to find a way to thread this down through `multiprocessing`/`PopenPool` so that
this configuration is propagated to e.g. autotuning Builder/Runner.
I agree with your point that temporary solutions can tend to become
permanent. In this case I'd argue that the complex part of this PR is in
parsing the environment variable and not in the change to the logging system.
Given the advantages listed above, I think I'd still want to keep this
functionality even if we overhaul the fundamental way logging works here. I
also don't think this mechanism has to be kept particularly
backwards-compatible--we can continue to improve it as we consider bigger
changes to logging. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
