[GitHub] [tvm] areusch commented on a change in pull request #9012: [Relay] VLOG for finer grained control of hyper-detailed logging

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org