Iteratively increase sample size for good/bad classification.

scripts/slave/recipe_modules/auto_bisect/perf_revision_state.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 math
 import tempfile
 import os
 import uuid
 
 from . import revision_state
 
 if 'CACHE_TEST_RESULTS' in os.environ:  # pragma: no cover
   from . import test_results_cache
 
 
 class PerfRevisionState(revision_state.RevisionState):
 
   """Contains the state and results for one revision in a perf bisect job."""
 
   def __init__(self, *args, **kwargs):
     super(PerfRevisionState, self).__init__(*args, **kwargs)
     self.values = []
     self.mean_value = None
     self.std_dev = None
+    self.custom_reps = None
     self._test_config = None
 
-  def _read_test_results(self):
+  def _read_test_results(self, check_revision_goodness=True):
     """Gets the test results from GS and checks if the rev is good or bad."""
     test_results = self._get_test_results()
     # Results will contain the keys 'results' and 'output' where output is the
     # stdout of the command, and 'results' is itself a dict with the key
     # 'values' unless the test failed, in which case 'results' will contain
     # the 'error' key explaining the type of error.
     results = test_results['results']
     if results.get('errors'):
       self.status = PerfRevisionState.FAILED
       if 'MISSING_METRIC' in results.get('errors'):  # pragma: no cover
         self.bisector.surface_result('MISSING_METRIC')
       return
-    self.values = results['values']
+    self.values = self.values or [] + results['values']

[qyearsley, 2016/01/26 19:26:00]

`or` is less tightly binding than `+`, so this is equivalent to:

  self.values = self.values or ([] + results['values'])

And if results['values'] is a list, then ([] + results['values']) should be the
same as results['values'] (I think).

Should this be changed to the following?

  self.values = (self.values or []) + results['values']

[RobertoCN, 2016/02/01 17:29:50]

Done.

     if self.bisector.is_return_code_mode():
       retcodes = test_results['retcodes']
       overall_return_code = 0 if all(v == 0 for v in retcodes) else 1
       self.mean_value = overall_return_code
     elif self.values:
       api = self.bisector.api
       self.mean_value = api.m.math_utils.mean(self.values)
       self.std_dev = api.m.math_utils.standard_deviation(self.values)
     # Values were not found, but the test did not otherwise fail.
     else:
       self.status = PerfRevisionState.FAILED
       self.bisector.surface_result('MISSING_METRIC')
       return
     # We cannot test the goodness of the initial rev range.
-    if self.bisector.good_rev != self and self.bisector.bad_rev != self:
+    if (self.bisector.good_rev != self and self.bisector.bad_rev != self and
+        check_revision_goodness):
       if self._check_revision_good():
         self.good = True
       else:
         self.bad = True
 
   def _write_deps_patch_file(self, build_name):
     api = self.bisector.api
     file_name = str(api.m.path['tmp_base'].join(build_name + '.diff'))
     api.m.file.write('Saving diff patch for ' + str(self.revision_string),
                      file_name, self.deps_patch + self.deps_sha_patch)
@@ skipping 49 matching lines @@
         'metric',
         'max_time_minutes',
         'command',
         'repeat_count',
         'test_type'
     }
     for k, v in self.bisector.bisect_config.iteritems():
       if k in required_test_properties:
         result[k] = v
     self._test_config = result
+    if self.custom_reps: # pragma: no cover

[qyearsley, 2016/01/26 19:26:00]

Two spaces before #.

[RobertoCN, 2016/02/01 17:29:50]

Done.

+      result['repeat_count'] = self.custom_reps
+    else:
+      # We need at the very least 3 data points. (e.g. that's the minimum for
+      # shapiro's test for normality [even though we don't really use it])

[qyearsley, 2016/01/26 19:26:00]

The parenthetical part of this comment could probably be removed.

[RobertoCN, 2016/02/01 17:29:50]

Done.

+      result['repeat_count'] = max(int(result['repeat_count']), 3)
     return result
 
+  # It seems it should be okay to call this again to test the revision some
+  # more.

[qyearsley, 2016/01/26 19:26:00]

I'm not sure whether this comment is helpful; maybe it could be removed?

[RobertoCN, 2016/02/01 17:29:50]

Yep, I was supposed to remove it and forgot.

Done.

   def _do_test(self):
     """Posts a request to buildbot to download and perf-test this build."""
     if self.bisector.dummy_builds:
       self.job_name = self.commit_hash + '-test'
     elif 'CACHE_TEST_RESULTS' in os.environ:  # pragma: no cover
       self.job_name = test_results_cache.make_id(
           self.revision_string, self._get_bisect_config_for_tester())
     else:  # pragma: no cover
       self.job_name = uuid.uuid4().hex
     api = self.bisector.api
@@ skipping 35 matching lines @@
       builder = self.bisector.get_builder_bot_for_this_platform()
     if self.status == PerfRevisionState.TESTING:
       builder = self.bisector.get_perf_tester_name()
     return {
         'type': 'buildbot',
         'master': master,
         'builder': builder,
         'job_name': self.job_name,
     }
 
+  def retest(self, target_sample_size):  # pragma: no cover
+    self.status = PerfRevisionState.NEED_MORE_DATA
+    self.custom_reps = target_sample_size - len(self.values)
+    self.start_job()
+    self.bisector.wait_for_any([self])

[qyearsley, 2016/01/26 19:26:00]

This triggers the bisect to run another |self.custom_reps| times, right?

[RobertoCN, 2016/02/01 17:29:50]

Yeah.

+
   def _get_test_results(self):
     """Tries to get the results of a test job from cloud storage."""
     api = self.bisector.api
     try:
       stdout = api.m.raw_io.output()
       name = 'Get test results for build ' + self.commit_hash
       step_result = api.m.gsutil.cat(self.test_results_url, stdout=stdout,
                                      name=name)
     except api.m.step.StepFailure:  # pragma: no cover
       self.bisector.surface_result('TEST_FAILURE')
       return None
     else:
       return json.loads(step_result.stdout)
 
+  def _calculate_target_sample_size(self):  # pragma: no cover
+    # Make the smallest bigger than the largest, by increasing it at least 33%
+    sizes = map(lambda x: len(x.values),  [self.bisector.lkgr, self,

[qyearsley, 2016/01/26 19:26:00]

Extra space after the comma.

[RobertoCN, 2016/02/01 17:29:50]

Done.

+                                           self.bisector.fkbr])
+    smallest_sample_size = min(sizes)
+    target_sample_size = max(sizes) + 1
+    delta = target_sample_size - smallest_sample_size

[dtu, 2016/02/05 00:18:01]

delta is unused.

+    min_increase = math.ceil(smallest_sample_size/3.0)

[qyearsley, 2016/01/26 19:26:00]

Optional: could add spaces around the / operator.

[RobertoCN, 2016/02/01 17:29:50]

Done.

+    return max(smallest_sample_size + min_increase, target_sample_size)
+
   def _check_revision_good(self):

[qyearsley, 2016/01/26 19:26:00]

Parts of the previous docstring could be kept here -- overall, it still returns
True if it's going to be considered "good", and False otherwise.

[RobertoCN, 2016/02/01 17:29:51]

Done.

-    """Determines if a revision is good or bad.
-
-    Note that our current approach is to determine whether it is closer to
-    either the 'good' and 'bad' revisions given for the bisect job.
-
-    Returns:
-      True if this revision is closer to the initial good revision's value than
-      to the initial bad revision's value. False otherwise.
-    """
-    # TODO: Reevaluate this approach
-    bisector = self.bisector
-    distance_to_good = abs(self.mean_value - bisector.good_rev.mean_value)
-    distance_to_bad = abs(self.mean_value - bisector.bad_rev.mean_value)
-    if distance_to_good < distance_to_bad:
-      return True
-    return False
+    diff_from_good = self.bisector.significantly_different(
+        self.bisector.lkgr.values, self.values)
+    diff_from_bad = self.bisector.significantly_different(
+        self.bisector.fkbr.values, self.values)
+    while not (diff_from_good or diff_from_bad):  # pragma: no cover
+      target_sample_size = self._calculate_target_sample_size()
+      min(self.bisector.lkgr, self, self.bisector.fkbr,
+          key=lambda x: len(x.values)).retest(target_sample_size)
+      diff_from_good = self.bisector.significantly_different(
+          self.bisector.lkgr.values, self.values)
+      diff_from_bad = self.bisector.significantly_different(
+          self.bisector.fkbr.values, self.values)
+    if diff_from_good and diff_from_bad:

[qyearsley, 2016/01/26 19:26:00]

A blank line above this if block might slightly help with readability.

[RobertoCN, 2016/02/01 17:29:50]

Done.

+      # Multiple regressions.
+      # For now, proceed bisecting the biggest difference of the means, and
+      # handle the secondary regression by spinning a second job.

[qyearsley, 2016/01/26 19:26:00]

No second job is started as of this CL, right?

[RobertoCN, 2016/02/01 17:29:50]

Correct, for now we'll go with the larger regression (calculated by distance of
means)

+      dist_from_good = abs(self.mean_value - self.bisector.lkgr.mean_value)
+      dist_from_bad = abs(self.mean_value - self.bisector.fkbr.mean_value)
+      if dist_from_good > dist_from_bad:
+        #self.bisector.handle_secondary_regression(self, self.bisector.fkbr)
+        return False
+      else:
+        #self.bisector.handle_secondary_regression(self.bisector.lkgr, self)
+        return True
+    return diff_from_bad  # pragma: no cover
 
   def __repr__(self):
     return ('PerfRevisionState(cp=%s, values=%r, mean_value=%r, std_dev=%r)' %
             (self.commit_pos, self.values, self.mean_value, self.std_dev))