[Findit] Pipeline change to save suspected cls to data store.

appengine/findit/waterfall/build_failure_analysis.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import collections
+from collections import defaultdict
 import os
 import re
 
 from common.diff import ChangeType
 from common.git_repository import GitRepository
 from common.http_client_appengine import HttpClientAppengine as HttpClient
+from model import analysis_approach_type
+from waterfall import suspected_cl_util
 from waterfall import waterfall_config
 from waterfall.failure_signal import FailureSignal
 
 
 def _IsSameFile(changed_src_file_path, file_path_in_log):
   """Guesses if the two files are the same.
 
   Args:
     changed_src_file_path (str): Full path of a file committed to git repo.
     file_path_in_log (str): Path of a file appearing in a failure log. It might
@@ skipping 204 matching lines @@
         appearing in the compile failure.
      2) If a hint shows that a CL is likely-suspected, the hint is given 1
         score point. Eg. a CL is just likely suspected if it only changed a
         related file (x_impl.cc vs. x.h) appearing in a failure.
   2. hints: each hint is a string describing a reason for suspecting a CL and
      could be shown to the user (eg., "added x_impl.cc (and it was in log)").
   """
 
   def __init__(self):
     self._score = 0
-    self._hints = collections.defaultdict(int)
+    self._hints = defaultdict(int)
 
   @property
   def score(self):
     return self._score
 
   def AddFileChange(self, change_action, changed_src_file_path,
                     file_path_in_log, score, num_file_name_occurrences,
                     changed_line_numbers=None):
     """Adds a suspected file change.
 
@@ skipping 311 matching lines @@
     failure_signal (FailureSignal): The failure signal of a failed step or test.
     change_log (dict): The change log of a CL as returned by
         common.change_log.ChangeLog.ToDict().
     deps_info (dict): Output of pipeline ExtractDEPSInfoPipeline.
 
   Returns:
     A dict as returned by _Justification.ToDict() if the CL is suspected for the
     failure; otherwise None.
   """
   # Use a dict to map each file name of the touched files to their occurrences.
-  file_name_occurrences = collections.defaultdict(int)
+  file_name_occurrences = defaultdict(int)
   for touched_file in change_log['touched_files']:
     change_type = touched_file['change_type']
     if (change_type in (ChangeType.ADD, ChangeType.COPY,
                         ChangeType.RENAME, ChangeType.MODIFY)):
       file_name = os.path.basename(touched_file['new_path'])
       file_name_occurrences[file_name] += 1
 
     if change_type in (ChangeType.DELETE, ChangeType.RENAME):
       file_name = os.path.basename(touched_file['old_path'])
       file_name_occurrences[file_name] += 1
@@ skipping 14 matching lines @@
 
     _CheckFileInDependencyRolls(file_path_in_log, rolls, justification,
                                 line_numbers)
 
   if not justification.score:
     return None
   else:
     return justification.ToDict()
 
 
+class _CLInfo(object):

[lijeffrey, 2016/09/12 22:55:11]

So how about making this depend on your other cl
https://codereview.chromium.org/2320153003/ and make this a BaseCL object and
get rid of _CLInfo?

[chanli, 2016/09/16 21:24:19]

Based on our discussion offline, I'll leave it as is for now.

+  """A object of information we need for a suspected CL.
+
+  The information is specific to current build.
+  """
+  def __init__(self):
+    self.failures = defaultdict(list)
+    self.top_score = 0
+
+  @property
+  def not_none_failures(self):

[stgao, 2016/09/15 17:35:12]

What does this mean?

[chanli, 2016/09/16 21:24:19]

For compile and other non-swarming steps, we will have None in the list, I just
remove those Nones from the list.

[stgao, 2016/09/23 18:30:21]

Why we will have None in the list? Can we avoid that at the first place?

If this turns out to be a necessary function, should we add comments as the
functionality is not obvious?

[chanli, 2016/09/24 00:04:59]

Since I'm using defaultdict here, I need to make sure to include steps like
compile(just step_name but no test_name) to the dict, and I use none as the
test_name to ensure that.

+    failures = {
+        key: [t for t in self.failures[key] if t] for key in self.failures
+    }
+    return failures
+
+
+def _SaveFailureToMap(
+    cl_failure_map, new_suspected_cl_dict, step_name, test_name, top_score):
+  """Saves a failure's info to the cl that caused it."""
+  cl_key = (
+      new_suspected_cl_dict['repo_name'], new_suspected_cl_dict['revision'],
+      new_suspected_cl_dict['commit_position'])
+
+  cl_failure_map[cl_key].failures[step_name].append(test_name)
+  cl_failure_map[cl_key].top_score = top_score

[stgao, 2016/09/15 17:35:12]

What if the original score is higher than this one?

[chanli, 2016/09/16 21:24:19]

The real problem should be what if the original score is different from this
one... Since this should be a rare case and I don't want to handle it here, I
just override the value.

I can do 
cl_failure_map[cl_key].top_score = cl_failure_map[cl_key].top_score or top_score

if you think it should be more reasonable.

[stgao, 2016/09/23 18:30:21]

Add a comment for that?

[chanli, 2016/09/24 00:04:59]

Done.

+
+
+def _SaveSuspectedCLs(
+    cl_failure_map, master_name, builder_name, build_number, failure_type):
+  """Saves suspected CLs to dataStore."""
+  for cl_key, cl_object in cl_failure_map.iteritems():
+    repo_name, revision, commit_position = cl_key
+    suspected_cl_util.UpdateSuspectedCL(
+        repo_name, revision, commit_position, analysis_approach_type.HEURISTIC,
+        master_name, builder_name, build_number, failure_type,
+        cl_object.not_none_failures, cl_object.top_score)
+
+
 def AnalyzeBuildFailure(
     failure_info, change_logs, deps_info, failure_signals):
   """Analyze the given failure signals, and figure out culprit CLs.
 
   Args:
     failure_info (dict): Output of pipeline DetectFirstFailurePipeline.
     change_logs (dict): Output of pipeline PullChangelogPipeline.
     deps_info (dict): Output of pipeline ExtractDEPSInfoPipeline.
     failure_signals (dict): Output of pipeline ExtractSignalPipeline.
 
@@ skipping 45 matching lines @@
         'commit_position': change_log.get('commit_position'),
         'url':
             change_log.get('code_review_url') or change_log.get('commit_url'),
     }
 
     cl_info.update(justification_dict)
     return cl_info
 
   failed_steps = failure_info['failed_steps']
   builds = failure_info['builds']
-  master_name = failure_info.get('master_name')
+  master_name = failure_info['master_name']
+
+  cl_failure_map = defaultdict(_CLInfo)
 
   for step_name, step_failure_info in failed_steps.iteritems():
     is_test_level = step_failure_info.get('tests') is not None
 
     failed_build_number = step_failure_info['current_failure']
     if step_failure_info.get('last_pass') is not None:
       start_build_number = step_failure_info.get('last_pass') + 1
     else:
       start_build_number = step_failure_info['first_failure']
     step_analysis_result = {
@@ skipping 30 matching lines @@
                   test_signal, change_logs[revision], deps_info)
 
               if not justification_dict:
                 continue
 
               new_suspected_cl_dict = CreateCLInfoDict(
                   justification_dict, build_number, change_logs[revision])
               test_analysis_result['suspected_cls'].append(
                   new_suspected_cl_dict)
 
+              _SaveFailureToMap(

[stgao, 2016/09/15 17:35:12]

I'm wondering if it would be possible and cleaner if we do a post-analysis
process for saving the data into datastore?

What do you think?

[chanli, 2016/09/16 21:24:19]

I actually do datastore operations in a patch here. But I moved the part doing
datastore operations to identify_culprit_pipeline while leave the process part
here.

+                  cl_failure_map, new_suspected_cl_dict, step_name, test_name,
+                  max(justification_dict.values()))
+
           # Checks Files on step level using step level signals
           # regardless of test level signals so we can make sure
           # no duplicate justifications added to the step result.
           failure_signal = FailureSignal.FromDict(failure_signals[step_name])
           justification_dict = _CheckFiles(
               failure_signal, change_logs[revision], deps_info)
 
           if not justification_dict:
             continue
 
-          step_analysis_result['suspected_cls'].append(
-              CreateCLInfoDict(justification_dict, build_number,
-                               change_logs[revision]))
+          new_suspected_cl_dict = CreateCLInfoDict(
+              justification_dict, build_number, change_logs[revision])
+          step_analysis_result['suspected_cls'].append(new_suspected_cl_dict)
+
+          if not is_test_level:
+            _SaveFailureToMap(
+                cl_failure_map, new_suspected_cl_dict, step_name, None,
+                max(justification_dict.values()))
 
     # TODO(stgao): sort CLs by score.
     analysis_result['failures'].append(step_analysis_result)
 
+  # Save suspected_cls to data_store.
+  _SaveSuspectedCLs(
+      cl_failure_map, failure_info['master_name'], failure_info['builder_name'],
+      failure_info['build_number'], failure_info['failure_type'])
+
   return analysis_result