[Findit] Reorganizing findit_for_*.py

appengine/findit/crash/findit.py

+# Copyright 2016 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 copy
+import logging
+
+from google.appengine.ext import ndb
+
+from common import appengine_util
+from common import chromium_deps
+from common import constants
+from common import time_util
+from crash.crash_pipeline import CrashWrapperPipeline
+from crash.crash_report import CrashReport
+from crash.culprit import NullCulprit
+from model import analysis_status
+from model.crash.crash_config import CrashConfig
+from model.crash.cracas_crash_analysis import CracasCrashAnalysis
+from model.crash.fracas_crash_analysis import FracasCrashAnalysis
+
+
+# TODO: this class depends on ndb stuff, and should therefore move to
+# cr-culprit-finder/service/predator as part of the big reorganization.
+# Also, this class should be renamed to "PredatorApp" (alas, "Azalea"
+# was renamed to "Predator").
+class Findit(object):
+  def __init__(self):
+    # TODO(wrengr): because self.client is volatile, we need some way
+    # of updating the Azelea instance whenever the config changes. How to
+    # do that cleanly?
+    self.azalea = None
+    self.stacktrace_parser = None
+
+  # This is a method rather than an attribute to ensure it can't be
+  # changed. And it's a classmethod rather than a property, since we can
+  # get it directly from the class itself, without needing to allocate
+  # an instance first.
+  @classmethod
+  def ClientID(cls):
+    """Reify the name of this class as a CrashClient id, for serialization."""
+    raise NotImplementedError()
+
+  @property
+  def client_id(self):
+    """Get the client id from the class of this object.
+
+    N.B., this property is static and should not be overridden."""
+    return self.__class__.ClientID()
+
+  # TODO(wrengr): can we remove the dependency on CrashConfig
+  # entirely? It'd be better to receive method calls whenever things
+  # change, so that we know the change happened (and what in particular
+  # changed) so that we can update our internal state as appropriate.
+  @property
+  def config(self):
+    """Get the current value of the client config.
+
+    N.B., this property is volatile and may change asynchronously."""
+    return CrashConfig.Get().GetClientConfig(self.client_id)
+
+  # TODO(wrengr): rename to CanonicalizePlatform or something like that.
+  def RenamePlatform(self, platform):
+    """Remap the platform to a different one, based on the config."""
+    # TODO(katesonia): Remove the default value after adding validity check to
+    # config.
+    return self.config.get('platform_rename', {}).get(platform, platform)
+
+  def CheckPolicy(self, crash_data):
+    """Check whether this client supports analyzing the given report.
+
+    Some clients only support analysis for crashes on certain platforms
+    or channels, etc. This method checks to see whether this client can
+    analyze the given crash.
+
+    Args:
+      crash_data (dict from JSON): ??
+
+    Returns:
+      If satisfied, we return the |crash_data| which may have had some
+      fields modified. Otherwise returns None.
+    """
+    return None
+
+  # TODO(wrengr): rename this to something like _NewAnalysis, since
+  # it only does the "allocation" and needs to/will be followed up with
+  # _InitializeAnalysis anyways.
+  def CreateAnalysis(self, crash_identifiers):
+    return None
+
+  def GetAnalysis(self, crash_identifiers):
+    """Return the CrashAnalysis for the |crash_identifiers|, if one exists.
+
+    Args:
+      crash_identifiers (??): ??
+
+    Returns:
+      If a CrashAnalysis ndb.Model already exists for the
+      |crash_identifiers|, then we return it. Otherwise, returns None.
+    """
+    return None
+
+  # TODO(wrengr): this should be a method on CrashAnalysis, not on Findit.
+  def _InitializeAnalysis(self, model, crash_data):
+    """(Re)Initialize a CrashAnalysis ndb.Model, but do not |put()| it yet.
+
+    This method is only ever called from _NeedsNewAnalysis which is only
+    ever called from ScheduleNewAnalysis. It is used for filling in the
+    fields of a CrashAnalysis ndb.Model for the first time (though it
+    can also be used to re-initialize a given CrashAnalysis). Subclasses
+    should extend (not override) this to (re)initialize any
+    client-specific fields they may have."""
+    # Get rid of any previous values there may have been.
+    model.Reset()
+
+    # Set the version.
+    # |handlers.crash.test.crash_handler_test.testAnalysisScheduled|
+    # provides and expects this field to be called 'chrome_version',
+    # whereas everyone else (e.g., in |crash.test.crash_pipeline_test|
+    # the tests |testAnalysisNeededIfNoAnalysisYet|,
+    # |testRunningAnalysisNoSuspectsFound|, |testRunningAnalysis|,
+    # |testAnalysisNeededIfLastOneFailed|, |testRunningAnalysisWithSuspectsCls|)
+    # expects it to be called 'crashed_version'. The latter is the
+    # better/more general name, so the former needs to be changed in
+    # order to get rid of this defaulting ugliness.
+    model.crashed_version = crash_data.get('crashed_version',
+        crash_data.get('chrome_version', None))
+
+    # Set (other) common properties.
+    model.stack_trace = crash_data['stack_trace']
+    model.signature = crash_data['signature']
+    model.platform = crash_data['platform']
+    # TODO(wrengr): assert that crash_data['client_id'] == self.client_id
+    model.client_id = self.client_id
+
+    # Set progress properties.
+    model.status = analysis_status.PENDING
+    model.requested_time = time_util.GetUTCNow()
+
+  @ndb.transactional
+  def _NeedsNewAnalysis(self, crash_data):
+    raise NotImplementedError()
+
+  def ScheduleNewAnalysis(self, crash_data, queue_name=constants.DEFAULT_QUEUE):
+    """Schedules an analysis."""
+    # Check policy and tune arguments if needed.
+    crash_data = self.CheckPolicy(crash_data)
+    if crash_data is None:
+      return False
+
+    # Detect the regression range, and decide if we actually need to
+    # run a new anlaysis or not.
+    if not self._NeedsNewAnalysis(crash_data):
+      return False
+
+    crash_identifiers = crash_data['crash_identifiers']
+    analysis_pipeline = CrashWrapperPipeline(self, crash_identifiers)

[stgao, 2016/10/19 18:20:53]

The problem is here -- the `self`.
Only JSON-serializable objects could be passed over as parameters to an
AppEngine pipeline. 

What specific data or info from `self` has to be passed over into the pipeline?
Is it possible to pass over the raw data and reconstruct the object in the
pipeline?

[wrengr, 2016/10/22 00:18:49]

IIRC the previous version just passed the client_id here. In principle that's
not enough, since the `self` object also encapsulates various data from
CrashConfig (so when we try to reconstruct `self` from the client_id, various
other config-specified stuff may change, meaning the new Findit object may be
different from the old one; opening the possibility of some hard-to-diagnose
incoherence bugs in the future).

I'll try just passing the client_id and see if that'll get the tests to pass at
least. Thanks for tracking this down.

+    # Attribute defined outside __init__ - pylint: disable=W0201
+    analysis_pipeline.target = appengine_util.GetTargetNameForModule(
+        constants.CRASH_BACKEND[self.client_id])
+    analysis_pipeline.start(queue_name=queue_name)
+    logging.info('New %s analysis is scheduled for %s', self.client_id,
+                 repr(crash_identifiers))
+    return True
+
+  # TODO(wrengr): does the parser actually need the version, signature,
+  # and platform? If not, then we should be able to just pass the string
+  # to be parsed (which would make a lot more sense than passing the
+  # whole model).
+  def ParseStacktrace(self, model):
+    """Parse a CrashAnalysis's |stack_trace| string into a Stacktrace object.
+
+    Args:
+      model (CrashAnalysis): The model containing the stack_trace string
+        to be parsed.
+
+    Returns:
+      On success, returns a Stacktrace object; on failure, returns None.
+    """
+    stacktrace = self.stacktrace_parser.Parse(model.stack_trace,
+        chromium_deps.GetChromeDependency(
+            model.crashed_version, model.platform),
+        model.signature)
+    if not stacktrace:
+      logging.warning('Failed to parse the stacktrace %s', model.stack_trace)
+      return None
+
+    return stacktrace
+
+  # TODO(wrengr): This is only called by |CrashAnalysisPipeline.run|;
+  # we should be able to adjust things so that we only need to take in
+  # |crash_identifiers|, or a CrashReport, rather than taking in the
+  # whole model.
+  # TODO(wrengr): as part of inverting crash_pipeline.py wrt
+  # findit.py, this method should probably be the one to create the
+  # |CrashWrapperPipeline| and |CrashAnalysisPipeline| therein, rather
+  # than the other way around.
+  def FindCulprit(self, model):
+    """Given a CrashAnalysis ndb.Model, return a Culprit."""
+    stacktrace = self.ParseStacktrace(model)
+    if stacktrace is None:
+      # TODO(wrengr): refactor things so we don't need the NullCulprit class.
+      return NullCulprit()
+
+    return self.azalea.FindCulprit(CrashReport(
+        crashed_version = model.crashed_version,
+        signature = model.signature,
+        platform = model.platform,
+        stacktrace = stacktrace,
+        regression_range = model.regression_range))