Requiring confidence in initial regression range before bisecting.

Requiring confidence in initial regression range before bisecting.

BUG=422727
Committed: https://crrev.com/4436048a948a2010062d3c6a171775d9389fd732
Cr-Commit-Position: refs/heads/master@{#300196}

[RobertoCN, 2014-10-12 00:53:40]

robertocn@chromium.org changed reviewers:
+ qyearsley@chromium.org

[RobertoCN, 2014-10-12 00:53:40]

This still needs some proper tests, but please tell me if this seems kinda right
or if it's completely off.

[qyearsley, 2014-10-12 04:07:22]

This looks about right :-)

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:2451: if regression_confidence <
HIGH_CONFIDENCE:
I'm not 100% sure it's good practice to re-use the HIGH_CONFIDENCE here for this
slightly different purpose.

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:2455: 'bisecting a different
revision range.')
I think that this message can be improved. We want to communicate that:
   1. We couldn't reproduce it in this run with this test and metric and
platform combination.
   2. This gives evidence that there is no regression between "good" and "bad",
but it might still potentially be able to be reproduced on a different platform.

[RobertoCN, 2014-10-17 22:44:04]

Added some tests, re-based to work along the improvement_direction check.

Please review.

[RobertoCN, 2014-10-17 22:44:36]

robertocn@chromium.org changed reviewers:
+ chrisphan@chromium.org, prasadv@chromium.org

[RobertoCN, 2014-10-17 22:44:37]

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:2451: if regression_confidence <
HIGH_CONFIDENCE:
On 2014/10/12 04:07:21, qyearsley wrote:
> I'm not 100% sure it's good practice to re-use the HIGH_CONFIDENCE here for
this
> slightly different purpose.

Done.

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:2455: 'bisecting a different
revision range.')
On 2014/10/12 04:07:22, qyearsley wrote:
> I think that this message can be improved. We want to communicate that:
>    1. We couldn't reproduce it in this run with this test and metric and
> platform combination.
>    2. This gives evidence that there is no regression between "good" and
"bad",
> but it might still potentially be able to be reproduced on a different
platform.

Done.

[qyearsley, 2014-10-17 23:53:52]

LGTM with a suggestion about the message text.

Might want to wait for someone else to take a look as well.

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/644323002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:2455: 'bisecting a different
revision range.')
Sometimes when the bisect can't reproduce the regression, the next step to take
is to try on a different platform with the same revision range.

So, I suggest changing the last sentence here; either expand it to "Try
bisecting a different revision range or using a different platform." or just
remove it.

[RobertoCN, 2014-10-18 00:40:22]

> tools/auto_bisect/bisect_perf_regression.py:2455: 'bisecting a different
> revision range.')
> Sometimes when the bisect can't reproduce the regression, the next step to
take
> is to try on a different platform with the same revision range.
> 
> So, I suggest changing the last sentence here; either expand it to "Try
> bisecting a different revision range or using a different platform." or just
> remove it.

Did you see the latest patchset? It says 'We could not reproduce the regression
with this test/metric/platform combination with enough confidence. There's still
a chance that this is actually a regression, but you may need to bisect a
different platform.'

Isn't that clear enough?

[RobertoCN, 2014-10-18 00:41:18]

robertocn@chromium.org changed reviewers:
+ tonyg@chromium.org

[qyearsley, 2014-10-18 00:43:39]

On 2014/10/18 00:40:22, robertocn wrote:
> > tools/auto_bisect/bisect_perf_regression.py:2455: 'bisecting a different
> > revision range.')
> > Sometimes when the bisect can't reproduce the regression, the next step to
> take
> > is to try on a different platform with the same revision range.
> > 
> > So, I suggest changing the last sentence here; either expand it to "Try
> > bisecting a different revision range or using a different platform." or just
> > remove it.
> 
> Did you see the latest patchset? It says 'We could not reproduce the
regression
> with this test/metric/platform combination with enough confidence. There's
still
> a chance that this is actually a regression, but you may need to bisect a
> different platform.'
> 
> Isn't that clear enough?

Oh, sorry, I was looking at the old patch! Yep, the new one is clearer I think.

[tonyg, 2014-10-18 00:48:24]

lgtm

This should be a great shortcut :)

[RobertoCN, 2014-10-18 00:57:36]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2014-10-18 00:58:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644323002/110001

[commit-bot: I haz the power, 2014-10-18 01:27:41]

Committed patchset #4 (id:110001)

[commit-bot: I haz the power, 2014-10-18 01:28:33]

Patchset 4 (id:??) landed as
https://crrev.com/4436048a948a2010062d3c6a171775d9389fd732
Cr-Commit-Position: refs/heads/master@{#300196}

[prasadv, 2014-10-20 16:48:51]

A revert of this CL (patchset #4 id:110001) has been created in
https://codereview.chromium.org/661393003/ by prasadv@chromium.org.

The reason for reverting is: Bisect is broken because the ConfidenceScore
expects list of list, but here only list is passed.
http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisec....

[qyearsley, 2015-05-19 17:11:46]

On 2014/10/20 16:48:51, prasadv wrote:
> A revert of this CL (patchset #4 id:110001) has been created in
> https://codereview.chromium.org/661393003/ by mailto:prasadv@chromium.org.
> 
> The reason for reverting is: Bisect is broken because the ConfidenceScore
> expects list of list, but here only list is passed.
>
http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisec....

A fix was later submitted in: https://codereview.chromium.org/665893003