Separates faster and slower bench alerts; sorts by delta.

bench/check_bench_regressions.py

 '''
 Created on May 16, 2011
 
 @author: bungeman
 '''
 import bench_util
 import getopt
 import httplib
 import itertools
 import json
 import os
 import re
 import sys
 import urllib
 import urllib2
 import xml.sax.saxutils
 
 # Maximum expected number of characters we expect in an svn revision.
 MAX_SVN_REV_LENGTH = 5
 
+# Indices for getting elements from bench expectation files.
+# See bench_expectations_<builder>.txt for details.
+EXPECTED_IDX = -3
+LB_IDX = -2
+UB_IDX = -1
+
 def usage():
     """Prints simple usage information."""
 
     print '-a <representation_alg> bench representation algorithm to use. '
     print '   Defaults to "25th". See bench_util.py for details.'
     print '-b <builder> name of the builder whose bench data we are checking.'
     print '-d <dir> a directory containing bench_<revision>_<scalar> files.'
     print '-e <file> file containing expected bench builder values/ranges.'
     print '   Will raise exception if actual bench values are out of range.'
     print '   See bench_expectations_<builder>.txt for data format / examples.'
@@ skipping 96 matching lines @@
         elements = expectation.strip().split(',')
         if not elements[0] or elements[0].startswith('#'):
             continue
         if len(elements) != 5:
             raise Exception("Invalid expectation line format: %s" %
                             expectation)
         bench_entry = elements[0] + ',' + elements[1]
         if bench_entry in expectations:
             raise Exception("Dup entries for bench expectation %s" %
                             bench_entry)
-        # [<Bench_BmpConfig_TimeType>,<Platform-Alg>] -> (LB, UB)
-        expectations[bench_entry] = (float(elements[-2]),
-                                     float(elements[-1]))
+        # [<Bench_BmpConfig_TimeType>,<Platform-Alg>] -> (LB, UB, EXPECTED)
+        expectations[bench_entry] = (float(elements[LB_IDX]),
+                                     float(elements[UB_IDX]),
+                                     float(elements[EXPECTED_IDX]))
 
-def check_expectations(lines, expectations, revision, key_suffix):
+def check_expectations(lines, expectations, key_suffix):
     """Check if there are benches in the given revising out of range.

[epoger, 2014/02/19 21:27:51]

revising -> revision ?

Actually, revision disappeared from the arg list anyway.  So I guess this should
read:

Check if any bench results are outside of expected range.

[benchen, 2014/02/19 21:47:54]

Done.

+
+    For each input line in lines, checks the expectations dictionary to see if
+    the bench is out of the given range.
+
+    Args:
+      lines: dictionary mapping Label objects to the bench values.
+      expectations: dictionary returned by read_expectations().
+      key_suffix: string of <Platform>-<Alg> containing the bot platform and the
+        bench representation algorithm (default to "25th").

[epoger, 2014/02/19 21:27:51]

I don't see anything about defaulting to "25th" in this function.  Maybe just
remove "(default to "25th")" from this arg description?

[benchen, 2014/02/19 21:47:54]

Agreed. Done.
On 2014/02/19 21:27:51, epoger wrote:

+
+    Returns:
+      No return value.
+
+    Raises:
+      Exception containing bench data that are out of range, if nonempty.

[epoger, 2014/02/19 21:27:51]

nonempty -> any

[benchen, 2014/02/19 21:47:54]

Done.

     """
     # The platform for this bot, to pass to the dashboard plot.
     platform = key_suffix[ : key_suffix.rfind('-')]
-    exceptions = []
+    # Tuple of dictionaries recording exceptions that are slower and faster,
+    # respectively. Each dictionary maps off_ratio (ratio of actual to expected)
+    # to a list of correspondig exception messages.

[epoger, 2014/02/19 21:27:51]

correspondig -> corresponding

[benchen, 2014/02/19 21:47:54]

Done. How did you catch this one?
On 2014/02/19 21:27:51, epoger wrote:

+    exceptions = ({}, {})
     for line in lines:
         line_str = str(line)
         line_str = line_str[ : line_str.find('_{')]
         bench_platform_key = line_str + ',' + key_suffix
         if bench_platform_key not in expectations:
             continue
         this_bench_value = lines[line]
-        this_min, this_max = expectations[bench_platform_key]
+        this_min, this_max, this_expected = expectations[bench_platform_key]
         if this_bench_value < this_min or this_bench_value > this_max:
-            exception = 'Bench %s value %s out of range [%s, %s].' % (
-                bench_platform_key, this_bench_value, this_min, this_max)
-            exceptions.append(exception)
-    if exceptions:
-        raise Exception('Bench values out of range:\n' +
-                        '\n'.join(exceptions))
+            off_ratio = this_bench_value / this_expected
+            exception = 'Bench %s out of range [%s, %s] (%s vs %s, %s%%).' % (
+                bench_platform_key, this_min, this_max, this_bench_value,
+                this_expected, (off_ratio - 1) * 100)
+            if off_ratio > 1:  # Bench is slower.
+                exceptions[0].setdefault(off_ratio, []).append(exception)

[epoger, 2014/02/19 21:27:51]

Rather than magic values 0 and 1, please use defined constants or a named tuple.
 Or just create two separate dictionary variables (slower_than_expected,
faster_than_expected).

[benchen, 2014/02/19 21:47:54]

Done using constants.
On 2014/02/19 21:27:51, epoger wrote:

+            else:
+                exceptions[1].setdefault(off_ratio, []).append(exception)
+    outputs = []
+    for i in [0, 1]:
+      if exceptions[i]:
+          ratios = exceptions[i].keys()
+          ratios.sort(reverse=True)
+          li = []
+          for ratio in ratios:
+              li.extend(exceptions[i][ratio])
+          header = '%s Slower benche(s) (Sorted):' % len(li)

[epoger, 2014/02/19 21:27:51]

I think this would be clearer:

'%s benches got slower (sorted by %% difference):'

Part of this is: I think it's easier to read if you just leave out the "(s)"
business to account for singular-vs-plural.  I think that's distracting.

[benchen, 2014/02/19 21:47:54]

Agreed. Done.
On 2014/02/19 21:27:51, epoger wrote:

+          if i == 1:
+              header = header.replace('Slower', 'Faster')
+          outputs.extend(['', header] + li)
+
+    if outputs:
+        raise Exception('\n'.join(outputs))
 
 def main():
     """Parses command line and checks bench expectations."""
     try:
         opts, _ = getopt.getopt(sys.argv[1:],
                                 "a:b:d:e:r:",
                                 "default-setting=")
     except getopt.GetoptError, err:
         print str(err)
         usage()
@@ skipping 31 matching lines @@
     platform_and_alg = bot + '-' + rep
 
     data_points = parse_dir(directory,
                             {},  # Sets default settings to empty.
                             rev,
                             rep)
 
     bench_dict = create_bench_dict(data_points)
 
     if bench_expectations:
-        check_expectations(bench_dict, bench_expectations, rev,
-                           platform_and_alg)
+        check_expectations(bench_dict, bench_expectations, platform_and_alg)
 
 
 if __name__ == "__main__":
     main()