Refactored auto_bisect bot

Refactored auto_bisect bot:

 - Extracted BisectPrinter, which contains everything related to printing
 - Extracted BisectState and RevisionState, which represent bisect-in-progress
 - Rewrote BisectResults - in particular split up GetRevisionDict, which is now its constructor
 - Added tests for BisectResults, fixed bugs in _FindOtherRegressions
 - Added tests for BisectState

R=qyearsley@chromium.org

Committed: https://crrev.com/cfc4b1f15ce8285cb5b902bc8851ee7f29664308
Cr-Commit-Position: refs/heads/master@{#300677}

[Sergiy Byelozyorov, 2014-10-20 22:44:35]

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

[qyearsley, 2014-10-21 05:09:11]

This is excellent.

We'll definitely want unit tests for these classes, and it might be a good idea
to add some in this CL, even though we want to try to submit this quickly.

Some methods which I think tests would be good for are
BisectState.CreateRevisionStatesAfter, and all of the complicated methods in
BisectResults.

Do you think it would be better/easier to break some of those methods down
further in order to write tests?

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2820: bisect_printer =
BisectPrinter(opts)
In this patchset, BisectPrinter takes a BisectPerformanceMetrics object as
input.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_printer.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:4: 
Could add a short intro to this file here -- the most important thing is that
the constructor takes a BisectPerformanceMetrics object, and it has just one
public method, which is FormatAndPrintResults, which takes a BisectResults
object and outputs the Results step.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:76: def __init__(self, bisect_perf_metrics):
Another possibility here: Make this constructor take an Options object and a
DepotDirectoryRegistry object instead of a whole BisectPerformanceMetrics
object. This could be done later if we want to do it.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:336: def FormatAndPrintResults(self,
bisect_results):
I think this might be better to put this at the top of the class, just below
__init__?

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:337: """Prints the results from a bisection
run in a readable format.
And it also prints the buildbot annotation for starting and ending the "Results"
step.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:340: bisect_results: The results from a
bisection test run.
You could note that this is expected to be a BisectResults object.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_results.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:20: error: Error message if the bisect
failed.
In BisectState, there's no blank line after "Properties". For consistency, you
could either remove the blank line here or maybe add one there?

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:25: state: BisectState object from this
these results were generated.
s/from this/from which/

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:51: repository in the bisect_state.
For consistency, you could also use the 4-space hanging indent here.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:145: revision_states: Sorted list of
RevisionState objects.
If this method were not a class method, then it could get this via
self.state.GetRevisionStates().

But in the interest of keeping this CL from including too many changes at once,
it would be good to keep it like this for now.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:256: for state in
rev_states[first_working_rev.index:]
Indent one more space over

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_state.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_state.py:40: """Initializes a new RevisionData object
with given revision data entries.
s/RevisionData/BisectState/

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_state.py:43: depot: Name of the depot used for initial
set of revision states.
This is always expected to be "chromium" for now, right?

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_state.py:44: revision: List of revisions used for
initial set of revision states.
s/revision/revisions/

[Sergiy Byelozyorov, 2014-10-21 12:27:43]

On 2014/10/21 05:09:11, qyearsley wrote:
> We'll definitely want unit tests for these classes, and it might be a good
idea
> to add some in this CL, even though we want to try to submit this quickly.



> Some methods which I think tests would be good for are
> BisectState.CreateRevisionStatesAfter, and all of the complicated methods in
> BisectResults.
Will add tests later today.


> Do you think it would be better/easier to break some of those methods down
> further in order to write tests?
BisectState.CreateRevisionStatesAfter is relatively simple. Most of he methods
in BisectResults are very hard to break into pieces. Perhaps only
_FindOtherRegressions can be subdivided - I'll look into that.

> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> File tools/auto_bisect/bisect_perf_regression.py (right):
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_perf_regression.py:2820: bisect_printer =
> BisectPrinter(opts)
> In this patchset, BisectPrinter takes a BisectPerformanceMetrics object as
> input.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> File tools/auto_bisect/bisect_printer.py (right):
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_printer.py:4: 
> Could add a short intro to this file here -- the most important thing is that
> the constructor takes a BisectPerformanceMetrics object, and it has just one
> public method, which is FormatAndPrintResults, which takes a BisectResults
> object and outputs the Results step.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_printer.py:76: def __init__(self,
bisect_perf_metrics):
> Another possibility here: Make this constructor take an Options object and a
> DepotDirectoryRegistry object instead of a whole BisectPerformanceMetrics
> object. This could be done later if we want to do it.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_printer.py:336: def FormatAndPrintResults(self,
> bisect_results):
> I think this might be better to put this at the top of the class, just below
> __init__?
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_printer.py:337: """Prints the results from a
bisection
> run in a readable format.
> And it also prints the buildbot annotation for starting and ending the
"Results"
> step.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_printer.py:340: bisect_results: The results from a
> bisection test run.
> You could note that this is expected to be a BisectResults object.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> File tools/auto_bisect/bisect_results.py (right):
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_results.py:20: error: Error message if the bisect
> failed.
> In BisectState, there's no blank line after "Properties". For consistency, you
> could either remove the blank line here or maybe add one there?
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_results.py:25: state: BisectState object from this
> these results were generated.
> s/from this/from which/
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_results.py:51: repository in the bisect_state.
> For consistency, you could also use the 4-space hanging indent here.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_results.py:145: revision_states: Sorted list of
> RevisionState objects.
> If this method were not a class method, then it could get this via
> self.state.GetRevisionStates().
> 
> But in the interest of keeping this CL from including too many changes at
once,
> it would be good to keep it like this for now.
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_results.py:256: for state in
> rev_states[first_working_rev.index:]
> Indent one more space over
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> File tools/auto_bisect/bisect_state.py (right):
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_state.py:40: """Initializes a new RevisionData object
> with given revision data entries.
> s/RevisionData/BisectState/
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_state.py:43: depot: Name of the depot used for
initial
> set of revision states.
> This is always expected to be "chromium" for now, right?
> 
>
https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
> tools/auto_bisect/bisect_state.py:44: revision: List of revisions used for
> initial set of revision states.
> s/revision/revisions/

[Sergiy Byelozyorov, 2014-10-21 12:28:05]

Forgot replies to comments.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2820: bisect_printer =
BisectPrinter(opts)
On 2014/10/21 05:09:10, qyearsley wrote:
> In this patchset, BisectPrinter takes a BisectPerformanceMetrics object as
> input.

Thanks. Nice catch.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_printer.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:4: 
On 2014/10/21 05:09:10, qyearsley wrote:
> Could add a short intro to this file here -- the most important thing is that
> the constructor takes a BisectPerformanceMetrics object, and it has just one
> public method, which is FormatAndPrintResults, which takes a BisectResults
> object and outputs the Results step.

Added a short info, but I find it redundant to add description of what is
already visible in the code. It doesn't add more information and will likely get
out of sync when the code changes.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:76: def __init__(self, bisect_perf_metrics):
On 2014/10/21 05:09:10, qyearsley wrote:
> Another possibility here: Make this constructor take an Options object and a
> DepotDirectoryRegistry object instead of a whole BisectPerformanceMetrics
> object. This could be done later if we want to do it.

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:336: def FormatAndPrintResults(self,
bisect_results):
On 2014/10/21 05:09:10, qyearsley wrote:
> I think this might be better to put this at the top of the class, just below
> __init__?

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:337: """Prints the results from a bisection
run in a readable format.
On 2014/10/21 05:09:10, qyearsley wrote:
> And it also prints the buildbot annotation for starting and ending the
"Results"
> step.

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_printer.py:340: bisect_results: The results from a
bisection test run.
On 2014/10/21 05:09:10, qyearsley wrote:
> You could note that this is expected to be a BisectResults object.

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_results.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:20: error: Error message if the bisect
failed.
On 2014/10/21 05:09:11, qyearsley wrote:
> In BisectState, there's no blank line after "Properties". For consistency, you
> could either remove the blank line here or maybe add one there?

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:25: state: BisectState object from this
these results were generated.
On 2014/10/21 05:09:10, qyearsley wrote:
> s/from this/from which/

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:51: repository in the bisect_state.
On 2014/10/21 05:09:11, qyearsley wrote:
> For consistency, you could also use the 4-space hanging indent here.

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:145: revision_states: Sorted list of
RevisionState objects.
On 2014/10/21 05:09:11, qyearsley wrote:
> If this method were not a class method, then it could get this via
> self.state.GetRevisionStates().
> 
> But in the interest of keeping this CL from including too many changes at
once,
> it would be good to keep it like this for now.

I've intentionally kept it as a classmethod. It makes sure that we don't forget
to set value to self.revision_states before calling this method.

In fact, at first I wanted to get rid of the BisectResults altogether and keep
of these methods as functions, but this made me use dict as a return type, which
I wasn't happy with.

Later, I've tried to have individual methods populate values in self, but that
made it error-prone as some methods expect certain values to be present, e.g.
_FindOtherRegressions depended on _ComputeRegressionStatistics to set
"bad_greater_than_good". This is "easy to use incorrectly", so I've ended up
having them take all necessary parameters instead.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results.py:256: for state in
rev_states[first_working_rev.index:]
On 2014/10/21 05:09:11, qyearsley wrote:
> Indent one more space over

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_state.py (right):

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_state.py:40: """Initializes a new RevisionData object
with given revision data entries.
On 2014/10/21 05:09:11, qyearsley wrote:
> s/RevisionData/BisectState/

Done.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_state.py:43: depot: Name of the depot used for initial
set of revision states.
On 2014/10/21 05:09:11, qyearsley wrote:
> This is always expected to be "chromium" for now, right?

Yes, but BisectState class is universal. It would work with any repository.

https://codereview.chromium.org/664753002/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_state.py:44: revision: List of revisions used for
initial set of revision states.
On 2014/10/21 05:09:11, qyearsley wrote:
> s/revision/revisions/

Done.

[Sergiy Byelozyorov, 2014-10-21 18:54:10]

Added test. PTAL

[qyearsley, 2014-10-21 19:29:57]

On 2014/10/21 18:54:10, Sergiy Byelozyorov wrote:
> Added test. PTAL

LGTM for committing now, all of the test methods are readable and test one thing
which can be figured out from the name.

[qyearsley, 2014-10-21 19:30:48]

On 2014/10/21 19:29:57, qyearsley wrote:
> On 2014/10/21 18:54:10, Sergiy Byelozyorov wrote:
> > Added test. PTAL
> 
> LGTM for committing now, all of the test methods are readable and test one
thing
> which can be figured out from the name.

Looks like bot_update step is failing in those try jobs though, probably need to
rebase and try again.

[qyearsley, 2014-10-21 19:30:48]

On 2014/10/21 19:29:57, qyearsley wrote:
> On 2014/10/21 18:54:10, Sergiy Byelozyorov wrote:
> > Added test. PTAL
> 
> LGTM for committing now, all of the test methods are readable and test one
thing
> which can be figured out from the name.

Looks like bot_update step is failing in those try jobs though, probably need to
rebase and try again.

[qyearsley, 2014-10-21 19:30:59]

https://codereview.chromium.org/664753002/diff/60001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_results_test.py (right):

https://codereview.chromium.org/664753002/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_results_test.py:223: def
testWarningForTooManyCulpritRevisions(self):
Too many culprit revisions right now means more than one culprit, right? Maybe
change to testWarningForMultipleCulpritRevisions

[qyearsley, 2014-10-21 23:42:35]

lgtm

https://codereview.chromium.org/664753002/diff/160001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_constants.py (right):

https://codereview.chromium.org/664753002/diff/160001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_constants.py:175: BISECT_MODE_RETURN_CODE =
'return_code'
Could these go into bisect_perf_regression.py? Also, maybe MAX_*_BUILD_TIME go
there as well?

If some constants are only used in one module, then putting them into that
module won't cause any circular imports, right?

https://codereview.chromium.org/664753002/diff/160001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_constants.py:226: RESULTS_BANNER = """
Could any of these strings go into the bisect_printer module?

https://codereview.chromium.org/664753002/diff/160001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/664753002/diff/160001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:73: )
I think that a four-space indentation might be preferred style here, since we've
been using four-space indentation for lists and dicts that take up multiple
lines.

Note: The style guide isn't super-clear on this. The google python style says 4
spaces; the Chromium style guide* says 2 spaces, but I interpret that as meaning
two spaces for indented if/for/def blocks etc., not for lists, tuples, etc.

Regarding imports in general, the Google Python Style Guide** says to import
only modules, not classes, functions or constants from modules. However, the
Chromium style guide says that PEP-8*** is the general authority, which says
importing classes etc. is OK.

My opinion: This is OK but I think an indent of 4 spaces is more consistent.
Also, for the list of constants to import, the fewer the better.

* http://www.chromium.org/developers/coding-style
** https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports
*** http://legacy.python.org/dev/peps/pep-0008/#imports

[Sergiy Byelozyorov, 2014-10-22 11:27:48]

I am not sure about the results of the tests. None of the errors seem to be
related to my changes. Also I am not sure if *_builder bots are relevant... PTAL
and if you think it's not relevant to my changes - please check the CQ.

[Sergiy Byelozyorov, 2014-10-22 13:47:21]

I've also noticed that try.py starts a large number of jobs on tryserver that do
not show up in CL. I've cancelled most of them.

[Sergiy Byelozyorov, 2014-10-22 13:54:16]

On 2014/10/22 11:27:48, Sergiy Byelozyorov wrote:
> I am not sure about the results of the tests. None of the errors seem to be
> related to my changes. Also I am not sure if *_builder bots are relevant...
PTAL
> and if you think it's not relevant to my changes - please check the CQ.

Ok. It seems that after rebase only android_nexus10_perf_bisect and
android_gn_perf_bisect are consistently failing. However, they are also failing
for other changes, e.g.
http://build.chromium.org/p/tryserver.chromium.perf/builders/android_gn_perf_...
and
http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus10_....
I assume that my changed are unrelated and they are broken for other reasons.
Committing this.

[Sergiy Byelozyorov, 2014-10-22 13:54:20]

The CQ bit was checked by sergiyb@chromium.org

[commit-bot: I haz the power, 2014-10-22 13:55:08]

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

[commit-bot: I haz the power, 2014-10-22 14:45:50]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-10-22 14:46:23]

Patchset 12 (id:??) landed as
https://crrev.com/cfc4b1f15ce8285cb5b902bc8851ee7f29664308
Cr-Commit-Position: refs/heads/master@{#300677}