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

tools/run-bisect-perf-regression.py

 #!/usr/bin/env python
 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Run Performance Test Bisect Tool
 
 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
 import subprocess
 import sys
 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') +
           ' cl.exe')
       os.environ['CXX'] = (os.path.join(self._abs_path_to_goma, 'gomacc.exe') +
           ' cl.exe')
     else:
       os.environ['PATH'] = os.pathsep.join([self._abs_path_to_goma,
           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.
 
     Returns:
       True if successful."""
     self._SetupEnvVars()
 
     # Sometimes goma is lingering around if something went bad on a previous
     # run. Stop it before starting a new process. Can ignore the return code
     # since it will return an error if it wasn't running.
     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
     return {}
 
 
 def _ValidateConfigFile(config_contents, valid_parameters):
   """Validates the config file contents, checking whether all values are
   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)
 
 
 def _OutputFailedResults(text_to_print):
   bisect_utils.OutputAnnotationStepStart('Results - Failed')
   print
   print text_to_print
   print
   bisect_utils.OutputAnnotationStepClosed()
 
@@ skipping 35 matching lines @@
       opts_dict['target_platform'] = 'android'
     elif 'android-chrome' in config['command']:
       opts_dict['target_platform'] = 'android-chrome'
     else:
       opts_dict['target_platform'] = 'android'
 
   return bisect.BisectOptions.FromDict(opts_dict)
 
 
 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')
 
   opts = _CreateBisectOptionsFromConfig(config)
   b = bisect.BisectPerformanceMetrics(None, opts)
 
   if bisect_utils.RunGClient(['runhooks']):
     raise RuntimeError('Failed to run gclient runhooks')
 
@@ skipping 73 matching lines @@
 def _SetupAndRunPerformanceTest(config, path_to_file, path_to_goma):
   """Attempts to build and run the current revision with and without the
   current patch, with the parameters passed in.
 
   Args:
     config: The config read from run-perf-test.cfg.
     path_to_file: Path to the bisect-perf-regression.py script.
     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)
     return 0
   except RuntimeError, e:
     bisect_utils.OutputAnnotationStepClosed()
     _OutputFailedResults('Error: %s' % e.message)
     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.
     working_directory: A working directory to provide to the
       bisect-perf-regression.py script, where it will store it's own copy of
       the depot.
     path_to_file: Path to the bisect-perf-regression.py script.
     path_to_goma: Path to goma directory.
     path_to_extra_src: Path to extra source file.
     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
   for k, v in config.iteritems():
     print '  %s : %s' % (k, v)
   print
   bisect_utils.OutputAnnotationStepClosed()
 
   cmd = ['python', os.path.join(path_to_file, 'bisect-perf-regression.py'),
          '-c', config['command'],
@@ skipping 17 matching lines @@
 
   cmd.extend(['--build_preference', 'ninja'])
 
   if '--browser=cros' in config['command']:
     cmd.extend(['--target_platform', 'cros'])
 
     if os.environ[CROS_BOARD_ENV] and os.environ[CROS_IP_ENV]:
       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']:
     if 'android-chrome-shell' in config['command']:
       cmd.extend(['--target_platform', 'android'])
     elif 'android-chrome' in config['command']:
       cmd.extend(['--target_platform', 'android-chrome'])
     else:
       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.')
       path_to_goma = None
     cmd.append('--use_goma')
 
   if path_to_extra_src:
     cmd.extend(['--extra_src', path_to_extra_src])
 
   # These flags are used to download build archives from cloud storage if
   # available, otherwise will post a try_job_http request to build it on
   # tryserver.
   if config.get('gs_bucket'):
     if config.get('builder_host') and config.get('builder_port'):
       cmd.extend(['--gs_bucket', config['gs_bucket'],
                   '--builder_host', config['builder_host'],
                   '--builder_port', config['builder_port']
                  ])
     else:
       print ('Error: Specified gs_bucket, but missing builder_host or '
              'builder_port information in config.')
       return 1
 
   if dry_run:
     cmd.extend(['--debug_ignore_build', '--debug_ignore_sync',
         '--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',
                     help='A working directory to supply to the bisection '
                     'script, which will use it as the location to checkout '
                     'a copy of the chromium depot.')
   parser.add_option('-p', '--path_to_goma',
                     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. The script will attempt to load it '
                     'as a bisect config first, then a perf config.')
   parser.add_option('--extra_src',
                     type='str',
                     help='Path to extra source file. If this is supplied, '
                     'bisect script will use this to override default behavior.')
   parser.add_option('--dry_run',
                     action="store_true",
                     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.
+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()
+
+  current_dir = os.path.abspath(os.path.dirname(sys.argv[0]))
+
+  # 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:
-    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')
+    config_path = opts.path_to_config
+  config = _LoadConfigFile(config_path)
 
-  config = _LoadConfigFile(path_to_bisect_cfg)
-
-  # Check if the config is valid.
+  # 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')]
+    return _RunBisectionScript(
+        config, opts.working_directory, current_dir,
+        opts.path_to_goma, opts.extra_src, opts.dry_run)
 
-    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)
+  # 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)
+    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)
+    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/run-perf-test.cfg for syntax errors.'
-    print
-    return 1
+  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__':
   sys.exit(main())