[GitHub] [tvm] mdw-octoml commented on a change in pull request #8503: [PROFILING] Add json output to profiling reports

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

mdw-octoml commented on a change in pull request #8503:
URL: https://github.com/apache/tvm/pull/8503#discussion_r672796586



##########
File path: src/runtime/profiling.cc
##########
@@ -228,6 +228,71 @@ String ReportNode::AsCSV() const {
   return s.str();
 }
 
+namespace {
+void print_metric(std::ostream& os, ObjectRef o) {
+  if (o.as<StringObj>()) {
+    os << "\"" << Downcast<String>(o) << "\"";
+  } else if (const CountNode* n = o.as<CountNode>()) {
+    os << "{\"count\":" << std::to_string(n->value) << "}";
+  } else if (const DurationNode* n = o.as<DurationNode>()) {
+    os << "{\"microseconds\":" << std::to_string(n->microseconds) << "}";
+  } else if (const PercentNode* n = o.as<PercentNode>()) {
+    os << "{\"percent\":" << std::to_string(n->percent) << "}";
+  } else {
+    LOG(FATAL) << "Unprintable type " << o->GetTypeKey();
+  }
+}
+}  // namespace
+
+String ReportNode::AsJSON() const {
+  std::ostringstream s;
+  // DMLC's JSONWriter does not allow us to write a key value pair without
+  // implementing Write for the value. We want a specific write for the value,
+  // so we would have to implement a custom data structure for each type of
+  // value we want to print. Instead we construct the json by hand because it
+  // is easier.
+  s << "{";
+  s << "\"calls\":[";
+  for (size_t i = 0; i < calls.size(); i++) {
+    size_t j = 0;
+    s << "{";
+    for (const auto& kv : calls[i]) {
+      s << "\"" << kv.first << "\":";
+      print_metric(s, kv.second);
+      if (j < calls[i].size() - 1) {
+        s << ",";
+      }
+      j++;
+    }
+    s << "}";
+    if (i < calls.size() - 1) {
+      s << ",";
+    }
+  }
+  s << "],";
+  s << "\"device_metrics\":{";
+  size_t i = 0;
+  for (const auto& dev_kv : device_metrics) {
+    size_t j = 0;
+    s << "\"" << dev_kv.first << "\":{";
+    for (const auto& metric_kv : dev_kv.second) {
+      s << "\"" << metric_kv.first << "\":";

Review comment:
       I know this is being really nit-picky, but looking at how these profile reports are being generated, having the key be `Duration (us)` is kind of silly in the case of a JSON-formatted object (I understand why it should be human-readable in this way for the text report). It would be much better, IMHO, just for this to be `Duration` since the `microseconds` field in the child disambiguates the units. The easy way to achieve this here is to have `print_metric` print both the key and the value for that key, so it can decide based on the type of the node what to print.
   
   The reason this matters? Mainly so that the JSON schema can evolve over time and not have to tie values under "Duration" to the concept of "us", in case those units were to change at some point.

##########
File path: include/tvm/runtime/profiling.h
##########
@@ -205,6 +205,28 @@ class ReportNode : public Object {
    *  `aggregate` is true.
    */
   String AsTable(bool sort = true, bool aggregate = true) const;
+  /*! \brief Convert this report to json.
+   *
+   * Output json will be of this format:

Review comment:
       Also, recommend using `JSON` (not `json`) in the comments for the sake of the autogenerated documentation.

##########
File path: python/tvm/runtime/profiling/__init__.py
##########
@@ -47,6 +47,36 @@ def csv(self):
         """
         return _ffi_api.AsCSV(self)
 
+    def json(self):
+        """Convert this profiling report into JSON format.
+
+        Example output:
+
+        .. code-block:
+
+           {"calls":

Review comment:
       See comment on C++ above, would be great if this were pretty-printed for readability (in the comment itself, no need for the actual JSON output).

##########
File path: include/tvm/runtime/profiling.h
##########
@@ -205,6 +205,28 @@ class ReportNode : public Object {
    *  `aggregate` is true.
    */
   String AsTable(bool sort = true, bool aggregate = true) const;
+  /*! \brief Convert this report to json.
+   *
+   * Output json will be of this format:

Review comment:
       My eyes :-) Would you mind pretty printing this JSON in the comment? It makes the format more readable.

##########
File path: tests/python/unittest/test_runtime_profiling.py
##########
@@ -109,5 +110,21 @@ def test_papi(target, dev):
     assert any([float(x) > 0 for x in csv[metric]])
 
 
+@tvm.testing.requires_llvm
+def test_json():
+    mod, params = mlp.get_workload(1)
+
+    exe = relay.vm.compile(mod, "llvm", params=params)
+    vm = profiler_vm.VirtualMachineProfiler(exe, tvm.cpu())
+
+    data = np.random.rand(1, 1, 28, 28).astype("float32")
+    report = vm.profile(data, func_name="main")
+    parsed = json.loads(report.json())
+    assert "device_metrics" in parsed
+    assert "calls" in parsed
+    assert "Duration (us)" in parsed["calls"][0]
+    assert "microseconds" in parsed["calls"][0]["Duration (us)"]

Review comment:
       To strengthen this test, could you add some assertions like:
   
   ```
   assert len(parsed["calls"]) > 0
   for call in parsed["calls"]:
     assert isinstance(call["Name"], str)
     assert isinstance(call["Count"]["count"], int)
     assert isinstance(call["Duration"]["microseconds"], float)
   ```

##########
File path: python/tvm/runtime/profiling/__init__.py
##########
@@ -47,6 +47,36 @@ def csv(self):
         """
         return _ffi_api.AsCSV(self)
 
+    def json(self):
+        """Convert this profiling report into JSON format.
+
+        Example output:
+
+        .. code-block:
+
+           {"calls":

Review comment:
       See comment on C++ above, would be great if this were pretty-printed for readability (in the comment itself, no need for the actual JSON output).




-- 
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