[Predator] Switch from anonymous dict to CrashData.

[Predator] Switch from anonymous dict to CrashData.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and ``ChromeCrashAnalysis``.

BUG=688578
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2663063007
Committed: https://chromium.googlesource.com/infra/infra/+/07f71dabccc03510b789ddf5e2df7c2074a3420e

[Sharu Jiang, 2017-02-02 22:45:05]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2017-02-02 22:45:12]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2017-02-02 22:45:19]

Patchset #1 (id:40001) has been deleted

[Sharu Jiang, 2017-02-02 22:45:28]

Patchset #1 (id:60001) has been deleted

[Sharu Jiang, 2017-02-02 23:27:56]

Description was changed from

==========
Merge branch 'switch-to-llm' into refactor-crash-report


.


Merge branch 'switch-to-llm' into refactor-crash-report


.

BUG=
==========

to

==========
[Predator] Switch from anonymous dict to CrashBuffer.

BUG=
==========

[Sharu Jiang, 2017-02-02 23:27:57]

katesonia@chromium.org changed reviewers:
+ mbarbella@chromium.org, wrengr@chromium.org

[Sharu Jiang, 2017-02-02 23:28:07]

Patchset #1 (id:80001) has been deleted

[Sharu Jiang, 2017-02-02 23:33:48]

PTAL.

[wrengr, 2017-02-03 18:34:48]

I only took a cursory look. It'd be easier to review if you split apart into two
CLs: one for the CrashReport stuff, and another for passing the config as an
argument rather than querying it.

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit.py:56:
config.project_classifier['non_chromium_project_rank_priority'])
newline between 56 and 57?

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit.py:95: raise NotImplementedError()
What's this for?

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit.py:127: return
self.client_config['platform_rename'].get(platform, platform)
would be nice if ``platform_rename`` were a property of the config rather than a
key

[Sharu Jiang, 2017-02-03 23:13:38]

Split the passing config as argument part in another cl -
https://codereview.chromium.org/2673993003/

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit.py:56:
config.project_classifier['non_chromium_project_rank_priority'])
On 2017/02/03 18:34:47, wrengr wrote:
> newline between 56 and 57?

Done.

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit.py:95: raise NotImplementedError()
On 2017/02/03 18:34:46, wrengr wrote:
> What's this for?

Since the ``FindCulprit`` will use self._predator, so I want to force people to
implement it in subclasses and not call it on this abstract class.

https://codereview.chromium.org/2663063007/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit.py:127: return
self.client_config['platform_rename'].get(platform, platform)
On 2017/02/03 18:34:46, wrengr wrote:
> would be nice if ``platform_rename`` were a property of the config rather than
a
> key

Now the client config are all dicts, later we may refactor it.

[Sharu Jiang, 2017-02-06 21:50:23]

Patchset #5 (id:180001) has been deleted

[Sharu Jiang, 2017-02-06 22:06:54]

Patchset #6 (id:220001) has been deleted

[Sharu Jiang, 2017-02-10 23:09:10]

Patchset #6 (id:240001) has been deleted

[Sharu Jiang, 2017-02-10 23:09:20]

Patchset #6 (id:260001) has been deleted

[Sharu Jiang, 2017-02-10 23:27:45]

Patchset #6 (id:280001) has been deleted

[Sharu Jiang, 2017-02-10 23:27:52]

Patchset #5 (id:200001) has been deleted

[Sharu Jiang, 2017-02-10 23:36:25]

Patchset #5 (id:300001) has been deleted

[Sharu Jiang, 2017-02-10 23:36:31]

Patchset #4 (id:160001) has been deleted

[Sharu Jiang, 2017-02-10 23:42:38]

Description was changed from

==========
[Predator] Switch from anonymous dict to CrashBuffer.

BUG=
==========

to

==========
[Predator] Switch from anonymous dict to CrashBuffer.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
==========

[Sharu Jiang, 2017-02-10 23:43:28]

Ping :)

[wrengr (wrong one), 2017-02-11 00:11:33]

wrengr@google.com changed reviewers:
+ wrengr@google.com

[wrengr (wrong one), 2017-02-11 00:11:34]

lgtm with nits

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:203: # directly, but rather must access it
indirectly through the module.
This comment is no longer relevant, since we don't call DetectRegressionRange
here anymore

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:207: model.regression_range)):
this seems a bit strange/convoluted. Why not do something like
``tuple(model.regression_range or [])``?

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
File
appengine/findit/crash/loglinear/changelist_features/test/min_distance_test.py
(right):

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/test/min_distance_test.py:11:
from crash.crash_report import CrashReport
duplicate import

[Sharu Jiang, 2017-02-11 00:41:20]

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:203: # directly, but rather must access it
indirectly through the module.
On 2017/02/11 00:11:34, wrengr (wrong one) wrote:
> This comment is no longer relevant, since we don't call DetectRegressionRange
> here anymore

Done.

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:207: model.regression_range)):
On 2017/02/11 00:11:34, wrengr (wrong one) wrote:
> this seems a bit strange/convoluted. Why not do something like
> ``tuple(model.regression_range or [])``?

If a crash doesn't have regression range, it should be None instead of empty
tuple.

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
File
appengine/findit/crash/loglinear/changelist_features/test/min_distance_test.py
(right):

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/test/min_distance_test.py:11:
from crash.crash_report import CrashReport
On 2017/02/11 00:11:34, wrengr (wrong one) wrote:
> duplicate import

Done.

[wrengr (wrong one), 2017-02-11 01:14:54]

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:207: model.regression_range)):
On 2017/02/11 00:41:20, Sharu Jiang wrote:
> On 2017/02/11 00:11:34, wrengr (wrong one) wrote:
> > this seems a bit strange/convoluted. Why not do something like
> > ``tuple(model.regression_range or [])``?
> 
> If a crash doesn't have regression range, it should be None instead of empty
> tuple.

ah.

Maybe make the else-branch explicitly return None rather than returning
model.regression_range? Or maybe make the regression_range property so that it's
already a tuple-or-None so we don't need to do anything here?

[Sharu Jiang, 2017-02-13 18:40:05]

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2663063007/diff/320001/appengine/findit/crash...
appengine/findit/crash/findit.py:207: model.regression_range)):
On 2017/02/11 01:14:54, wrengr (wrong one) wrote:
> On 2017/02/11 00:41:20, Sharu Jiang wrote:
> > On 2017/02/11 00:11:34, wrengr (wrong one) wrote:
> > > this seems a bit strange/convoluted. Why not do something like
> > > ``tuple(model.regression_range or [])``?
> > 
> > If a crash doesn't have regression range, it should be None instead of empty
> > tuple.
> 
> ah.
> 
> Maybe make the else-branch explicitly return None rather than returning
> model.regression_range? Or maybe make the regression_range property so that
it's
> already a tuple-or-None so we don't need to do anything here?

Done.

[Sharu Jiang, 2017-02-13 18:43:02]

Description was changed from

==========
[Predator] Switch from anonymous dict to CrashBuffer.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
==========

to

==========
[Predator] Switch from anonymous dict to CrashBuffer.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
TBR=stgao@chromium.org
==========

[Sharu Jiang, 2017-02-13 18:43:10]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2017-02-13 18:43:10]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@google.com
Link to the patchset: https://codereview.chromium.org/2663063007/#ps360001
(title: "Fix nits.")

[commit-bot: I haz the power, 2017-02-13 18:43:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 18:50:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 18:50:50]

Try jobs failed on following builders:
  Infra Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/34519af9df1cba10)

[Sharu Jiang, 2017-02-13 22:54:28]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2017-02-13 22:54:28]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@google.com
Link to the patchset: https://codereview.chromium.org/2663063007/#ps380001
(title: "Rebase and fix delta test.")

[commit-bot: I haz the power, 2017-02-13 22:54:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 23:04:24]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1487026468301200,
"parent_rev": "92914c46205dae342f046f608a00d3b35fc648bc", "commit_rev":
"07f71dabccc03510b789ddf5e2df7c2074a3420e"}

[commit-bot: I haz the power, 2017-02-13 23:04:27]

Description was changed from

==========
[Predator] Switch from anonymous dict to CrashBuffer.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
TBR=stgao@chromium.org
==========

to

==========
[Predator] Switch from anonymous dict to CrashBuffer.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2663063007
Committed:
https://chromium.googlesource.com/infra/infra/+/07f71dabccc03510b789ddf5e2df7...
==========

[commit-bot: I haz the power, 2017-02-13 23:04:29]

Committed patchset #7 (id:380001) as
https://chromium.googlesource.com/infra/infra/+/07f71dabccc03510b789ddf5e2df7...

[Sharu Jiang, 2017-02-13 23:10:23]

Description was changed from

==========
[Predator] Switch from anonymous dict to CrashBuffer.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2663063007
Committed:
https://chromium.googlesource.com/infra/infra/+/07f71dabccc03510b789ddf5e2df7...
==========

to

==========
[Predator] Switch from anonymous dict to CrashData.

Add ``Initialize`` and ``ToCrashReport`` to ``CrashAnalysis`` and
``ChromeCrashAnalysis``.

BUG=688578
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2663063007
Committed:
https://chromium.googlesource.com/infra/infra/+/07f71dabccc03510b789ddf5e2df7...
==========