Separates faster and slower bench alerts; sorts by delta.

bench/check_bench_regressions.py

Index: bench/check_bench_regressions.py
diff --git a/bench/check_bench_regressions.py b/bench/check_bench_regressions.py
index b26eb56ae00ea71ed68f6d8bd0d037fd210069cb..250bd48b5d6db26bb3120c7cabe3bc42675b542a 100644
--- a/bench/check_bench_regressions.py
+++ b/bench/check_bench_regressions.py
@@ -18,6 +18,12 @@ 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."""
 
@@ -134,16 +140,35 @@ def read_expectations(expectations, filename):
         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('_{')]
@@ -151,14 +176,31 @@ def check_expectations(lines, expectations, revision, 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."""
@@ -210,8 +252,7 @@ def main():
     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__":