[Findit] Add a sub-pipeline to detect first-known failure.

[Findit] Add a sub-pipeline to detect first-known failure.

0. Add a base class for build-related models.
1. Split part of build.py into build_analysis.py
2. Add a sub-pipeline to detect first-known step-level failure.

BUG=434918
Committed: https://chromium.googlesource.com/infra/infra/+/ed1bf763570e13466c598108b184dd3b109df63e

[stgao, 2014-12-23 23:46:31]

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

[stgao, 2014-12-23 23:46:32]

ptal

[stgao, 2014-12-23 23:46:40]

Patchset #1 (id:1) has been deleted

[qyearsley, 2014-12-29 19:44:54]

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
File appengine/findit/model/build.py (right):

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build.py:18: """Create a Build instance, but not save it
to datastore."""
1. In general, the verb for a docstring is in third-person singular form, e.g.
"Creates a Build instance...".

2. s/but not/but does not/

IMO, this whole docstring is probably unnecessary (same in build.py). You could
probably remove this docstring entirely and it would still be pretty clear what
the function does just by looking at the one line below.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
File appengine/findit/model/build_analysis.py (right):

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build_analysis.py:11: """Represent an analysis of a build
cycle of a builder in a waterfall."""
As for functions, I think that verbs in docstrings for classes should also be in
third-person singular form -- "Represent" -> "Represents".

You can change this in build.py as well.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build_analysis.py:21: """Create a BuildAnalysis instance,
but not save it to datastore."""
You can change this to:
"""Creates a BuildAnalysis instance, but does not save it to the datastore."""
Or, just remove it.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build_analysis.py:43: build_number =
ndb.IntegerProperty(indexed=False)
Since master_name, builder_name and build_number are all already in the key,
it's unnecessary to store them separately -- you can use ndb.ComputedProperty,
and they'll be treated the same as any other property:

  @ndb.ComputedProperty
  def master_name(self):
    return self.key.parent().parent().id()

  @ndb.ComputedProperty
  def builder_name(self):
    return self.key.parent().id()

  @ndb.ComputedProperty
  def build_number(self):
    return self.key.id()

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right):

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:41:
DOWNLOAD_INTERVAL_SECONDS = 5
If these aren't used in any other modules, you can mark them as private:

_MEMCACHE_MASTER_DOWNLOAD_LOCK = 'master-download-lock-%s'
_MEMCACHE_MASTER_DOWNLOAD_EXPIRATION_SECONDS = 60 * 60
_DOWNLOAD_INTERVAL_SECONDS = 5

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:51: """
I might put WaitUntilDownloadAllowed below where it's used in
DetectFirstFailure, because in general I prefer putting helper functions after
the place where they're used.

Also, if it's not used in other modules, it could be private.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:79: class
DetectFirstFailure(BasePipeline):
How about the name DetectFirstFailurePipeline? (It seems like the name of a
class should always be a noun phrase, not a verb phrase.)

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:83: """Return a
BuildInfo instance for the specified build."""
Return -> Returns

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:139: # }
Is this an example of what the `builds` dict below might look like? If so, maybe
you should explicitly say this.

# Save blame list and tested chromium revision for builds.
# This will be saved in a dict that looks like this:
# ...

If you extract these few lines to a sub-function, then this could be put into
the Returns part of the docstring.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:161: for i in
range(20):
This 20 should probably be a module-level constant.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:193: return
failure_info
This function is very long, and would probably be easier to understand if it
were broken into some sub-functions. Possible candidates for sub-functions:

Lines 133-143: Constructing the builds dict
Lines 147-158: Constructing the failed_steps dict
Lines 162-189: Looking back at a particular past build number in order to see
whether it passed or failed.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:212: # Arguments
number differs from overridden method - pylint: disable=W0221
Cool, I didn't know you could add a comment on the same line as a pylint pragma.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:219: """Check
status of analysis for the build and decide if a new one is needed.
Check -> Checks, decide -> decides (third-person singular form.)

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:223: build
(Build): the build as specified by the input.
Make sure to update the docstring to reflect the current state of the code.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:225: """
Similar to WaitUntilDownloadAllowed, I think NeedANewAnalysis could be put below
where it's used (below ScheduleAnalysisIfNeeded) and make it private if
possible.

Also, it would be easier to understand this function if it only returned a
boolean instead of a pair, and if it were named "NeedsNewAnalysis". To make it
return only a boolean, you could make it take a BuildAnalysis (or None), and
call BuildAnalysis.GetBuildAnalysis before calling this function.

[stgao, 2015-01-03 01:31:47]

Hi Quinten,

I've addressed your comments and added unittest.
ptal when you have a cycle.

Thanks,
Shuotao Gao

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
File appengine/findit/model/build.py (right):

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build.py:18: """Create a Build instance, but not save it
to datastore."""
On 2014/12/29 19:44:53, qyearsley wrote:
> 1. In general, the verb for a docstring is in third-person singular form, e.g.
> "Creates a Build instance...".
Good catch!

> 
> 2. s/but not/but does not/
> 
> IMO, this whole docstring is probably unnecessary (same in build.py). You
could
> probably remove this docstring entirely and it would still be pretty clear
what
> the function does just by looking at the one line below.

Docstring Removed.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
File appengine/findit/model/build_analysis.py (right):

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build_analysis.py:11: """Represent an analysis of a build
cycle of a builder in a waterfall."""
On 2014/12/29 19:44:53, qyearsley wrote:
> As for functions, I think that verbs in docstrings for classes should also be
in
> third-person singular form -- "Represent" -> "Represents".
> 
> You can change this in build.py as well.

Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build_analysis.py:21: """Create a BuildAnalysis instance,
but not save it to datastore."""
On 2014/12/29 19:44:53, qyearsley wrote:
> You can change this to:
> """Creates a BuildAnalysis instance, but does not save it to the datastore."""
> Or, just remove it.
> 

Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/model/b...
appengine/findit/model/build_analysis.py:43: build_number =
ndb.IntegerProperty(indexed=False)
On 2014/12/29 19:44:53, qyearsley wrote:
> Since master_name, builder_name and build_number are all already in the key,
> it's unnecessary to store them separately -- you can use ndb.ComputedProperty,
> and they'll be treated the same as any other property:
> 
>   @ndb.ComputedProperty
>   def master_name(self):
>     return self.key.parent().parent().id()
> 
>   @ndb.ComputedProperty
>   def builder_name(self):
>     return self.key.parent().id()
> 
>   @ndb.ComputedProperty
>   def build_number(self):
>     return self.key.id()

Good idea! I created a base class for this, because Build and BuildAnalysis
could share the code. Later, another two models Step/Test could share the code
too.

By "store them separately", I assume you meant setting them as separate fields
in the model, rather than storing them in the datastore. Computed properties
will be stored in datastore too.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right):

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:41:
DOWNLOAD_INTERVAL_SECONDS = 5
On 2014/12/29 19:44:54, qyearsley wrote:
> If these aren't used in any other modules, you can mark them as private:
> 
> _MEMCACHE_MASTER_DOWNLOAD_LOCK = 'master-download-lock-%s'
> _MEMCACHE_MASTER_DOWNLOAD_EXPIRATION_SECONDS = 60 * 60
> _DOWNLOAD_INTERVAL_SECONDS = 5

Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:51: """
On 2014/12/29 19:44:53, qyearsley wrote:
> I might put WaitUntilDownloadAllowed below where it's used in
> DetectFirstFailure, because in general I prefer putting helper functions after
> the place where they're used.

In infra repo, it seems the convention is to put helper functions before where
they are used.
As findit is in infra repo, I'd follow the convention here :)

> 
> Also, if it's not used in other modules, it could be private.
Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:79: class
DetectFirstFailure(BasePipeline):
On 2014/12/29 19:44:54, qyearsley wrote:
> How about the name DetectFirstFailurePipeline? (It seems like the name of a
> class should always be a noun phrase, not a verb phrase.)

Good point. Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:83: """Return a
BuildInfo instance for the specified build."""
On 2014/12/29 19:44:53, qyearsley wrote:
> Return -> Returns

Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:139: # }
On 2014/12/29 19:44:53, qyearsley wrote:
> Is this an example of what the `builds` dict below might look like? If so,
maybe
> you should explicitly say this.
> 
> # Save blame list and tested chromium revision for builds.
> # This will be saved in a dict that looks like this:
> # ...
> 
> If you extract these few lines to a sub-function, then this could be put into
> the Returns part of the docstring.

Done. Moved to a helper function.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:161: for i in
range(20):
On 2014/12/29 19:44:54, qyearsley wrote:
> This 20 should probably be a module-level constant.

Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:193: return
failure_info
On 2014/12/29 19:44:53, qyearsley wrote:
> This function is very long, and would probably be easier to understand if it
> were broken into some sub-functions. Possible candidates for sub-functions:
> 
> Lines 133-143: Constructing the builds dict
> Lines 147-158: Constructing the failed_steps dict
> Lines 162-189: Looking back at a particular past build number in order to see
> whether it passed or failed.

Done. Hope the code is easier to understand.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:212: # Arguments
number differs from overridden method - pylint: disable=W0221
On 2014/12/29 19:44:54, qyearsley wrote:
> Cool, I didn't know you could add a comment on the same line as a pylint
pragma.

Yes, they could fit into one line.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:219: """Check
status of analysis for the build and decide if a new one is needed.
On 2014/12/29 19:44:54, qyearsley wrote:
> Check -> Checks, decide -> decides (third-person singular form.)

Done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:223: build
(Build): the build as specified by the input.
On 2014/12/29 19:44:53, qyearsley wrote:
> Make sure to update the docstring to reflect the current state of the code.

Oops, done.

https://codereview.chromium.org/820113002/diff/20001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:225: """
On 2014/12/29 19:44:54, qyearsley wrote:
> Similar to WaitUntilDownloadAllowed, I think NeedANewAnalysis could be put
below
> where it's used (below ScheduleAnalysisIfNeeded) and make it private if
> possible.

I intentionally made this function public for testing. Last time, pylint
complained that testing code accesses this private function.
(Same for other functions that are unit-tested.)

> 
> Also, it would be easier to understand this function if it only returned a
> boolean instead of a pair, and if it were named "NeedsNewAnalysis". To make it
> return only a boolean, you could make it take a BuildAnalysis (or None), and
> call BuildAnalysis.GetBuildAnalysis before calling this function.

Good point. I made this function return a boolean only now.
The caller could just call BuildAnalysis.GetBuildAnalysis again. That's fine
too.

But we couldn't pass in a BuildAnalysis. Because this function is run in a
transaction to avoid scheduling more than two analysis for the same build
failure. If two concurrent analysis requests for the same build are sent to
findit and a BuildAnalysis instance is passed into this function, two analysis
might get scheduled.

[stgao, 2015-01-06 23:54:27]

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right):

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:84: class
DetectFirstFailurePipeline(BasePipeline):
I'm planning to move sub-pipelines to separate modules and import them in this
root pipeline.

[qyearsley, 2015-01-07 06:59:46]

LGTM, with a couple comments.

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/model/b...
File appengine/findit/model/base_model.py (right):

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/model/b...
appengine/findit/model/base_model.py:1: # Copyright (c) 2014 The Chromium
Authors. All rights reserved.
Note, the "(c)" isn't necessary, and can be removed from the header in all
files.

http://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/model/b...
appengine/findit/model/base_model.py:9: """A base class to provide computed
properties from the key.
This looks like a good idea. Possible alternative names: BaseBuildModel or
BuildBaseModel (BaseModel should be fine too).

Currently the two subclasses are Build and BuildAnalysis, right? But if you had
any other build-specific models then they could also have similar keys.


Note about one problem we ran into with the perf dashboard: all entities under
one top-level key (in this case, master name) are part of one "entity group",
and there's a limit to the number of puts per second within one entity group.

This was a problem for the dashboard only because points were added at a high
rate. If there's ever going to be a time when entities are added at a high rate,
then you could make your model so that they're not nested. For the dashboard
points, we switched to keys like:
  ndb.Key('KindName', 'master/bot/test/specific/path', 'Row', 289083)

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
File appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py
(right):

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:80:
f.read())
It's not very clear what's happening here --
1) Maybe you could extract a helper function?
2) What does urlfetch.register_handler do?

[stgao, 2015-01-09 01:24:08]

Patchset #6 (id:120001) has been deleted

[stgao, 2015-01-09 01:24:16]

Patchset #5 (id:100001) has been deleted

[stgao, 2015-01-09 01:28:23]

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/model/b...
File appengine/findit/model/base_model.py (right):

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/model/b...
appengine/findit/model/base_model.py:1: # Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/01/07 06:59:46, qyearsley wrote:
> Note, the "(c)" isn't necessary, and can be removed from the header in all
> files.
> 
> http://www.chromium.org/developers/coding-style#TOC-File-headers

Good catch.
Done for files in this CL.
For other files in Findit, I will do it in a separate CL.

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/model/b...
appengine/findit/model/base_model.py:9: """A base class to provide computed
properties from the key.
On 2015/01/07 06:59:46, qyearsley wrote:
> This looks like a good idea. Possible alternative names: BaseBuildModel or
> BuildBaseModel (BaseModel should be fine too).
I like BaseBuildModel better.
Done.

> 
> Currently the two subclasses are Build and BuildAnalysis, right? But if you
had
> any other build-specific models then they could also have similar keys.
Yes, this base class is for models Step/Test.

> 
> 
> Note about one problem we ran into with the perf dashboard: all entities under
> one top-level key (in this case, master name) are part of one "entity group",
> and there's a limit to the number of puts per second within one entity group.
> 
> This was a problem for the dashboard only because points were added at a high
> rate. If there's ever going to be a time when entities are added at a high
rate,
> then you could make your model so that they're not nested. For the dashboard
> points, we switched to keys like:
>   ndb.Key('KindName', 'master/bot/test/specific/path', 'Row', 289083)

Good catch!
For scaling, we'd better "keep entity groups small"
(https://cloud.google.com/appengine/articles/scaling/contention).

I'd switch to keys like:
   ndb.Key('Build', 'master/builder/build', 'optional_KindName', optional_id,
...)

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right):

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
appengine/findit/waterfall/build_failure_analysis_pipelines.py:84: class
DetectFirstFailurePipeline(BasePipeline):
On 2015/01/06 23:54:26, Shuotao wrote:
> I'm planning to move sub-pipelines to separate modules and import them in this
> root pipeline.

Done.

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
File appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py
(right):

https://codereview.chromium.org/820113002/diff/80001/appengine/findit/waterfa...
appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:80:
f.read())
On 2015/01/07 06:59:46, qyearsley wrote:
> It's not very clear what's happening here --
> 1) Maybe you could extract a helper function?
> 2) What does urlfetch.register_handler do?

I add a comment about the purpose of the code.
urlfetch.register_handler is to map a url to the dummy data for HTTP response.

[stgao, 2015-01-09 01:31:01]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2015-01-09 01:31:11]

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

[commit-bot: I haz the power, 2015-01-09 01:33:58]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/infra/infra/+/ed1bf763570e13466c598108b184d...