Style, comments, refactoring in run-bisect-perf-regression.py.

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 104b20c659148f2d2e757c51545d0849ef925bdd..4be13ab6a74980046d4331f67d9d3695c915d67f 100755
--- a/tools/run-bisect-perf-regression.py
+++ b/tools/run-bisect-perf-regression.py
@@ -9,10 +9,8 @@ This script is used by a trybot to run the src/tools/bisect-perf-regression.py
 script with the parameters specified in run-bisect-perf-regression.cfg. It will
 check out a copy of the depot in a subdirectory 'bisect' of the working
 directory provided, and run the bisect-perf-regression.py script there.
-
 """
 
-import imp
 import optparse
 import os
 import platform
@@ -22,43 +20,42 @@ import traceback
 
 from auto_bisect import bisect_utils
 
-bisect = imp.load_source('bisect-perf-regression',
-    os.path.join(os.path.abspath(os.path.dirname(sys.argv[0])),
-        'bisect-perf-regression.py'))
-
+# Special import required because of dashes in file name.
+bisect = __import__('bisect-perf-regression')

[qyearsley, 2014/07/24 21:36:37]

Even if run-bisect-perf-regression.py is run from another directory, I believe
this should work.

This should basically be
  import bisect-perf-regression as bisect
if "bisect-perf-regression" were a valid module name.

 
 CROS_BOARD_ENV = 'BISECT_CROS_BOARD'
 CROS_IP_ENV = 'BISECT_CROS_IP'
 
+# Default config file names.
+BISECT_REGRESSION_CONFIG = 'run-bisect-perf-regression.cfg'
+RUN_TEST_CONFIG = 'run-perf-test.cfg'
+WEBKIT_RUN_TEST_CONFIG = os.path.join(
+    '..', 'third_party', 'WebKit', 'Tools', 'run-perf-test.cfg')
 
 class Goma(object):
+  """Convenience class to start and stop goma."""
 
   def __init__(self, path_to_goma):
     self._abs_path_to_goma = None
     self._abs_path_to_goma_file = None
-    if path_to_goma:
-      self._abs_path_to_goma = os.path.abspath(path_to_goma)
-      self._abs_path_to_goma_file = self._GetExecutablePath(
-          self._abs_path_to_goma)
+    if not path_to_goma:
+      return
+    self._abs_path_to_goma = os.path.abspath(path_to_goma)
+    filename = 'goma_ctl.bat' if os.name == 'nt' else 'goma_ctl.sh'

[qyearsley, 2014/07/24 21:36:37]

I thought this is simpler than how it was done with _GetExecutablePath.

+    self._abs_path_to_goma_file = os.path.join(self._abs_path_to_goma, filename)
 
   def __enter__(self):
-    if self._HasGOMAPath():
+    if self._HasGomaPath():

[qyearsley, 2014/07/24 21:36:37]

Goma is not spelled with all caps in its official documentation.

       self._SetupAndStart()
     return self
 
   def __exit__(self, *_):
-    if self._HasGOMAPath():
+    if self._HasGomaPath():
       self._Stop()
 
-  def _HasGOMAPath(self):
+  def _HasGomaPath(self):
     return bool(self._abs_path_to_goma)
 
-  def _GetExecutablePath(self, path_to_goma):
-    if os.name == 'nt':
-      return os.path.join(path_to_goma, 'goma_ctl.bat')
-    else:
-      return os.path.join(path_to_goma, 'goma_ctl.sh')
-
   def _SetupEnvVars(self):
     if os.name == 'nt':
       os.environ['CC'] = (os.path.join(self._abs_path_to_goma, 'gomacc.exe') +
@@ -70,7 +67,7 @@ class Goma(object):
           os.environ['PATH']])
 
   def _SetupAndStart(self):
-    """Sets up GOMA and launches it.
+    """Sets up goma and launches it.
 
     Args:
       path_to_goma: Path to goma directory.
@@ -85,32 +82,29 @@ class Goma(object):
     self._Stop()
 
     if subprocess.call([self._abs_path_to_goma_file, 'start']):
-      raise RuntimeError('GOMA failed to start.')
+      raise RuntimeError('Goma failed to start.')
 
   def _Stop(self):
     subprocess.call([self._abs_path_to_goma_file, 'stop'])
 
 
+def _LoadConfigFile(config_file_path):
+  """Loads to given file as a python module and returns the config dictionary.
 
-def _LoadConfigFile(path_to_file):
-  """Attempts to load the specified config file as a module
-  and grab the global config dict.
+  The config file is loaded as a Python module.
 
   Args:
-    path_to_file: Path to the file.
+    config_file_path: Path to the config file.
 
   Returns:
-    The config dict which should be formatted as follows:
-    {'command': string, 'good_revision': string, 'bad_revision': string
-     'metric': string, etc...}.
-    Returns None on failure.
+    If successful, returns the config dict loaded from the file. If no
+    such dictionary could be loaded, returns the empty dictionary.
   """
   try:
     local_vars = {}
-    execfile(path_to_file, local_vars)
-
+    execfile(config_file_path, local_vars)
     return local_vars['config']
-  except:
+  except Exception:
     print
     traceback.print_exc()
     print
@@ -122,54 +116,67 @@ def _ValidateConfigFile(config_contents, valid_parameters):
   non-empty.
 
   Args:
-    config_contents: Contents of the config file passed from _LoadConfigFile.
+    config_contents: A config dictionary.
     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 and type(v) is str
-        for v in config_contents.values() if v]
-    return config_has_values
-  except KeyError:
-    return False
+  for parameter in valid_parameters:
+    if parameter not in config_contents:
+      return False
+    value = config_contents[parameter]
+    if not value or type(value) is not str:
+      return False
+  return True

[qyearsley, 2014/07/24 21:36:37]

I believe that this is equivalent.

PTAL to verify.

 
 
 def _ValidatePerfConfigFile(config_contents):
-  """Validates that the perf config file contents. This is used when we're
-  doing a perf try job, rather than a bisect. The file is called
-  run-perf-test.cfg by default.
+  """Validates the perf config file contents.
+
+  This is used when we're doing a perf try job, rather than a bisect.
+  The config file is called run-perf-test.cfg by default.
 
   The parameters checked are the required parameters; any additional optional
   parameters won't be checked and validation will still pass.
 
   Args:
-    config_contents: Contents of the config file passed from _LoadConfigFile.
+    config_contents: A config dictionary.
 
   Returns:
     True if valid.
   """
-  valid_parameters = ['command', 'metric', 'repeat_count', 'truncate_percent',
-      'max_time_minutes']
+  valid_parameters = [
+      'command',
+      'metric',
+      'repeat_count',
+      'truncate_percent',
+      'max_time_minutes',
+  ]
   return _ValidateConfigFile(config_contents, valid_parameters)
 
 
 def _ValidateBisectConfigFile(config_contents):
-  """Validates that the bisect config file contents. The parameters checked are
-  the required parameters; any additional optional parameters won't be checked
-  and validation will still pass.
+  """Validates the bisect config file contents.
+
+  The parameters checked are the required parameters; any additional optional
+  parameters won't be checked and validation will still pass.
 
   Args:
-    config_contents: Contents of the config file passed from _LoadConfigFile.
+    config_contents: A config dictionary.
 
   Returns:
     True if valid.
   """
-  valid_params = ['command', 'good_revision', 'bad_revision', 'metric',
-      'repeat_count', 'truncate_percent', 'max_time_minutes']
+  valid_params = [
+      'command',
+      'good_revision',
+      'bad_revision',
+      'metric',
+      'repeat_count',
+      'truncate_percent',
+      'max_time_minutes',
+  ]
   return _ValidateConfigFile(config_contents, valid_params)
 
 
@@ -225,7 +232,16 @@ def _CreateBisectOptionsFromConfig(config):
 
 
 def _RunPerformanceTest(config, path_to_file):
-  # Bisect script expects to be run from src
+  """Runs a performance test with and without the current patch.
+
+  Args:
+    config: Contents of the config file, a dictionary.
+    path_to_file: Path to the bisect-perf-regression.py script.
+
+  Attempts to build and run the current revision with and without the
+  current patch, with the parameters passed in.
+  """
+  # Bisect script expects to be run from the src directory
   os.chdir(os.path.join(path_to_file, '..'))
 
   bisect_utils.OutputAnnotationStepStart('Building With Patch')
@@ -319,10 +335,10 @@ def _SetupAndRunPerformanceTest(config, path_to_file, path_to_goma):
     path_to_goma: Path to goma directory.
 
   Returns:
-    0 on success, otherwise 1.
+    The exit code of bisect-perf-regression.py: 0 on success, otherwise 1.
   """
   try:
-    with Goma(path_to_goma) as goma:
+    with Goma(path_to_goma) as _:
       config['use_goma'] = bool(path_to_goma)
       config['goma_dir'] = os.path.abspath(path_to_goma)
       _RunPerformanceTest(config, path_to_file)
@@ -333,10 +349,12 @@ def _SetupAndRunPerformanceTest(config, path_to_file, path_to_goma):
     return 1
 
 
-def _RunBisectionScript(config, working_directory, path_to_file, path_to_goma,
-    path_to_extra_src, dry_run):
-  """Attempts to execute src/tools/bisect-perf-regression.py with the parameters
-  passed in.
+def _RunBisectionScript(
+    config, working_directory, path_to_file, path_to_goma, path_to_extra_src,
+    dry_run):
+  """Attempts to execute bisect-perf-regression.py with the given parameters.
+
+  This function also

[prasadv, 2014/07/25 00:00:42]

causes sheriffs to scream at us:)
? 

[qyearsley, 2014/07/25 00:22:28]

Yeah, that's an unintended side-effect of this function :-P

I think that I was in the middle of writing that it also outputs the config step
(the rest of the steps along with their annotations are output in the
sub-process) -- so I just extracted that out here.

 
   Args:
     config: A dict containing the parameters to pass to the script.
@@ -349,7 +367,7 @@ def _RunBisectionScript(config, working_directory, path_to_file, path_to_goma,
     dry_run: Do a dry run, skipping sync, build, and performance testing steps.
 
   Returns:
-    0 on success, otherwise 1.
+    An exit status code: 0 on success, otherwise 1.
   """
   bisect_utils.OutputAnnotationStepStart('Config')
   print
@@ -387,9 +405,8 @@ def _RunBisectionScript(config, working_directory, path_to_file, path_to_goma,
       cmd.extend(['--cros_board', os.environ[CROS_BOARD_ENV]])
       cmd.extend(['--cros_remote_ip', os.environ[CROS_IP_ENV]])
     else:
-      print 'Error: Cros build selected, but BISECT_CROS_IP or'\
-            'BISECT_CROS_BOARD undefined.'
-      print
+      print ('Error: Cros build selected, but BISECT_CROS_IP or'
+             'BISECT_CROS_BOARD undefined.\n')
       return 1
 
   if 'android' in config['command']:
@@ -401,9 +418,10 @@ def _RunBisectionScript(config, working_directory, path_to_file, path_to_goma,
       cmd.extend(['--target_platform', 'android'])
 
   if path_to_goma:
-    # crbug.com/330900. For Windows XP platforms, GOMA service is not supported.
+    # For Windows XP platforms, goma service is not supported.
     # Moreover we don't compile chrome when gs_bucket flag is set instead
-    # use builds archives, therefore ignore GOMA service for Windows XP.
+    # use builds archives, therefore ignore goma service for Windows XP.
+    # See http://crbug.com/330900.
     if config.get('gs_bucket') and platform.release() == 'XP':
       print ('Goma doesn\'t have a win32 binary, therefore it is not supported '
              'on Windows XP platform. Please refer to crbug.com/330900.')
@@ -432,23 +450,21 @@ def _RunBisectionScript(config, working_directory, path_to_file, path_to_goma,
         '--debug_ignore_perf_test'])
   cmd = [str(c) for c in cmd]
 
-  with Goma(path_to_goma) as goma:
+  with Goma(path_to_goma) as _:
     return_code = subprocess.call(cmd)
 
   if return_code:
-    print 'Error: bisect-perf-regression.py returned with error %d' %\
-        return_code
-    print
+    print ('Error: bisect-perf-regression.py returned with error %d\n'
+           % return_code)
 
   return return_code
 
 
-def main():
-
+def _OptionParser():
+  """Returns the options parser for run-bisect-perf-regression.py."""
   usage = ('%prog [options] [-- chromium-options]\n'
            'Used by a trybot to run the bisection script using the parameters'
            ' provided in the run-bisect-perf-regression.cfg file.')
-
   parser = optparse.OptionParser(usage=usage)
   parser.add_option('-w', '--working_directory',
                     type='str',
@@ -474,55 +490,64 @@ def main():
                     help='The script will perform the full bisect, but '
                     'without syncing, building, or running the performance '
                     'tests.')
-  (opts, args) = parser.parse_args()
+  return parser
 
-  path_to_current_directory = os.path.abspath(os.path.dirname(sys.argv[0]))
 
-  # If they've specified their own config file, use that instead.
-  if opts.path_to_config:
-    path_to_bisect_cfg = opts.path_to_config
-  else:
-    path_to_bisect_cfg = os.path.join(path_to_current_directory,
-        'run-bisect-perf-regression.cfg')
+def main():
+  """Entry point for run-bisect-perf-regression.py.
+
+  Reads the config file, and then tries to either bisect a regression or
+  just run a performance test, depending on the particular config parameters
+  specified in the config file.
+  """
+
+  parser = _OptionParser()
+  opts, _ = parser.parse_args()
 
-  config = _LoadConfigFile(path_to_bisect_cfg)
+  current_dir = os.path.abspath(os.path.dirname(sys.argv[0]))
 
-  # Check if the config is valid.
+  # Use the default config file path unless one was specified.
+  config_path = os.path.join(current_dir, BISECT_REGRESSION_CONFIG)
+  if opts.path_to_config:
+    config_path = opts.path_to_config
+  config = _LoadConfigFile(config_path)
+
+  # Check if the config is valid for running bisect job.
   config_is_valid = _ValidateBisectConfigFile(config)
 
   if config and config_is_valid:
     if not opts.working_directory:
-      print 'Error: missing required parameter: --working_directory'
-      print
+      print 'Error: missing required parameter: --working_directory\n'
       parser.print_help()
       return 1
 
-    return _RunBisectionScript(config, opts.working_directory,
-        path_to_current_directory, opts.path_to_goma, opts.extra_src,
-        opts.dry_run)
-  else:
-    perf_cfg_files = ['run-perf-test.cfg', os.path.join('..', 'third_party',
-        'WebKit', 'Tools', 'run-perf-test.cfg')]
-
-    for current_perf_cfg_file in perf_cfg_files:
-      if opts.path_to_config:
-        path_to_perf_cfg = opts.path_to_config
-      else:
-        path_to_perf_cfg = os.path.join(
-            os.path.abspath(os.path.dirname(sys.argv[0])),
-            current_perf_cfg_file)
-
-      config = _LoadConfigFile(path_to_perf_cfg)
-      config_is_valid = _ValidatePerfConfigFile(config)
-
-      if config and config_is_valid:
-        return _SetupAndRunPerformanceTest(config, path_to_current_directory,
-            opts.path_to_goma)
-
-    print 'Error: Could not load config file. Double check your changes to '\
-          'run-bisect-perf-regression.cfg/run-perf-test.cfg for syntax errors.'
-    print
-    return 1
+    return _RunBisectionScript(
+        config, opts.working_directory, current_dir,
+        opts.path_to_goma, opts.extra_src, opts.dry_run)
+
+  # If it wasn't valid for running a bisect, then maybe the user wanted
+  # to run a perf test instead of a bisect job. Try reading any possible
+  # perf test config files.
+  perf_cfg_files = [RUN_TEST_CONFIG, WEBKIT_RUN_TEST_CONFIG]
+  for current_perf_cfg_file in perf_cfg_files:
+    if opts.path_to_config:
+      path_to_perf_cfg = opts.path_to_config
+    else:
+      path_to_perf_cfg = os.path.join(
+          os.path.abspath(os.path.dirname(sys.argv[0])),
+          current_perf_cfg_file)
+
+    config = _LoadConfigFile(path_to_perf_cfg)
+    config_is_valid = _ValidatePerfConfigFile(config)
+
+    if config and config_is_valid:
+      return _SetupAndRunPerformanceTest(
+          config, current_dir, opts.path_to_goma)
+
+  print ('Error: Could not load config file. Double check your changes to '
+         'run-bisect-perf-regression.cfg or run-perf-test.cfg for syntax '
+         'errors.\n')
+  return 1
 
 
 if __name__ == '__main__':