Refactor stages 1, 2 and test_api overhaul.

scripts/slave/recipe_modules/auto_bisect/bisector.py

 # Copyright 2015 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.
 
 import json
 import re
 import time
 import urllib
 
 from . import config_validation
@@ skipping 29 matching lines @@
 )
 
 # When we look for the next revision to build, we search nearby revisions
 # looking for a revision that's already been archived. Since we don't want
 # to move *too* far from the original revision, we'll cap the search at 25%.
 DEFAULT_SEARCH_RANGE_PERCENTAGE = 0.25
 
 # How long to re-test the initial good-bad range for until significant
 # difference is established.
 REGRESSION_CHECK_TIMEOUT = 2 * 60 * 60
-# If we reach this number of samples on the reference range and have not
-# achieved statistical significance, bail.
-MAX_REQUIRED_SAMPLES = 15
-
-# Significance level to use for determining difference between revisions via
-# hypothesis testing.
-SIGNIFICANCE_LEVEL = 0.01
 
 _FAILED_INITIAL_CONFIDENCE_ABORT_REASON = (
     'The metric values for the initial "good" and "bad" revisions '
     'do not represent a clear regression.')
 
 _DIRECTION_OF_IMPROVEMENT_ABORT_REASON = (
     'The metric values for the initial "good" and "bad" revisions match the '
     'expected direction of improvement. Thus, likely represent an improvement '
     'and not a regression.')
 
@@ skipping 81 matching lines @@
 
     Returns:
       A 40-digit git commit hash string.
     """
     if self._is_sha1(rev):  # pragma: no cover
       return rev
     if rev.isdigit():
       commit_position = self._api.m.commit_position.construct(
           branch='refs/heads/master', value=rev)
       try:
-        return self._api.m.crrev.to_commit_hash(commit_position)
+        return self._api.m.crrev.to_commit_hash(
+            commit_position,
+            step_test_data=lambda: self.api._test_data['hash_cp_map'][rev])
       except self.api.m.step.StepFailure:  # pragma: no cover
         self.surface_result('BAD_REV')
         raise
     self.surface_result('BAD_REV')  # pragma: no cover
     raise self.api.m.step.StepFailure(
         'Invalid input revision: %r' % (rev,))  # pragma: no cover
 
   @staticmethod
   def _is_sha1(s):
     return bool(re.match('^[0-9A-Fa-f]{40}$', s))
 
-  def significantly_different(
-      self, list_a, list_b,
-      significance_level=SIGNIFICANCE_LEVEL):  # pragma: no cover
-    """Uses an external script to run hypothesis testing with scipy.
+  def compare_revisions(self, revision_a, revision_b):
+    """
+    Returns:
+      True if the samples are significantly different.
+      None if there is not enough data to tell.
+      False if there's enough data but still can't tell the samples apart.
+    """
+    output_format = 'chartjson'
+    values_a = revision_a.chartjson_paths
+    values_b = revision_b.chartjson_paths
+    if revision_a.valueset_paths and revision_b.valueset_paths:
+      output_format = 'valueset'
 
-    The reason why we need an external script is that scipy is not available to
-    the default python installed in all platforms. We instead rely on an
-    anaconda environment to provide those packages.
+    result = self.api.stat_compare(
+        values_a,
+        values_b,
+        self.bisect_config['metric'],
+        output_format=output_format,
+        step_test_data=lambda: self.api.test_api.compare_samples_data(
+            self.api._test_data.get('revision_data'), revision_a, revision_b))
 
-    Args:
-      list_a, list_b: Two lists representing samples to be compared.
-      significance_level: Self-describing. As a decimal fraction.
+    revision_a.debug_values = result['sample_a']['debug_values']
+    revision_b.debug_values = result['sample_b']['debug_values']
+    revision_a.mean = result['sample_a']['mean']

[RobertoCN, 2016/08/23 00:26:24]

Make mean and std_dev properties of revision_state and generate the values on
the fly

[RobertoCN, 2016/09/07 00:33:24]

Done.

+    revision_b.mean = result['sample_b']['mean']
+    revision_a.std_dev = result['sample_a']['std_dev']
+    revision_b.std_dev = result['sample_b']['std_dev']
 
-    Returns:
-      A boolean indicating whether the null hypothesis ~(that the lists are
-      samples from the same population) can be rejected at the specified
-      significance level.
-    """
-    step_result = self.api.m.python(
-        'Checking sample difference',
-        self.api.resource('significantly_different.py'),
-        [json.dumps(list_a), json.dumps(list_b), str(significance_level)],
-        stdout=self.api.m.json.output())
-    results = step_result.stdout
-    if results is None:
-      assert self.dummy_builds
-      return True
-    significantly_different = results['significantly_different']
-    step_result.presentation.logs[str(significantly_different)] = [
-        'See json.output for details']
-    return significantly_different
+    if result['result'] == 'needMoreData':

[RobertoCN, 2016/08/23 00:26:24]

Make 3 constants for true, false and needMore.

[RobertoCN, 2016/09/07 00:33:24]

Done.

+      return None
+    return bool(result['result'])
 
   def config_step(self):
     """Yields a step that prints the bisect config."""
     api = self.api
 
     # bisect_config may come as a FrozenDict (which is not serializable).
     bisect_config = dict(self.bisect_config)
 
     def fix_windows_backslashes(s):
       backslash_regex = re.compile(r'(?<!\\)\\(?!\\)')
@@ skipping 17 matching lines @@
     except config_validation.ValidationFail as error:
       self.surface_result('BAD_CONFIG')
       self.api.m.halt(error.message)
       raise self.api.m.step.StepFailure(error.message)
 
   @property
   def api(self):
     return self._api
 
   def compute_relative_change(self):
-    old_value = float(self.good_rev.mean_value)
-    new_value = float(self.bad_rev.mean_value)
+    old_value = float(self.good_rev.mean)
+    new_value = float(self.bad_rev.mean)
 
     if new_value and not old_value:  # pragma: no cover
       self.relative_change = ZERO_TO_NON_ZERO
       return
 
     rel_change = self.api.m.math_utils.relative_change(old_value, new_value)
     self.relative_change = '%.2f%%' % (100 * rel_change)
 
   def make_deps_sha_file(self, deps_sha):
     """Make a diff patch that creates DEPS.sha.
@@ skipping 16 matching lines @@
         file is to be written to.
       commit_hash (str): An identifier for the step.
 
     Returns:
       A string containing the hash of the interned object.
     """
     cmd = 'hash-object -t blob -w --stdin'.split(' ')
     stdin = self.api.m.raw_io.input(file_contents)
     stdout = self.api.m.raw_io.output()
     step_name = 'Hashing modified DEPS file with revision ' + commit_hash
-    step_result = self.api.m.git(*cmd, cwd=cwd, stdin=stdin, stdout=stdout,
-                                 name=step_name)
+    step_result = self.api.m.git(
+        *cmd, cwd=cwd, stdin=stdin, stdout=stdout, name=step_name,
+        step_test_data=lambda:
+            self.api.m.raw_io.test_api.stream_output(commit_hash))
     hash_string = step_result.stdout.splitlines()[0]
     try:
       if hash_string:
         int(hash_string, 16)
         return hash_string
     except ValueError:  # pragma: no cover
       reason = 'Git did not output a valid hash for the interned file.'
       self.api.m.halt(reason)
       raise self.api.m.step.StepFailure(reason)
 
@@ skipping 14 matching lines @@
     Returns:
       A string containing the diff patch as produced by the 'git diff' command.
     """
     # The prefixes used in the command below are used to find and replace the
     # tree-ish git object id's on the diff output more easily.
     cmd = 'diff %s %s --src-prefix=IAMSRC: --dst-prefix=IAMDST:'
     cmd %= (git_object_a, git_object_b)
     cmd = cmd.split(' ')
     stdout = self.api.m.raw_io.output()
     step_name = 'Generating patch for %s to %s' % (git_object_a, deps_rev)
-    step_result = self.api.m.git(*cmd, cwd=cwd, stdout=stdout, name=step_name)
+    step_result = self.api.m.git(
+        *cmd, cwd=cwd, stdout=stdout, name=step_name,
+        step_test_data=lambda: self.api._test_data['diff_patch'])
     patch_text = step_result.stdout
     src_string = 'IAMSRC:' + git_object_a
     dst_string = 'IAMDST:' + git_object_b
     patch_text = patch_text.replace(src_string, src_alias)
     patch_text = patch_text.replace(dst_string, dst_alias)
     return patch_text
 
   def make_deps_patch(self, base_revision, base_file_contents,
                       depot, new_commit_hash):
     """Make a diff patch that updates a specific dependency revision.
@@ skipping 42 matching lines @@
         interned_deps_hash, deps_file, deps_file,
         cwd=cwd,
         deps_rev=new_commit_hash)
     return patch_text, patched_contents
 
   def _expand_initial_revision_range(self):
     """Sets the initial contents of |self.revisions|."""
     with self.api.m.step.nest('Expanding revision range'):
       good_hash = self.good_rev.commit_hash
       bad_hash = self.bad_rev.commit_hash
+      depot = self.good_rev.depot_name
       step_name = 'for revisions %s:%s' % (good_hash, bad_hash)
       revisions = self._revision_range(
           start=good_hash,
           end=bad_hash,
           depot_name=self.base_depot,
-          step_name=step_name)
+          step_name=step_name,
+          step_test_data=lambda: self.api._test_data['revision_list'][depot]
+      )
       self.revisions = [self.good_rev] + revisions + [self.bad_rev]
       self._update_revision_list_indexes()
 
   def _revision_range(self, start, end, depot_name, base_revision=None,
-                      step_name=None):
+                      step_name=None, **kwargs):
     """Returns a list of RevisionState objects between |start| and |end|.
 
     Args:
       start (str): Start commit hash.
       end (str): End commit hash.
       depot_name (str): Short string name of repo, e.g. chromium or v8.
       base_revision (str): Base revision in the downstream repo (e.g. chromium).
       step_name (str): Optional step name.
 
     Returns:
       A list of RevisionState objects, not including the given start or end.
     """
     if self.internal_bisect:  # pragma: no cover
       return self._revision_range_with_gitiles(
           start, end, depot_name, base_revision, step_name)
     try:
       step_result = self.api.m.python(
           step_name,
           self.api.resource('fetch_intervening_revisions.py'),
           [start, end, depot_config.DEPOT_DEPS_NAME[depot_name]['url']],
-          stdout=self.api.m.json.output())
+          stdout=self.api.m.json.output(), **kwargs)
     except self.api.m.step.StepFailure:  # pragma: no cover
       self.surface_result('BAD_REV')
       raise
     revisions = []
     for commit_hash, _ in step_result.stdout:
       revisions.append(self.revision_class(
           bisector=self,
           commit_hash=commit_hash,
           depot_name=depot_name,
           base_revision=base_revision))
@@ skipping 85 matching lines @@
           dep_revision_max = max_revision.deps[depot_name]
           if (dep_revision_min and dep_revision_max and
               dep_revision_min != dep_revision_max):
             step_name = ('Expanding revision range for revision %s'
                          ' on depot %s' % (dep_revision_max, depot_name))
             rev_list = self._revision_range(
                 start=dep_revision_min,
                 end=dep_revision_max,
                 depot_name=depot_name,
                 base_revision=min_revision,
-                step_name=step_name)
+                step_name=step_name,
+                step_test_data=lambda:
+                    self.api._test_data['revision_list'][depot_name])
             new_revisions = self.revisions[:max_revision.list_index]
             new_revisions += rev_list
             new_revisions += self.revisions[max_revision.list_index:]
             self.revisions = new_revisions
             self._update_revision_list_indexes()
             return True
     except RuntimeError:  # pragma: no cover
       warning_text = ('Could not expand dependency revisions for ' +
                       revision_to_expand.commit_hash)
       self.surface_result('BAD_REV')
@@ skipping 16 matching lines @@
 
     The change between the test results obtained for the given 'good' and
     'bad' revisions is expected to be considered a regression. The
     `improvement_direction` attribute is positive if a larger number is
     considered better, and negative if a smaller number is considered better.
 
     Returns:
       True if the check passes (i.e. no problem), False if the change is not
       a regression according to the improvement direction.
     """
-    good = self.good_rev.mean_value
-    bad = self.bad_rev.mean_value
+    good = self.good_rev.mean
+    bad = self.bad_rev.mean
 
     if self.is_return_code_mode():
       return True
 
     direction = self.improvement_direction
     if direction is None:
       return True
     if (bad > good and direction > 0) or (bad < good and direction < 0):
       self._set_failed_direction_results()
       return False
@@ skipping 18 matching lines @@
     or REGRESSION_CHECK_TIMEOUT seconds have elapsed.
 
     Returns: True if the revisions produced results that differ from each
     other in a statistically significant manner. False if such difference could
     not be established in the time or sample size allowed.
     """
     if self.test_type == 'return_code':
       return (self.good_rev.overall_return_code !=
               self.bad_rev.overall_return_code)
 
     if self.bypass_stats_check:

[RobertoCN, 2016/08/23 00:26:24]

Remove this flag

[RobertoCN, 2016/09/07 00:33:24]

Acknowledged.

-      dummy_result = self.good_rev.values != self.bad_rev.values
+      self.compare_revisions(self.good_rev, self.bad_rev)
+      dummy_result = self.good_rev.mean != self.bad_rev.mean
       if not dummy_result:
         self._set_insufficient_confidence_warning()
       return dummy_result
 
+    # TODO(robertocn): This step should not be necessary in some cases.
     with self.api.m.step.nest('Re-testing reference range'):
       expiration_time = time.time() + REGRESSION_CHECK_TIMEOUT
       while time.time() < expiration_time:
-        if len(self.good_rev.values) >= 5 and len(self.bad_rev.values) >= 5:
-          if self.significantly_different(self.good_rev.values,
-                                          self.bad_rev.values):
+        if (self.good_rev.test_run_count >= 5
+            and self.bad_rev.test_run_count >= 5):
+          if self.compare_revisions(self.good_rev, self.bad_rev):
             return True
-        if len(self.good_rev.values) == len(self.bad_rev.values):
+        if self.good_rev.test_run_count == self.bad_rev.test_run_count:
           revision_to_retest = self.last_tested_revision
         else:
           revision_to_retest = min(self.good_rev, self.bad_rev,
-                                   key=lambda x: len(x.values))
-        if len(revision_to_retest.values) < MAX_REQUIRED_SAMPLES:
-          revision_to_retest.retest()
-        else:
-          break
+                                   key=lambda x: x.test_run_count)
       self._set_insufficient_confidence_warning()
       return False
 
 
   def get_exception(self):
     raise NotImplementedError()  # pragma: no cover
     # TODO: should return an exception with the details of the failure.
 
   def _set_insufficient_confidence_warning(
       self):  # pragma: no cover
     """Adds a warning about the lack of initial regression confidence."""
     self.failed_initial_confidence = True
     self.surface_result('LO_INIT_CONF')
     self.warnings.append(
         'Bisect failed to reproduce the regression with enough confidence.')
 
   def _results_debug_message(self):
     """Returns a string with values used to debug a bisect result."""
     result = 'bisector.lkgr: %r\n' % self.lkgr
     result += 'bisector.fkbr: %r\n\n' % self.fkbr
     result += self._revision_value_table()
-    if (self.lkgr and self.lkgr.values and self.fkbr and self.fkbr.values):
-      result += '\n' + self._t_test_results()
+    if (self.lkgr and self.lkgr.test_run_count and self.fkbr and
+        self.fkbr.test_run_count):
+      result += '\n' + '\n'.join([
+          'LKGR values: %r' % list(self.lkgr.debug_values),
+          'FKBR values: %r' % list(self.fkbr.debug_values),
+      ])
     return result
 
   def _revision_value_table(self):
     """Returns a string table showing revisions and their values."""
     header = [['Revision', 'Values']]
-    rows = [[r.revision_string(), str(r.values)] for r in self.revisions]
+    rows = [[r.revision_string(), str(r.debug_values)] for r in self.revisions]
     return self._pretty_table(header + rows)
 
   def _pretty_table(self, data):
     results = []
     for row in data:
       results.append('%-15s' * len(row) % tuple(row))
     return '\n'.join(results)
 
-  def _t_test_results(self):
-    """Returns a string showing t-test results for lkgr and fkbr."""
-    t, df, p = self.api.m.math_utils.welchs_t_test(
-        self.lkgr.values, self.fkbr.values)
-    lines = [
-        'LKGR values: %r' % self.lkgr.values,
-        'FKBR values: %r' % self.fkbr.values,
-        't-statistic: %r' % t,
-        'deg. of freedom:  %r' % df,
-        'p-value: %r' % p,
-        'Confidence score: %r' % (100 * (1 - p))
-    ]
-    return '\n'.join(lines)
-
   def print_result_debug_info(self):
     """Prints extra debug info at the end of the bisect process."""
     lines = self._results_debug_message().splitlines()
     # If we emit a null step then add a log to it, the log should be kept
     # longer than 7 days (which is often needed to debug some issues).
     self.api.m.step('Debug Info', [])
     self.api.m.step.active_result.presentation.logs['Debug Info'] = lines
 
   def post_result(self, halt_on_failure=False):
     """Posts bisect results to Perf Dashboard."""
     self.api.m.perf_dashboard.set_default_config()
     self.api.m.perf_dashboard.post_bisect_results(
         self.get_result(), halt_on_failure)
 
   def get_revision_to_eval(self):
     """Gets the next RevisionState object in the candidate range.
 
     Returns:
        The next Revision object in a list.
     """
     self._update_candidate_range()
     candidate_range = [revision for revision in
                        self.revisions[self.lkgr.list_index + 1:
                                       self.fkbr.list_index]
-                       if not revision.tested and not revision.failed]
+                       if not revision.failed]
     if len(candidate_range) == 1:
       return candidate_range[0]
     if len(candidate_range) == 0:
       return None
 
     default_revision = candidate_range[len(candidate_range) / 2]
 
     with self.api.m.step.nest(
         'Wiggling revision ' + default_revision.revision_string()):
       # We'll search up to 25% of the range (in either direction) to try and
@@ skipping 36 matching lines @@
       return True
     if (revision.good and revision.next_revision and
         revision.next_revision.bad):
       if (revision.next_revision.deps_change()
           and self._expand_deps_revisions(revision.next_revision)):
         return False
       self.culprit = revision.next_revision
       return True
     return False
 
-  def wait_for_all(self, revision_list):
-    """Waits for all revisions in list to finish."""
-    for r in revision_list:
-      self.wait_for(r)
-
-  def wait_for(self, revision, nest_check=True):
-    """Waits for the revision to finish its job."""
-    if nest_check and not self.flags.get(
-        'do_not_nest_wait_for_revision'):  # pragma: no cover
-      with self.api.m.step.nest('Waiting for ' + revision.revision_string()):
-        return self.wait_for(revision, nest_check=False)
-    while True:
-      revision.update_status()
-      if revision.in_progress:
-        self.api.m.python.inline(
-            'sleeping',
-            """
-            import sys
-            import time
-            time.sleep(20*60)
-            sys.exit(0)
-            """)
-      else:
-        break
-
   def _update_candidate_range(self):
     """Updates lkgr and fkbr (last known good/first known bad) revisions.
 
     lkgr and fkbr are 'pointers' to the appropriate RevisionState objects in
     bisectors.revisions."""
     for r in self.revisions:
-      if r.tested:
+      if r.test_run_count:
         if r.good:
           self.lkgr = r
         elif r.bad:
           self.fkbr = r
           break
     assert self.lkgr and self.fkbr
 
   def get_perf_tester_name(self):
     """Gets the name of the tester bot (on tryserver.chromium.perf) to use.
 
@@ skipping 84 matching lines @@
     assert len(result_code) <= 20
     if result_code not in self.result_codes:
       self.result_codes.add(result_code)
       properties = self.api.m.step.active_result.presentation.properties
       properties['extra_result_code'] = sorted(self.result_codes)
 
   def get_result(self):
     """Returns the results as a jsonable object."""
     config = self.bisect_config
     results_confidence = 0
-    if self.culprit:
-      results_confidence = self.api.m.math_utils.confidence_score(
-          self.lkgr.values, self.fkbr.values)
 
     if self.failed:
       status = 'failed'
     elif self.bisect_over:
       status = 'completed'
     else:
       status = 'started'
 
     aborted_reason = None
     if self.failed_initial_confidence:
       aborted_reason = _FAILED_INITIAL_CONFIDENCE_ABORT_REASON
     elif self.failed_direction:
       aborted_reason = _DIRECTION_OF_IMPROVEMENT_ABORT_REASON
     return {
         'try_job_id': config.get('try_job_id'),
         'bug_id': config.get('bug_id'),
         'status': status,
         'buildbot_log_url': self._get_build_url(),
         'bisect_bot': self.get_perf_tester_name(),
         'command': config['command'],
         'test_type': config['test_type'],
         'metric': config['metric'],
         'change': self.relative_change,
-        'score': results_confidence,
         'good_revision': self.good_rev.commit_hash,
         'bad_revision': self.bad_rev.commit_hash,
         'warnings': self.warnings,
         'aborted_reason': aborted_reason,
         'culprit_data': self._culprit_data(),
         'revision_data': self._revision_data()
     }
 
   def _culprit_data(self):
     culprit = self.culprit
@@ skipping 11 matching lines @@
         'email': culprit_info['email'],
         'cl_date': culprit_info['date'],
         'commit_info': culprit_info['body'],
         'revisions_links': [],
         'cl': culprit.commit_hash
     }
 
   def _revision_data(self):
     revision_rows = []
     for r in self.revisions:
-      if r.tested or r.aborted:
+      if r.test_run_count:
         revision_rows.append({
             'depot_name': r.depot_name,
             'commit_hash': r.commit_hash,
             'revision_string': r.revision_string(),
-            'mean_value': r.mean_value,
+            'mean_value': r.mean,
             'std_dev': r.std_dev,
-            'values': r.values,
+            'values': r.debug_values,
             'result': 'good' if r.good else 'bad' if r.bad else 'unknown',
         })
     return revision_rows
 
   def _get_build_url(self):
     properties = self.api.m.properties
     bot_url = properties.get('buildbotURL',
                              'http://build.chromium.org/p/chromium/')
     builder_name = urllib.quote(properties.get('buildername', ''))
     builder_number = str(properties.get('buildnumber', ''))
     return '%sbuilders/%s/builds/%s' % (bot_url, builder_name, builder_number)