[Findit] Add a sub-pipeline to analyze failures caused by DEPS rolls.

[Findit] Add a sub-pipeline to analyze failures caused by DEPS rolls.

BUG=487762
Example result: https://stgao-dot-findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/builds/191

Committed: https://chromium.googlesource.com/infra/infra/+/447171024cfc9434723747d661631c40a14c9725

[stgao, 2015-05-28 23:57:53]

Patchset #1 (id:1) has been deleted

[stgao, 2015-05-28 23:59:52]

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

[stgao, 2015-05-28 23:59:53]

ptal
Will add unittest in next patch.

[chanli, 2015-05-29 01:00:08]

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:287: deps_info (dict):
Output of pipeline ExtractDEPSInfoPipeline.
Why I can't find where do you use this argument in the function?

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:327: # TODO(stgao): remove
hard-coded 'chromium' when DEPS file parsing is
does this todo relate to thi CL?

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/extract_deps_info_pipeline.py (right):

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:11: def
_GetOSPlatformName(master_name, builder_name):  # pragma: no cover
Any reason why do you want to skip this function from testing?

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:94: """A pipeline to
pull change log of CLs."""
I think this comment is not clear enough. From my understanding, we've already
have the change log when we begin to run this pipeline, right?

Maybe you actually mean DEPS Info?

[stgao, 2015-05-29 16:46:54]

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:287: deps_info (dict):
Output of pipeline ExtractDEPSInfoPipeline.
On 2015/05/29 01:00:07, chanli wrote:
> Why I can't find where do you use this argument in the function?

I added a TODO above for that.

However, I made a change to use it to check for failures caused by DEPS roll.

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:327: # TODO(stgao): remove
hard-coded 'chromium' when DEPS file parsing is
On 2015/05/29 01:00:07, chanli wrote:
> does this todo relate to thi CL?

More work is needed for this. I'd leave this as is for now.

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/extract_deps_info_pipeline.py (right):

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:11: def
_GetOSPlatformName(master_name, builder_name):  # pragma: no cover
On 2015/05/29 01:00:08, chanli wrote:
> Any reason why do you want to skip this function from testing?

Because this is just a workaround, it seems not worth it.
I could add some tests if that is preferred.

https://codereview.chromium.org/1154593005/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:94: """A pipeline to
pull change log of CLs."""
On 2015/05/29 01:00:08, chanli wrote:
> I think this comment is not clear enough. From my understanding, we've already
> have the change log when we begin to run this pipeline, right?
> 
> Maybe you actually mean DEPS Info?

oops, forgot to update it.

[chanli, 2015-05-29 18:47:55]

https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/extract_deps_info_pipeline.py (right):

https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:46: deps = {}
I get that the info you need here is less than the returned dict, but I think it
should be OK if you directly use that dict?

https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:94: """A pipeline to
extract DEPS information and rolls.."""
nit: 2 '.'s.

[stgao, 2015-05-29 19:51:18]

Patchset #3 (id:60001) has been deleted

[stgao, 2015-05-29 20:16:14]

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

[stgao, 2015-05-29 20:16:14]

+Quinten for committer review :)

https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/extract_deps_info_pipeline.py (right):

https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:46: deps = {}
On 2015/05/29 18:47:55, chanli wrote:
> I get that the info you need here is less than the returned dict, but I think
it
> should be OK if you directly use that dict?

Unfortunately, we couldn't do that.

Because appengine pipeline requires that the returned value by function run
below is in plain json and returning an object will lead to exception.

https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/extract_deps_info_pipeline.py:94: """A pipeline to
extract DEPS information and rolls.."""
On 2015/05/29 18:47:55, chanli wrote:
> nit: 2 '.'s.

Done.

[chanli, 2015-05-29 20:56:43]

On 2015/05/29 20:16:14, Shuotao wrote:
> +Quinten for committer review :)
> 
>
https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
> File appengine/findit/waterfall/extract_deps_info_pipeline.py (right):
> 
>
https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
> appengine/findit/waterfall/extract_deps_info_pipeline.py:46: deps = {}
> On 2015/05/29 18:47:55, chanli wrote:
> > I get that the info you need here is less than the returned dict, but I
think
> it
> > should be OK if you directly use that dict?
> 
> Unfortunately, we couldn't do that.
> 
> Because appengine pipeline requires that the returned value by function run
> below is in plain json and returning an object will lead to exception.
> 
>
https://codereview.chromium.org/1154593005/diff/40001/appengine/findit/waterf...
> appengine/findit/waterfall/extract_deps_info_pipeline.py:94: """A pipeline to
> extract DEPS information and rolls.."""
> On 2015/05/29 18:47:55, chanli wrote:
> > nit: 2 '.'s.
> 
> Done.

lgtm.

[qyearsley, 2015-05-29 23:14:33]

LGTM with a couple unimportant nits.

This is a pretty big change, I haven't fully understood it -- 

Is it true that if the blame list for a build contains a DEPS roll, Findit will
get the whole list of all changes in that DEPS roll, and use the files that
changed in all of those changes as signals for deciding whether the DEPS roll is
a suspect?

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:12: """Downloads DEPS from remote GIT
repo."""
GIT -> git or Git

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:21: # migration from SVN to GIT,
.DEPS.git contains dependencies from GIT while
GIT -> git or Git

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:38: """Returns all dependencies of
Chrome as a dict for the given revision and os.
Nit: os -> OS

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:72: new_cr_revision (str): The new
Chromium revision.
Are these git commit hashes or commit positions or either?

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
File appengine/findit/common/dependency.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/dependency.py:53: }
Classes like this could also be made shorter by being written as subclasses of
collections.namedtuple. For example, this class would be:

class DependencyRoll(collections.namedtuple(
    'DependencyRoll', ('path', 'repo_url', 'old_revision', 'new_revision'))):
  """Represents a dependency roll (revision update) in chromium."""
  # Note: It is possible that the DEPS roll is a revert so that |new_revision|
  # is actually older than |old_revision| in the dependency.

  def ToDict(self):
    return self._asdict()

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
File appengine/findit/common/test/chromium_deps_test.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/test/chromium_deps_test.py:148:
self.assertEqual(expected_deps_rolls[index], roll.ToDict())
Could these three lines also be written as the following?

deps_rolls = chromium_deps.GetChromiumDEPSRolls('rev1', 'rev2', 'unix')
self.assertEqual(
    expected_deps_rolls,
    [roll.ToDict() for roll in deps_rolls])

If so, would that be clearer? (I guess it's approximately just as clear.)

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/water...
File appengine/findit/waterfall/test/build_failure_analysis_test.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/water...
appengine/findit/waterfall/test/build_failure_analysis_test.py:199:
failure_info, None, None, None)
Optional: You can add argument names, even if they're not optional args in the
place where the function is defined:

result = build_failure_analysis.AnalyzeBuildFailure(
    failure_info, change_logs=None, deps_info=None, failure_signals=None)

Pros: Clearer to read here.
Cons: This will break if you change the argument names.

Also, if it still makes sense to call AnalyzeBuildFailure with those other
things passed as None, maybe you should make them optional/named arguments.

[stgao, 2015-05-30 00:21:05]

Comments are addressed.

> Is it true that if the blame list for a build contains a DEPS roll, Findit
will
> get the whole list of all changes in that DEPS roll, and use the files that
> changed in all of those changes as signals for deciding whether the DEPS roll
is
> a suspect?
> 

Not for now.

Because a DEPS roll (dep_rev1, dep_rev2] usually includes a big list of changes
for the rolled dependency(eg. blink, v8), pulling all the changes in that
dependency roll might take memory and time (back and forth to the Gitiles) while
most changes are unrelated to the failures.

IMHO, it might be better to use Git blame. Blame the file showing up in the
failure log, and then check if that file was also changed in the time window of
revisions (dep_rev1, dep_rev2].

I'd leave it out of this CL and maybe part of http://crbug.com/469870

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:12: """Downloads DEPS from remote GIT
repo."""
On 2015/05/29 23:14:33, qyearsley wrote:
> GIT -> git or Git

Done.

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:21: # migration from SVN to GIT,
.DEPS.git contains dependencies from GIT while
On 2015/05/29 23:14:32, qyearsley wrote:
> GIT -> git or Git

Done.

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:38: """Returns all dependencies of
Chrome as a dict for the given revision and os.
On 2015/05/29 23:14:33, qyearsley wrote:
> Nit: os -> OS

Done.

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:72: new_cr_revision (str): The new
Chromium revision.
On 2015/05/29 23:14:33, qyearsley wrote:
> Are these git commit hashes or commit positions or either?

Done.
Git commit hashes.

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
File appengine/findit/common/dependency.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/dependency.py:53: }
On 2015/05/29 23:14:33, qyearsley wrote:
> Classes like this could also be made shorter by being written as subclasses of
> collections.namedtuple. For example, this class would be:
> 
> class DependencyRoll(collections.namedtuple(
>     'DependencyRoll', ('path', 'repo_url', 'old_revision', 'new_revision'))):
>   """Represents a dependency roll (revision update) in chromium."""
>   # Note: It is possible that the DEPS roll is a revert so that |new_revision|
>   # is actually older than |old_revision| in the dependency.
> 
>   def ToDict(self):
>     return self._asdict()

That's a good idea!
Done.

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
File appengine/findit/common/test/chromium_deps_test.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/commo...
appengine/findit/common/test/chromium_deps_test.py:148:
self.assertEqual(expected_deps_rolls[index], roll.ToDict())
On 2015/05/29 23:14:33, qyearsley wrote:
> Could these three lines also be written as the following?
> 
> deps_rolls = chromium_deps.GetChromiumDEPSRolls('rev1', 'rev2', 'unix')
> self.assertEqual(
>     expected_deps_rolls,
>     [roll.ToDict() for roll in deps_rolls])
> 
> If so, would that be clearer? (I guess it's approximately just as clear.)

Done.

Was thinking of this approach too.
Although feeling they are almost the same, I opted in for this approach :)

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/water...
File appengine/findit/waterfall/test/build_failure_analysis_test.py (right):

https://codereview.chromium.org/1154593005/diff/100001/appengine/findit/water...
appengine/findit/waterfall/test/build_failure_analysis_test.py:199:
failure_info, None, None, None)
On 2015/05/29 23:14:33, qyearsley wrote:
> Optional: You can add argument names, even if they're not optional args in the
> place where the function is defined:
> 
> result = build_failure_analysis.AnalyzeBuildFailure(
>     failure_info, change_logs=None, deps_info=None, failure_signals=None)
> 
> Pros: Clearer to read here.
> Cons: This will break if you change the argument names.
> 

Done. That's a good idea for readability.

> Also, if it still makes sense to call AnalyzeBuildFailure with those other
> things passed as None, maybe you should make them optional/named arguments.

No. Passing None here is for testing purpose only.

[stgao, 2015-05-30 00:38:58]

The CQ bit was checked by stgao@chromium.org

[stgao, 2015-05-30 00:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org,
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/1154593005/#ps140001
(title: "Improve readability.")

[commit-bot: I haz the power, 2015-05-30 00:39:13]

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

[commit-bot: I haz the power, 2015-05-30 00:42:41]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/infra/infra/+/447171024cfc9434723747d661631...