PhilippvK commented on code in PR #12522:
URL: https://github.com/apache/tvm/pull/12522#discussion_r976058955


##########
python/tvm/driver/tvmc/autotuner.py:
##########
@@ -139,6 +138,11 @@ def add_tune_parser(subparsers, _, json_params):
         help="enable tuning the graph through the AutoScheduler tuner",
         action="store_true",
     )
+    parser.add_argument(
+        "--visualize",

Review Comment:
   @areusch 
   > `--plot=live,foo.png`
   
   This should be what is implemented right now just with a different name. The 
main disadvantage here is that the custom format/parsing of the argument (i.e. 
`live, path.png`, `path.png,live`,...) can be confusing.
   
   > `parser.add_argument("--visualize", nargs="?",   ...`
   
   Your proposed solution which accepts either one or no args at all, looks 
nice at the first glance but the `nargs="?"` might lead to some unexpected 
behavior if a model path is confused with the argument to the `--visualize` 
option by argparse. In addition, as you already suggested this only allows:
   - No visualizations
   - Live only
   - File only
   
   That's why I would prefer the two separate options, which also allow a 4 
combinations:
   - No visualizations
   - Live only
   - File only
   - Both
   



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

Reply via email to