[Predator] Add TouchCrashedFileMetaFeature.

[Predator] Add TouchCrashedFileMetaFeature.

TouchCrashedFileMetaFeature is a MetaFeature which wraps ``MinDistanceFeature``, ``TopFrameIndexFeature`` and ``TouchCrashedFile``

BUG=679097
Review-Url: https://codereview.chromium.org/2613153006
Committed: https://chromium.googlesource.com/infra/infra/+/894e9bbcdc91f9bb5029dc54f54257c1a72b1bb8

[Sharu Jiang, 2017-01-19 19:12:56]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2017-01-19 19:13:03]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2017-01-19 19:13:09]

Patchset #1 (id:40001) has been deleted

[Sharu Jiang, 2017-01-20 00:26:12]

Patchset #1 (id:60001) has been deleted

[Sharu Jiang, 2017-01-20 00:26:24]

Patchset #1 (id:80001) has been deleted

[Sharu Jiang, 2017-01-20 00:26:31]

Patchset #1 (id:100001) has been deleted

[Sharu Jiang, 2017-01-20 00:26:37]

Patchset #1 (id:120001) has been deleted

[Sharu Jiang, 2017-01-20 00:26:43]

Patchset #1 (id:140001) has been deleted

[Sharu Jiang, 2017-01-20 00:31:47]

Description was changed from

==========
[Predator] Add TouchCrashedFileMetaFeature.

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

to

==========
[Predator] Add TouchCrashedFileMetaFeature.

BUG=679097
==========

[Sharu Jiang, 2017-01-20 00:31:47]

katesonia@chromium.org changed reviewers:
+ chanli@chromium.org, lijeffrey@chromium.org, mbarbella@chromium.org

[Sharu Jiang, 2017-01-20 00:32:04]

katesonia@chromium.org changed reviewers:
+ inferno@chromium.org

[Sharu Jiang, 2017-01-20 00:41:38]

Description was changed from

==========
[Predator] Add TouchCrashedFileMetaFeature.

BUG=679097
==========

to

==========
[Predator] Add TouchCrashedFileMetaFeature.

TouchCrashedFileMetaFeature is a MetaFeature which wraps ``MinDistanceFeature``,
``TopFrameIndexFeature`` and ``TouchCrashedFile``

BUG=679097
==========

[Sharu Jiang, 2017-01-20 00:43:04]

PTAL.

[chanli, 2017-01-23 04:37:46]

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_features/min_distance.py
(right):

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:21: #
TODO(wrengr): we should change things to use integers with None as
Nit: since wrengr has left the team, change the TODO owner to yourself?

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:99:
self._maximum = maximum
Nit: this can be simplified to self._maximum = maximum or DEFAULT_MAXIMUM

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:109: Args:
Nit: add docstring for revision

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:127:
crash_dependency.revision)
Why not use revision directly? And crash_dependency can be removed

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:194: #
crashed file.
I don't quite understand this comment... IIUC,
distance_info_per_file.IsInfinity() when all regions in blame have different
revision from the suspect?

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:219:
``StackInfo``s.
DistanceInfo?

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

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/test/min_distance_test.py:19:
from crash.stacktrace import Stacktrace
Nit: line 17~19 should be up

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
File
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py
(right):

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py:1:
# Copyright 2016 The Chromium Authors. All rights reserved.
2017?

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py:28:
class TouchCrashedFileMetaFeature(MetaFeature):
Class name and file name mismatch?

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py:36:
max_line_distance=None, max_frame_index=None):
Why not change this to 
def __init__(self, get_repository,
max_line_distance=DEFAULT_MAX_LINE_DISTANCE,
max_frame_index=DEFAULT_MAX_FRAME_INDEX)?

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
File appengine/findit/crash/loglinear/feature.py (right):

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/feature.py:118: if self._reason else None)
Since you already have name and value properties, I think it'll be better if you
keep it consistent.

[Sharu Jiang, 2017-01-23 21:44:47]

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_features/min_distance.py
(right):

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:21: #
TODO(wrengr): we should change things to use integers with None as
On 2017/01/23 04:37:45, chanli wrote:
> Nit: since wrengr has left the team, change the TODO owner to yourself?

Done.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:99:
self._maximum = maximum
On 2017/01/23 04:37:45, chanli wrote:
> Nit: this can be simplified to self._maximum = maximum or DEFAULT_MAXIMUM

They are not exactly the same. If maximum is 0, we don't want to use the 
default value.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:109: Args:
On 2017/01/23 04:37:45, chanli wrote:
> Nit: add docstring for revision

Done.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:127:
crash_dependency.revision)
On 2017/01/23 04:37:45, chanli wrote:
> Why not use revision directly? And crash_dependency can be removed

We also need ``crash_dependency.repo_url`` in the line above.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:194: #
crashed file.
On 2017/01/23 04:37:45, chanli wrote:
> I don't quite understand this comment... IIUC,
> distance_info_per_file.IsInfinity() when all regions in blame have different
> revision from the suspect?

That's right, wrong comment... corrected it.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:219:
``StackInfo``s.
On 2017/01/23 04:37:45, chanli wrote:
> DistanceInfo?

Oops. Done.

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

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/test/min_distance_test.py:19:
from crash.stacktrace import Stacktrace
On 2017/01/23 04:37:45, chanli wrote:
> Nit: line 17~19 should be up

Done.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
File
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py
(right):

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py:1:
# Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/23 04:37:45, chanli wrote:
> 2017?

Done.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py:28:
class TouchCrashedFileMetaFeature(MetaFeature):
On 2017/01/23 04:37:45, chanli wrote:
> Class name and file name mismatch?

Yes, we basically omit ``feature`` in file name to avoid too long file name.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/touch_crashed_file_meta.py:36:
max_line_distance=None, max_frame_index=None):
On 2017/01/23 04:37:45, chanli wrote:
> Why not change this to 
> def __init__(self, get_repository,
> max_line_distance=DEFAULT_MAX_LINE_DISTANCE,
> max_frame_index=DEFAULT_MAX_FRAME_INDEX)?

Done.

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
File appengine/findit/crash/loglinear/feature.py (right):

https://codereview.chromium.org/2613153006/diff/160001/appengine/findit/crash...
appengine/findit/crash/loglinear/feature.py:118: if self._reason else None)
On 2017/01/23 04:37:45, chanli wrote:
> Since you already have name and value properties, I think it'll be better if
you
> keep it consistent. 

We want the reason string contains both name and value information as well.

[chanli, 2017-01-23 21:52:33]

lgtm

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_features/top_frame_index.py
(right):

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/top_frame_index.py:31: def
__init__(self, max_frame_index):
So no need for default any more?

[Sharu Jiang, 2017-01-23 23:02:17]

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_features/top_frame_index.py
(right):

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/top_frame_index.py:31: def
__init__(self, max_frame_index):
On 2017/01/23 21:52:33, chanli wrote:
> So no need for default any more?

Yes, the default value will be set in meta feature that contains this feature.

[Martin Barbella, 2017-01-24 18:52:53]

I'm still thinking about some of the naming, but one comment in the meantime.
Will likely post more soon.

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
File appengine/findit/crash/crash_util.py (right):

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:26: if not path_1 and not path_2:
We're only worried about None here, right? I don't necessarily think it's worth
it to handle this here at all, but you could simplify this to something like:

if not path_1 or not path_2:
  return path_1 == path_2

The current approach is a bit more robust if we care about returning true for ''
and None, I suppose, but in that case I'd assume something has gone horribly
wrong.

All of that said, This approach feels like it has the potential to hide subtle
bugs. We don't usually want these paths to be None, right? Unless it complicates
things further, I'd prefer cleaning up the call-sites and adding:

assert path_1 is not None
assert path_2 is not None

[Sharu Jiang, 2017-01-24 19:43:11]

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
File appengine/findit/crash/crash_util.py (right):

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:26: if not path_1 and not path_2:
On 2017/01/24 18:52:53, Martin Barbella wrote:
> We're only worried about None here, right? I don't necessarily think it's
worth
> it to handle this here at all, but you could simplify this to something like:
> 
> if not path_1 or not path_2:
>   return path_1 == path_2
> 
> The current approach is a bit more robust if we care about returning true for
''
> and None, I suppose, but in that case I'd assume something has gone horribly
> wrong.
> 
> All of that said, This approach feels like it has the potential to hide subtle
> bugs. We don't usually want these paths to be None, right? Unless it
complicates
> things further, I'd prefer cleaning up the call-sites and adding:
> 
> assert path_1 is not None
> assert path_2 is not None

Actually it's possible that a path is None, we are using
``touched_file_info.new_path`` to compare, so if the touched file is deleted,
the new_path is None, the ``IsSameFilePath`` should return False.

[Martin Barbella, 2017-01-24 22:06:40]

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_features/min_distance.py
(right):

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:17: #
TODO(katesonia): we should change things to use integers with None as
Could you move this to IsInfinity?

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:20: class
DistanceInfo(object):
Agreed that DistanceInfo isn't the best name for this. It sounds a bit too
generic for what this actually is. Either way, let's try to address this now. We
have too many TODOs as it is.

Maybe something liked ModifiedFrameDistanceInfo? That's a bit verbose, so I
could also understand wanting to take Distance out.

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:97: def
DistanceInfoBetweenTouchedFileAndStacktrace(
"Info" seems unnecessary to me, especially since it's documented that this
returns a DistanceInfo. Up to you, though, if you think it's clearer as-is.

[Sharu Jiang, 2017-01-25 19:13:31]

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_features/min_distance.py
(right):

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:17: #
TODO(katesonia): we should change things to use integers with None as
On 2017/01/24 22:06:40, Martin Barbella wrote:
> Could you move this to IsInfinity?

Done.

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:20: class
DistanceInfo(object):
On 2017/01/24 22:06:40, Martin Barbella wrote:
> Agreed that DistanceInfo isn't the best name for this. It sounds a bit too
> generic for what this actually is. Either way, let's try to address this now.
We
> have too many TODOs as it is.
> 
> Maybe something liked ModifiedFrameDistanceInfo? That's a bit verbose, so I
> could also understand wanting to take Distance out.

Changed to ``ModifiedFrameInfo``.

https://codereview.chromium.org/2613153006/diff/180001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_features/min_distance.py:97: def
DistanceInfoBetweenTouchedFileAndStacktrace(
On 2017/01/24 22:06:40, Martin Barbella wrote:
> "Info" seems unnecessary to me, especially since it's documented that this
> returns a DistanceInfo. Up to you, though, if you think it's clearer as-is.

Deleted it.

[Martin Barbella, 2017-01-26 00:52:47]

lgtm

[Sharu Jiang, 2017-01-26 01:46:46]

Patchset #4 (id:220001) has been deleted

[Sharu Jiang, 2017-01-26 01:47:02]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2017-01-26 01:47:03]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org,
mbarbella@chromium.org
Link to the patchset: https://codereview.chromium.org/2613153006/#ps240001
(title: "Add comments.")

[commit-bot: I haz the power, 2017-01-26 01:47:08]

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-01-26 01:57:29]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1485395222757270,
"parent_rev": "d9c001e373886d3c0246795a508826e461a2171b", "commit_rev":
"894e9bbcdc91f9bb5029dc54f54257c1a72b1bb8"}

[commit-bot: I haz the power, 2017-01-26 01:57:32]

Description was changed from

==========
[Predator] Add TouchCrashedFileMetaFeature.

TouchCrashedFileMetaFeature is a MetaFeature which wraps ``MinDistanceFeature``,
``TopFrameIndexFeature`` and ``TouchCrashedFile``

BUG=679097
==========

to

==========
[Predator] Add TouchCrashedFileMetaFeature.

TouchCrashedFileMetaFeature is a MetaFeature which wraps ``MinDistanceFeature``,
``TopFrameIndexFeature`` and ``TouchCrashedFile``

BUG=679097

Review-Url: https://codereview.chromium.org/2613153006
Committed:
https://chromium.googlesource.com/infra/infra/+/894e9bbcdc91f9bb5029dc54f5425...
==========

[commit-bot: I haz the power, 2017-01-26 01:57:33]

Committed patchset #4 (id:240001) as
https://chromium.googlesource.com/infra/infra/+/894e9bbcdc91f9bb5029dc54f5425...