[GitHub] [tvm] comaniac commented on a change in pull request #7816: [TVMC] --disable-pass option added to compile mode

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

comaniac commented on a change in pull request #7816:
URL: https://github.com/apache/tvm/pull/7816#discussion_r610784587



##########
File path: python/tvm/driver/tvmc/common.py
##########
@@ -333,6 +333,29 @@ def tracker_host_port_from_cli(rpc_tracker_str):
     return rpc_hostname, rpc_port
 
 
+def parse_disabled_pass(input_string):

Review comment:
       This utility could be more general. Since its functionatlity is taking a list of passes in a string and checking if they are valid, we don't need to limit this function to "disabled" passes. Instead, we could name it `parse_pass_list_str`, which does the same thing as now.

##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -95,6 +95,12 @@ def add_compile_parser(subparsers):
         type=common.parse_shape_string,
         default=None,
     )
+    parser.add_argument(
+        "--disabled-pass",
+        help="disable specific passes, comma-separated list of pass names",
+        type=common.parse_disabled_pass,
+        default=None,

Review comment:
       As I mentioned in another comment, you could assign `""` to default, so that the utility function can take only string but not None.

##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -138,6 +146,8 @@ def compile_model(
     target_host=None,
     tuning_records=None,
     alter_layout=None,
+    shape_dict=None,
+    disabled_pass=None,

Review comment:
       Update the docstring. And `shape_dict` seems not related to this PR?

##########
File path: python/tvm/driver/tvmc/common.py
##########
@@ -333,6 +333,29 @@ def tracker_host_port_from_cli(rpc_tracker_str):
     return rpc_hostname, rpc_port
 
 
+def parse_disabled_pass(input_string):
+    """Parse an input string for disabled passes
+
+    Parameters
+    ----------
+    input_string: str
+        Possibly comma-separated string with the names of disabled passes
+
+    Returns
+    -------
+    list: a list of disabled passes.
+    """
+    if input_string is not None:

Review comment:
       It's weird to see an only input could be None. I would suggest input must be a string, and you could assign `""` to the default value in the argparser.

##########
File path: python/tvm/driver/tvmc/common.py
##########
@@ -333,6 +333,29 @@ def tracker_host_port_from_cli(rpc_tracker_str):
     return rpc_hostname, rpc_port
 
 
+def parse_disabled_pass(input_string):
+    """Parse an input string for disabled passes
+
+    Parameters
+    ----------
+    input_string: str
+        Possibly comma-separated string with the names of disabled passes
+
+    Returns
+    -------
+    list: a list of disabled passes.
+    """
+    if input_string is not None:
+        pass_list = input_string.split(",")
+        nf = [_ for _ in pass_list if tvm.get_global_func("relay._transform." + _, True) is None]
+        if len(nf) > 0:

Review comment:
       ```suggestion
           invalid_passes = [p for p in pass_list if tvm.get_global_func("relay._transform.%s" % p, True) is None]
           if len(invalid_passes) > 0:
   ```
   Don't use underline as the variable name if you actually refer it.

##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -95,6 +95,12 @@ def add_compile_parser(subparsers):
         type=common.parse_shape_string,
         default=None,
     )
+    parser.add_argument(
+        "--disabled-pass",

Review comment:
       I would prefer the name `--disable-pass` in the CLI (active verb + noun).




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

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