[bisect] - First pass bisect functional breakages.

First pass bisect functional breakages.

Added a new --bisect_mode flag to the script that allows the bisect to go off changes in mean, standard deviation, or return code. Bisecting off standard deviation may be helpful in identifying situations where the amount of noise in a test has increased. Bisecting return codes will help track down functional breakages.

NOTRY=true
BUG=366713
Example output:

Bisect could not reproduce a change in the out/out metric.

Confidence in Bisection Results: 0%

To reproduce locally:
$ echo RESULT out: out= 0 ms

Tested commits:
         Depot                                        Commit SHA                                   Std. Dev        Mean         State    
        chromium                       50845b603a3faeb56d7f531c8a2b9a7030ae63c2                     +-0.00         0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265006        +-0.00         0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265005        +-0.00         0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265004        +-0.00         0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265003        +-0.00         0.00                  

Average build time : 0:00:00
Average test time  : 0:00:00




Bisect could not reproduce a change in the return code.

Confidence in Bisection Results: 0%

To reproduce locally:
$ echo RESULT out: out= 0 ms

Tested commits:
         Depot                                        Commit SHA                                 Return Code       State    
        chromium                       50845b603a3faeb56d7f531c8a2b9a7030ae63c2                        0                    
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265006           0                    
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265005           0                    
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265004           0                    
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265003           0                    

Average build time : 0:00:00
Average test time  : 0:00:00

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266629

[tonyg, 2014-04-25 20:38:42]

lgtm

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:2050: output_of_all_runs += output
I'm not following why this block moved into the try-except. Can you explain?

In general, code inside try blocks should be minimized.

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:3032: self._PrintConfidence(results_dict)
Looking at the output in the CL description, it seems like we should omit the
confidence when we indicate that we failed to repro. Because the 0% confidence
kind of contradicts the previous statement.

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:3069: def _PrintTestedCommitsHeader(self):
These are now fairly hard to read and a little redundant. Do you think a method
something like this would help?

def PrintTable(column_widths, row_data):
  ...

PrintTable([20, 70, 12, 14, 13],
           [['Depot', 'Commit SHA', ...],
            [current_data['depot'], ...],
            ...])

[shatch, 2014-04-25 22:46:02]

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:2050: output_of_all_runs += output
On 2014/04/25 20:38:42, tonyg wrote:
> I'm not following why this block moved into the try-except. Can you explain?
> 
> In general, code inside try blocks should be minimized.

Oops, leftover from earlier version. Also moved the if statements out, since you
mentioned it.

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:3032: self._PrintConfidence(results_dict)
On 2014/04/25 20:38:42, tonyg wrote:
> Looking at the output in the CL description, it seems like we should omit the
> confidence when we indicate that we failed to repro. Because the 0% confidence
> kind of contradicts the previous statement.

Done.

https://codereview.chromium.org/255943002/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:3069: def _PrintTestedCommitsHeader(self):
On 2014/04/25 20:38:42, tonyg wrote:
> These are now fairly hard to read and a little redundant. Do you think a
method
> something like this would help?
> 
> def PrintTable(column_widths, row_data):
>   ...
> 
> PrintTable([20, 70, 12, 14, 13],
>            [['Depot', 'Commit SHA', ...],
>             [current_data['depot'], ...],
>             ...])

Yeah, looks a lot more readable this way.

[tonyg, 2014-04-25 23:12:07]

lgtm

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:3069: def _PrintTable(self, column_widths,
row_data):
Oops, I was thinking this would take all rows instead of one row. That way the
column widths would be tied together. But your way is fine too. If you keep this
factoring, I'd recommend just calling this PrintTableRow or something.

[shatch, 2014-04-25 23:15:10]

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:3069: def _PrintTable(self, column_widths,
row_data):
On 2014/04/25 23:12:07, tonyg wrote:
> Oops, I was thinking this would take all rows instead of one row. That way the
> column widths would be tied together. But your way is fine too. If you keep
this
> factoring, I'd recommend just calling this PrintTableRow or something.

Done.

[qyearsley, 2014-04-25 23:53:28]

This works a little bit different from how I imagined the goal was: I thought it
would start in "mean mode", then as soon as any of the test runs fail, it
switches to "return code mode".

As it is, no bisects started from the bisect will specify a bisect-mode, so all
will be done in mean mode.

Did you intend to make it so that it would switch to return code mode whenever a
test run has a non-zero return code?

Regarding "standard deviation mode", I think this could be useful, but as it is
currently this won't be used in any bisect jobs started by the dashboard;
perhaps the script could switch to standard deviation mode in the beginning if
there's a large difference between the "good" and "bad" deviation, but a
relatively small difference between "good" and "bad" mean.

Or, perhaps it's possible for it to always take both mean and standard deviation
into account?

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:182: BISECT_MODE_RETURN_CODE = 'return_code'
Could add a comment about what these three constants are used for: they
represent the three possible values of of the --bisect-mode flag, which
determines what value is used to classify a revision as "good" or "bad", right?

https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:2098: }
It's potentially confusing that "mean" could be overall_return_code, since
that's not actually a mean, and also there no standard deviation or standard
error were actually calculated.

However, if you change this values dict to contain different key names, then
you'll have to change other things elsewhere, which would make this CL larger.

[qyearsley, 2014-04-25 23:55:01]

* In the 2nd paragraph I meant to say, "as it is, no bisects started from the
dashboard..."
And then I meant to ask, are there future plans for changing it so that it might
switch to return code mode even if it was started with mean mode?

[shatch, 2014-04-28 16:45:34]

On 2014/04/25 23:53:28, qyearsley wrote:
> This works a little bit different from how I imagined the goal was: I thought
it
> would start in "mean mode", then as soon as any of the test runs fail, it
> switches to "return code mode".
> 
> As it is, no bisects started from the bisect will specify a bisect-mode, so
all
> will be done in mean mode.
> 
> Did you intend to make it so that it would switch to return code mode whenever
a
> test run has a non-zero return code?
> 

Yeah, that was the original idea. After talking offline with Tony, we concluded
that automatically changing it up might lead to a lot of frustration. You can
imagine a flaky test and the bisect automatically switching from mean
regressions to return code, which obviously wasn't the intent. Explicitly
stating what you intended to bisect on is a lot more straightforward.


> Regarding "standard deviation mode", I think this could be useful, but as it
is
> currently this won't be used in any bisect jobs started by the dashboard;
> perhaps the script could switch to standard deviation mode in the beginning if
> there's a large difference between the "good" and "bad" deviation, but a
> relatively small difference between "good" and "bad" mean.
> 

The dashboard already knows quite a bit about the circumstances of the
regression, might make more sense for it to start the bisect config in the mode
it feels is mostly likely to catch the regression rather than have the script
try to infer it from the first 2 builds?


> Or, perhaps it's possible for it to always take both mean and standard
deviation
> into account?
> 

Not super clear how that would work, suggestions?

>
https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
> File tools/bisect-perf-regression.py (right):
> 
>
https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
> tools/bisect-perf-regression.py:182: BISECT_MODE_RETURN_CODE = 'return_code'
> Could add a comment about what these three constants are used for: they
> represent the three possible values of of the --bisect-mode flag, which
> determines what value is used to classify a revision as "good" or "bad",
right?
> 

Will follow up with CL.

>
https://codereview.chromium.org/255943002/diff/40001/tools/bisect-perf-regres...
> tools/bisect-perf-regression.py:2098: }
> It's potentially confusing that "mean" could be overall_return_code, since
> that's not actually a mean, and also there no standard deviation or standard
> error were actually calculated.
> 
> However, if you change this values dict to contain different key names, then
> you'll have to change other things elsewhere, which would make this CL larger.

Yeah, just wanted to keep it simple.

[qyearsley, 2014-04-28 17:52:50]

On 2014/04/28 16:45:34, shatch wrote:
> Yeah, that was the original idea. After talking offline with Tony, we
concluded
> that automatically changing it up might lead to a lot of frustration. You can
> imagine a flaky test and the bisect automatically switching from mean
> regressions to return code, which obviously wasn't the intent. Explicitly
> stating what you intended to bisect on is a lot more straightforward.

Sounds good. I expect that return code mode won't be specified from the
dashboard
though, since in general we expect the test to run successfully.


> The dashboard already knows quite a bit about the circumstances of the
> regression, might make more sense for it to start the bisect config in the
mode
> it feels is mostly likely to catch the regression rather than have the script
> try to infer it from the first 2 builds?

Right, when deciding whether there is "anomaly" in the first place, the
dashboard
uses the standard deviation of the left and right side segments (~6-44 points).

It's not using this, but it definitely could!

> > Or, perhaps it's possible for it to always take both mean and standard
> deviation
> > into account?
> > 
> 
> Not super clear how that would work, suggestions?

I'm not sure whether it's a way to do it, but mean and stddev could be
two dimensions, and the "difference" could be the Euclidean distance.
For example:

good: (mean 45, stddev 3)
bad: (mean 44, stddev 10)
revision under test: (mean 45, stddev 9)
distance from good: sqrt((45-45)**2 + (9-3)**2) = 6.0
distance from bad: sqrt((45-44)**2 + (9-10)**2) = 1.41421


Anyway, this CL LGTM; my only concern is what I mentioned above that using
 values = {
   'mean': overall_return_code,
   'std_err': 0.0,
   'std_dev': 0.0,
   'values': metric_values,
 }
might be confusing.

[qyearsley, 2014-04-28 17:58:20]

On 2014/04/28 17:52:50, qyearsley wrote:
> Right, when deciding whether there is "anomaly" in the first place, the
> dashboard uses the standard deviation of the left and right side segments
> (~6-44 points).
> 
> It's not using this, but it definitely could!
> 

I meant to write: it's not saving this information after the anomalies are
found, but if it did then in dashboard/bisect.py it would be possible to
use the relative difference between before/after stddev and mean to decide
on a config param to send.

(Although, if it works well to use a combination of mean and stddev
as mentioned above, then there would be no need to add code to the
dashboard for this.)

[shatch, 2014-04-28 19:33:17]

On 2014/04/28 17:52:50, qyearsley wrote:
> On 2014/04/28 16:45:34, shatch wrote:
> > Yeah, that was the original idea. After talking offline with Tony, we
> concluded
> > that automatically changing it up might lead to a lot of frustration. You
can
> > imagine a flaky test and the bisect automatically switching from mean
> > regressions to return code, which obviously wasn't the intent. Explicitly
> > stating what you intended to bisect on is a lot more straightforward.
> 
> Sounds good. I expect that return code mode won't be specified from the
> dashboard
> though, since in general we expect the test to run successfully.
> 

Yeah, for now sheriffs who want to bisect a functional breakage will need to
kick it off manually.

> 
> > The dashboard already knows quite a bit about the circumstances of the
> > regression, might make more sense for it to start the bisect config in the
> mode
> > it feels is mostly likely to catch the regression rather than have the
script
> > try to infer it from the first 2 builds?
> 
> Right, when deciding whether there is "anomaly" in the first place, the
> dashboard
> uses the standard deviation of the left and right side segments (~6-44
points).
> 
> It's not using this, but it definitely could!
> 
> > > Or, perhaps it's possible for it to always take both mean and standard
> > deviation
> > > into account?
> > > 
> > 
> > Not super clear how that would work, suggestions?
> 
> I'm not sure whether it's a way to do it, but mean and stddev could be
> two dimensions, and the "difference" could be the Euclidean distance.
> For example:
> 
> good: (mean 45, stddev 3)
> bad: (mean 44, stddev 10)
> revision under test: (mean 45, stddev 9)
> distance from good: sqrt((45-45)**2 + (9-3)**2) = 6.0
> distance from bad: sqrt((45-44)**2 + (9-10)**2) = 1.41421
> 

Oooh interesting, worth exploring in the future.

> 
> Anyway, this CL LGTM; my only concern is what I mentioned above that using
>  values = {
>    'mean': overall_return_code,
>    'std_err': 0.0,
>    'std_dev': 0.0,
>    'values': metric_values,
>  }
> might be confusing.

Will clean this up in a follow up CL.

[shatch, 2014-04-28 19:33:30]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/255943002/80001

[commit-bot: I haz the power, 2014-04-28 19:34:49]

Change committed as 266629