[Findit] Reorganizing findit_for_*.py

appengine/findit/crash/findit.py

Index: appengine/findit/crash/findit.py
diff --git a/appengine/findit/crash/findit.py b/appengine/findit/crash/findit.py
new file mode 100644
index 0000000000000000000000000000000000000000..39c283da1ae3aa74b0c298142684f53b43450fb6
--- /dev/null
+++ b/appengine/findit/crash/findit.py
@@ -0,0 +1,156 @@
+# 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 common import time_util
+from crash.chromecrash_parser import ChromeCrashParser
+from crash.component_classifier import Component
+from crash.component_classifier import ComponentClassifier
+from crash.crash_report import CrashReport
+from crash.culprit import NullCulprit
+from crash.project_classifier import ProjectClassifier
+from crash.type_enums import CrashClient
+from model import analysis_status
+from model.crash.cracas_crash_analysis import CracasCrashAnalysis
+from model.crash.crash_config import CrashConfig
+from model.crash.fracas_crash_analysis import FracasCrashAnalysis
+
+# TODO: this class depends on ndb stuff, and should therefore move to
+# ./findit/appengine as part of the big reorganization
+class Findit(object):
+  def __init__(self):
+    # TODO(wrengr): are the clients similar enough that we can initialize
+    # Azalea here?
+    # 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
+
+  # This is a method rather than an attribute to ensure it can't be changed.
+  @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 cna 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)
+
+  # TODO(wrengr): the crash_identifiers are only used for logging. Do
+  # we really need them here?
+  # TODO(wrengr): what does "policy" actually mean here? All we do is
+  # convert one report into another; should have a better name.
+  # TODO(wrengr): FinditForChromeCrash needs the channel for logging and
+  # bailing out for unsupported channels. Can we avoid needing it there,
+  # or can we instead pass it in **kwargs?
+  def CheckPolicy(self, crash_identifiers, report, channel):
+    """Check whether the report satisfies this client's policy.
+
+    Args:
+      crash_identifiers (??): ??
+      report (CrashReport): The report we're checking.
+      channel (str): ??
+
+    Returns:
+      If satisfied, we return a new report which may update some
+      fields. Otherwise returns None.
+    """
+    return None

[Sharu Jiang, 2016/10/12 17:46:29]

shouldn't this raise NotImplementedError()?

[wrengr, 2016/10/12 21:14:51]

That's what I originally had, but a lot of the unittests rely on it returning
None.

As written it fits the docstring (in this case: using the policy to reject all
reports), so I'm not too concerned.  I'm also fine with raising
NotImplementedError, but if we do that then I'll need someone to go through and
update the unittests to handle that appropriately.

[Sharu Jiang, 2016/10/12 22:35:03]

For those Not Implemented methods, we can simply skip testing, add "# pragma: no
cover." to skip code coverage and only test those implemented methods.

+
+  def CreateAnalysis(self, crash_identifiers):
+    return None

[Sharu Jiang, 2016/10/12 17:46:29]

Ditto.

[wrengr, 2016/10/12 21:14:51]

Ditto.

+
+  def GetAnalysis(self, crash_identifiers):
+    return None

[Sharu Jiang, 2016/10/12 17:46:29]

Ditto.

[wrengr, 2016/10/12 21:14:51]

ditto.

+
+  # TODO(wrengr): FinditForChromeCrash used to also set |channel| and
+  # |historical_metadata|. However, the values for those aren't clear from
+  # the one callsite for |UpdateAnalysis| (i.e., in crash_pipeline.py)
+  def ResetAnalysis(self, model, report):

[Sharu Jiang, 2016/10/12 22:35:03]

The create order should be:

client-input-messeage --> ndb.CrashAnalysis --> CrashReport

So the parameters here should be (model, signature, platform, chrome_versions,
...), CrashReport should be the parsed object which is ready for analysis.

The name may be a little confusing, however changing name of ndb.CrashAnalysis
would cause some legacy data issue, so I prefer keep it as it is.

[wrengr, 2016/10/18 23:13:54]

It's not clear to me when/why one would call ResetAnalysis, regardless of how
the arguments are phrased. The previous version was undocumented so I have no
idea what it's supposed to mean/do. The collection of arguments it takes are the
same as other places taking CrashReports, so that doesn't really help. And since
CrashAnalysis comprises information from both the CrashReport and the Culprit,
when I hear "reset" I think of discarding whatever's there and replacing it with
just the CrashReport to be analysed; which fits with the fact that it sets the
status to PENDING.

Provided the current code works, I'd rather land the CL and try to perfect
things later. All of this stuff will be moving to ./service/azalea-app/ anyways,
since it's appengine code not library code. And that reorg will entail changes
to the rest of the appengine code anyways.

+    """Copies the CrashReport into the ndb.Model, but does not |put()| it.
+
+    This method is distinguished from UpdateAnalysis in that we modify
+    the |model| but do not |put()| it. This is the method that subclasses
+    should extend (not override) in order to perform any additional work
+    needed to reset their ndb.Models."""
+    model.Reset()
+
+    # Set common properties.
+    model.crashed_version = report.crashed_version
+    model.stack_trace = str(report.stacktrace) # TODO(wrengr): fix this
+    model.signature = report.signature
+    model.platform = report.platform
+    model.client_id = self.client_id
+
+    # Set progress properties.
+    model.status = analysis_status.PENDING
+    model.requested_time = time_util.GetUTCNow()
+
+  def UpdateAnalysis(self, model, report):
+    """Update the CrashAnalysis ndb.Model and then put it.
+
+    This method simply calls ResetAnalysis and then calls |put()| on the
+    modified model. We distinguish it from that method because we need
+    subclasses to do extra work to reset their own parts of the ndb.Model,
+    but we need the call to |put()| to happen after those changes have
+    been executed. For now, this seems like the cleanest split."""
+    self.ResetAnalysis(model, report)
+    model.put()
+
+  def FindCulprit(self, mode, **kwargs):
+    """Given a CrashAnalysis ndb.Model, return a Culprit."""
+    return (
+        {'found': False},
+        {'found_suspects': False, 'has_regression_range': False})
+
+
+# TODO(katesonia): Implement this class.
+class FinditForClusterfuzz(Findit):

[Sharu Jiang, 2016/10/12 17:46:29]

Why is this in findit.py?

[wrengr, 2016/10/12 21:14:51]

Because it's as-yet too short to bother breaking out into its own file? I could
break it out if you like, I just figured you'd do that as part of the CL
implementing it.

[Sharu Jiang, 2016/10/12 22:35:03]

Can you break it out to findit_for_clustefuzz.py? since you already wrote the
skeleton code. Thanks :) 

No findit_for_clustefuzz_test.py is needed, so the test coverage won't skan this
file.

[wrengr, 2016/10/18 23:13:54]

Done.

+  @classmethod
+  def ClientID(cls):
+    return CrashClient.CLUSTERFUZZ
+
+  def __init__(self):
+    logging.info('Client %s is not supported by findit right now',
+        self.client_id)
+    raise NotImplementedError()
+
+  def CreateAnalysis(self, crash_identifiers):
+    raise NotImplementedError()
+
+  def GetAnalysis(self, crash_identifiers):
+    raise NotImplementedError()
+
+  def ResetAnalysis(self, model, report, *args, **kwargs):
+    super(FinditForClusterfuzz, self).ResetAnalysis(model, report)
+    raise NotImplementedError()
+
+  def CheckPolicy(self, crash_identifiers, report, channel):
+    return report
+
+  def FindCulprit(self, model, **kwargs):
+    raise NotImplementedError()