[bisect] - Add --path_to_config parameter to run script.

tools/run-bisect-perf-regression.py

Index: tools/run-bisect-perf-regression.py
diff --git a/tools/run-bisect-perf-regression.py b/tools/run-bisect-perf-regression.py
index 8dceea82fc0bcb8a7a8659aac3c300139e780a15..339442b1920b706222c7e5f70e658955ddcb4e33 100755
--- a/tools/run-bisect-perf-regression.py
+++ b/tools/run-bisect-perf-regression.py
@@ -115,6 +115,54 @@ def _LoadConfigFile(path_to_file):
     return {}
 
 
+def _ValidateConfigFile(config_contents, valid_parameters):
+  """Validates that the config file contents is a valid perf config, and all

[qyearsley, 2014/04/10 17:58:22]

Nit: This wording is a little bit redundant, since "to validate" means "to check
whether sth is valid".

Maybe something like: "Validates the config file contents, checking whether all
values are non-empty."

[shatch, 2014/04/10 18:21:11]

Done.

+  the values are non-empty.
+
+  Args:
+    config_contents: Contents of the config file passed from _LoadConfigFile.
+    valid_parameters: A list of parameters to check for.
+
+  Returns:
+    True if valid.
+  """
+  try:
+    [config_contents[current_parameter]
+        for current_parameter in valid_parameters]
+    config_has_values = [v for v in config_contents.values() if v]

[qyearsley, 2014/04/10 17:58:22]

What if the value used for truncate_percent was 0?

It looks like we're expecting that all of the values in the config dict must be
non-empty strings.

If you think that later someone might want to directly put integers in the
config file, maybe here you could check  
[v != '' and v is not None for v in config_values.values() if v]

OR, if we want to assert that the values in the config must be strings, perhaps
you could use:

[v and type(v) is str for v in config_values.values() if v]

[shatch, 2014/04/10 18:21:11]

Yeah, they've traditionally just been strings. Added the extra string check.

+    return config_has_values
+  except KeyError:
+    return False
+
+
+def _ValidatePerfConfigFile(config_contents):
+  """Validates that the config file contents is a valid perf config.

[qyearsley, 2014/04/10 17:58:22]

I wasn't aware before that there was such a thing as a "perf config" (usually at
run-perf-test.cfg, right?)

Maybe add a note saying that a "perf config file" is a config used when we only
want to run one perf test once, and we're not actually bisecting; it's usually
called run-perf-test.cfg by default.

[shatch, 2014/04/10 18:21:11]

Done.

+
+  Args:
+    config_contents: Contents of the config file passed from _LoadConfigFile.
+
+  Returns:
+    True if valid.
+  """
+  valid_parameters = ['command', 'metric', 'repeat_count', 'truncate_percent',
+      'max_time_minutes']
+  return _ValidateConfigFile(config_contents, valid_parameters)
+
+
+def _ValidateBisectConfigFile(config_contents):
+  """Validates that the config file contents is a valid bisect config.
+
+  Args:
+    config_contents: Contents of the config file passed from _LoadConfigFile.
+
+  Returns:
+    True if valid.
+  """
+  valid_params = ['command', 'good_revision', 'bad_revision', 'metric',
+      'repeat_count', 'truncate_percent', 'max_time_minutes']
+  return _ValidateConfigFile(config_contents, valid_params)
+
+
 def _OutputFailedResults(text_to_print):
   bisect_utils.OutputAnnotationStepStart('Results - Failed')
   print
@@ -369,6 +417,11 @@ def main():
                     type='str',
                     help='Path to goma directory. If this is supplied, goma '
                     'builds will be enabled.')
+  parser.add_option('--path_to_config',
+                    type='str',
+                    help='Path to the config file to use. If this is supplied, '
+                    'the bisect script will use this to override the default '
+                    'config file path.')
   parser.add_option('--extra_src',
                     type='str',
                     help='Path to extra source file. If this is supplied, '
@@ -381,15 +434,20 @@ def main():
   (opts, args) = parser.parse_args()
 
   path_to_current_directory = os.path.abspath(os.path.dirname(sys.argv[0]))
-  path_to_bisect_cfg = os.path.join(path_to_current_directory,
-      'run-bisect-perf-regression.cfg')
+
+  # If they've specified their own config file, use that instead.
+  if opts.path_to_config:
+    path_to_bisect_cfg = opts.path_to_config

[qyearsley, 2014/04/10 17:58:22]

At this point, we're not 100% sure whether they've supplied a "bisect config" or
"perf config".

[shatch, 2014/04/10 18:21:11]

True, added some changes so that if loading it as a bisect config fails, it
falls back to treating it as a perf config.

+  else:
+    path_to_bisect_cfg = os.path.join(path_to_current_directory,
+        'run-bisect-perf-regression.cfg')
 
   config = _LoadConfigFile(path_to_bisect_cfg)
 
-  # Check if the config is empty
-  config_has_values = [v for v in config.values() if v]
+  # Check if the config is valid.
+  config_is_valid = _ValidateBisectConfigFile(config)
 
-  if config and config_has_values:
+  if config and config_is_valid:
     if not opts.working_directory:
       print 'Error: missing required parameter: --working_directory'
       print
@@ -408,9 +466,9 @@ def main():
           os.path.abspath(os.path.dirname(sys.argv[0])), current_perf_cfg_file)
 
       config = _LoadConfigFile(path_to_perf_cfg)
-      config_has_values = [v for v in config.values() if v]
+      config_is_valid = _ValidatePerfConfigFile(config)
 
-      if config and config_has_values:
+      if config and config_is_valid:
         return _SetupAndRunPerformanceTest(config, path_to_current_directory,
             opts.path_to_goma)