[GitHub] [tvm] lhutton1 commented on a diff in pull request #15349: [TVMC] Add tvmc flag to print compilation time per pass

["lhutton1 (via GitHub)" <gi...@apache.org>]

lhutton1 commented on code in PR #15349:
URL: https://github.com/apache/tvm/pull/15349#discussion_r1268133213


##########
tests/python/driver/tvmc/test_command_line.py:
##########
@@ -272,3 +272,20 @@ def test_tvmc_logger_set_basicConfig(monkeypatch, tmpdir_factory, keras_simple):
     _main(compile_args)
 
     mock_basicConfig.assert_called_with(stream=sys.stdout)
+
+
+def test_tvmc_print_pass_times(capsys, keras_simple, tmpdir_factory):
+    pytest.importorskip("tensorflow")
+    tmpdir = tmpdir_factory.mktemp("out")
+    print_cmd = "--print-pass-times"
+
+    # Compile model
+    module_file = os.path.join(tmpdir, "keras-tvm.tar")
+    compile_cmd = f"tvmc compile --target 'llvm' {keras_simple} --output {module_file} {print_cmd}"
+    compile_args = compile_cmd.split(" ")[1:]
+    _main(compile_args)
+
+    # Check for timing results output
+    expected_out = "Printing results of timing profile..."
+    captured_out = capsys.readouterr().out
+    assert expected_out in captured_out

Review Comment:
   We should also try to check the pass breakdown exists and that it makes some form of sense. Could we check for some known parts of the outputs like pass names and the timing units?



##########
python/tvm/driver/tvmc/compiler.py:
##########
@@ -356,6 +363,13 @@ def compile_model(
         if codegen["config_key"] is not None:
             config[codegen["config_key"]] = codegen_from_cli["opts"]
 
+    timing_inst = None

Review Comment:
   `compile_model` is also used by the TVMC Python API, so if a user adds `PassTimingInstrument` to their list of instruments and calls `compile_model` providing the list of instruments, they may get an unexpected output printed to the console. 
   
   Since it is relatively easy for a python API user to add the instrument and print it themselves, I think it would be better to move the printing functionality up to `drive_compile` so that the feature is only exposed to the command line API. It would also simplify checking if the option was enabled.
   
   Happy to hear more thoughts from others though.



##########
python/tvm/driver/tvmc/compiler.py:
##########
@@ -442,6 +456,11 @@ def compile_model(
         if dumps:
             save_dumps(package_path, dumps)
 
+        # Print compilation times per pass
+        if timing_inst:
+            print("Printing results of timing profile...")

Review Comment:
   Perhaps we could rename to "Compilation time breakdown by pass:" - I feel like it's a bit more informative



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