[Findit] Initial code of findit for crash. Add scorers to apply heuristic rules.

[Findit] Initial code of findit for crash. Add scorers to apply heuristic rules.

Add FindItForCrash core module to findit for crash. The algorithm finds match of file paths in changlelogs in regression range and file paths in stacktrace. 

Added scorers to apply heuristic rules - top_frame_index, min_distance.

Committed: https://chromium.googlesource.com/infra/infra/+/194e660ea8aaad7f44ae5ad4f4eb61c2cecf6de9

[Sharu, 2016-04-06 17:33:14]

katesonia@google.com changed reviewers:
+ stgao@chromium.org

[Sharu, 2016-04-06 17:33:15]

Initial code :)

[Sharu, 2016-04-06 17:33:16]

Initial code :)

[Sharu, 2016-04-07 17:05:16]

Patchset #2 (id:20001) has been deleted

[Sharu, 2016-04-07 17:08:44]

Patchset #2 (id:40001) has been deleted

[Sharu, 2016-04-13 22:44:05]

Patchset #4 (id:100001) has been deleted

[Sharu, 2016-04-14 00:42:09]

Patchset #1 (id:1) has been deleted

[Sharu, 2016-04-14 00:42:20]

Patchset #1 (id:60001) has been deleted

[Sharu, 2016-04-14 00:42:27]

Patchset #2 (id:120001) has been deleted

[Sharu, 2016-04-14 00:45:49]

Description was changed from

==========
initial code of findit for crash.
==========

to

==========
Add FindItForCrash core module to findit for crash. The algorithm finds match of
file paths in changlelogs in regression range and file paths in stacktrace. 

Added scorers to apply heuristic rules - top_frame_index, min_distance.
==========

[Sharu, 2016-04-14 00:47:08]

katesonia@google.com changed reviewers:
+ lijeffrey@chromium.org

[Sharu, 2016-04-14 00:47:08]

PTAL :)

[Sharu, 2016-04-14 22:00:19]

katesonia@google.com changed reviewers:
+ mbarbella@google.com

[Sharu, 2016-04-14 22:00:19]

PTAL :)

[Martin Barbella, 2016-04-15 06:05:23]

mbarbella@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Martin Barbella, 2016-04-15 06:05:24]

Haven't had a chance to look at this too closely, but some initial nits and
questions.

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

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:7: """Determines if two paths represent
same path.
What's the use case for this? Just trying to understand the rationale for the 3
directory intersection.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:19: path_parts1 = path1.split('/')
Nit: _1. Ditto for the other.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:84: 
Nit: Remove this whitespace and the one on 88.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:97: # If a dependency is deleted or
is a .h file, skip this changelog.
s/skip this changelog/skip this file/ right?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/min_distance.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/min_distance.py:24: if not hasattr(result,
'min_distance'):
What would cause this to happen, other than an incorrect call? It may not be
worth handling.

[stgao, 2016-04-15 18:35:19]

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:120: """Get dep_path to DependencyRoll
dictionary for deps in
nit: One line summary here.
nit: "Get" -> "Gets".

Let's try to be consistent with other code in the repo.

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

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
How about adding the reason behind this comparison?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:26: return len(intersection) >= (min(3,
min(len(path_parts1),
min(3, 2, 1) == 1

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:20: if not crash_stack:
It seems this is just one callstack, while a crash stack might include multiple
callstacks. We'd better use the terms consistently to avoid confusion. Maybe I'm
wrong with the terms too?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:28: for dep in deps:
Can we merge these two for loop?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:98: if not touched_file.new_path or
touched_file.new_path.endswith('.h'):
Can we use the change_type instead?

https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:101: if changelog.reverted_revision:
Should this be in this inner for loop?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:104:
ignore_cls.add(changelog.reverted_revision)
Should we including the reverting cl too?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:146: def
GetDepFileToBlame(stacktrace, stack_deps):
What does "DepFile" mean here? The DEPS file?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:172: git_parser =
GitRepository(stack_deps[frame.dep_path].repo_url,
naming nit: it's not just a parser.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:211: if not
crash_util.IsSameFilePath(crashed_file_path, touched_file_path):
Note: syzyasan has lower case file path in ClusterFuzz.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:238: start_revision, end_revision =
regression_range
good_revision, bad_revision?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:243: start_revision, end_revision,
'all')
Why 'all' instead of the specific os platform?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/results.py:21: changelog_json = self.changelog.ToDict()
This seems unnecessary as we only refer to several fields. Why not refer to them
directly?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/results.py:66: stacktrace and touched by the changelog of
this result.
changelog here might not be that clear enough. It seems we meant to refer to a
commit or revision.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/aggregator.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/aggregator.py:18: curr_score, curr_reason =
scorer(result)
no abbreviation.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/aggregator.py:19: score *= curr_score
So we change from a vector of scores to multiplying all scores?
Do you plan to run some experiment to compare both approach?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/min_distance.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/min_distance.py:37: return 0.8
Where are these numbers from? Some comments in the code would be great.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/test/top_frame_index_test.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/test/top_frame_index_test.py:12: DUMMY_CHANGELOG
= ChangeLog.FromDict({
This is repeated for quite a few times, could it be shared?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/top_frame_index.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/top_frame_index.py:34: return 1 - top_frame_index
/ float(self.max_top_n)
I think this is reasonable, but without experiment and comparison I'm not quite
sure how effective this will be.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/top_frame_index.py:42: return 'Top frame changed
is frame #%d' % top_frame_index
Doesn't "top frame" refer to the first frame?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/test/crash_util_test.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/test/crash_util_test.py:16:
self.assertTrue(crash_util.IsSameFilePath(path1, path2))
Why these two are treated as the same file paths?

[Sharu, 2016-04-15 22:58:44]

Patchset #4 (id:170001) has been deleted

[Sharu, 2016-04-15 22:59:47]

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:120: """Get dep_path to DependencyRoll
dictionary for deps in
On 2016/04/15 18:35:18, stgao wrote:
> nit: One line summary here.
> nit: "Get" -> "Gets".
> 
> Let's try to be consistent with other code in the repo.

Ouch, didn't catch this.

Done.

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

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:7: """Determines if two paths represent
same path.
On 2016/04/15 06:05:24, Martin Barbella wrote:
> What's the use case for this? Just trying to understand the rationale for the
3
> directory intersection.

If a file moved directory, like from 'a/b/c/file.cc' to 'a/b/file.cc', as long
as it's new location is not too far from the old one (more than 3), we guess
it's the same file.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
On 2016/04/15 18:35:18, stgao wrote:
> How about adding the reason behind this comparison?

I just port over this method which is the method we use in clusterfuzz. I
thought we already had agreement on this as others? We can discuss this if you
have concerns here.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:19: path_parts1 = path1.split('/')
On 2016/04/15 06:05:24, Martin Barbella wrote:
> Nit: _1. Ditto for the other.

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:26: return len(intersection) >= (min(3,
min(len(path_parts1),
On 2016/04/15 18:35:18, stgao wrote:
> min(3, 2, 1) == 1

I think this is the intended behavior of this method. However it may be
problematic. like two files:
DEPS
a/b/c/d/e/f/g/DEPS 
would be treated as the same file.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:20: if not crash_stack:
On 2016/04/15 18:35:19, stgao wrote:
> It seems this is just one callstack, while a crash stack might include
multiple
> callstacks. We'd better use the terms consistently to avoid confusion. Maybe
I'm
> wrong with the terms too?

crash_stack is one callstack, the one that has the highest priority and cause
the crash.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:28: for dep in deps:
On 2016/04/15 18:35:19, stgao wrote:
> Can we merge these two for loop?

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:84: 
On 2016/04/15 06:05:24, Martin Barbella wrote:
> Nit: Remove this whitespace and the one on 88.

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:97: # If a dependency is deleted or
is a .h file, skip this changelog.
On 2016/04/15 06:05:24, Martin Barbella wrote:
> s/skip this changelog/skip this file/ right?

Right, done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:98: if not touched_file.new_path or
touched_file.new_path.endswith('.h'):
On 2016/04/15 18:35:18, stgao wrote:
> Can we use the change_type instead?
> 
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:101: if changelog.reverted_revision:
On 2016/04/15 18:35:19, stgao wrote:
> Should this be in this inner for loop?

Ouch, done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:104:
ignore_cls.add(changelog.reverted_revision)
On 2016/04/15 18:35:19, stgao wrote:
> Should we including the reverting cl too?

I didn't add the reverting cls in dep_file_to_changelogs, so I didn't add it.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:146: def
GetDepFileToBlame(stacktrace, stack_deps):
On 2016/04/15 18:35:19, stgao wrote:
> What does "DepFile" mean here? The DEPS file?

I wanted to mean a dict[dep_path][file_path] = Blame ... Cannot think of a
better name without being too long, any suggestion?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:172: git_parser =
GitRepository(stack_deps[frame.dep_path].repo_url,
On 2016/04/15 18:35:19, stgao wrote:
> naming nit: it's not just a parser.

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:211: if not
crash_util.IsSameFilePath(crashed_file_path, touched_file_path):
On 2016/04/15 18:35:18, stgao wrote:
> Note: syzyasan has lower case file path in ClusterFuzz.

Modified this inside IsSameFilePath function

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:238: start_revision, end_revision =
regression_range
On 2016/04/15 18:35:19, stgao wrote:
> good_revision, bad_revision?

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:243: start_revision, end_revision,
'all')
On 2016/04/15 18:35:19, stgao wrote:
> Why 'all' instead of the specific os platform?

I was supposed to set it to None :( 

Changed it.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/results.py:21: changelog_json = self.changelog.ToDict()
On 2016/04/15 18:35:19, stgao wrote:
> This seems unnecessary as we only refer to several fields. Why not refer to
them
> directly?

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/results.py:66: stacktrace and touched by the changelog of
this result.
On 2016/04/15 18:35:19, stgao wrote:
> changelog here might not be that clear enough. It seems we meant to refer to a
> commit or revision.

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/aggregator.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/aggregator.py:18: curr_score, curr_reason =
scorer(result)
On 2016/04/15 18:35:19, stgao wrote:
> no abbreviation.

Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/aggregator.py:19: score *= curr_score
On 2016/04/15 18:35:19, stgao wrote:
> So we change from a vector of scores to multiplying all scores?
> Do you plan to run some experiment to compare both approach?

Add a TODO here to compare these 2 methods later.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/min_distance.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/min_distance.py:24: if not hasattr(result,
'min_distance'):
On 2016/04/15 06:05:24, Martin Barbella wrote:
> What would cause this to happen, other than an incorrect call? It may not be
> worth handling.

I add this because I thought later I may add BlameResult, which won't be applied
to this scorer. Use this to enforce that right scorer applies to right kind of
Result.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/min_distance.py:37: return 0.8
On 2016/04/15 18:35:19, stgao wrote:
> Where are these numbers from? Some comments in the code would be great.

Added TODO, Done.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/test/top_frame_index_test.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/test/top_frame_index_test.py:12: DUMMY_CHANGELOG
= ChangeLog.FromDict({
On 2016/04/15 18:35:19, stgao wrote:
> This is repeated for quite a few times, could it be shared?

Yes, created a scorer_test_suite

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/scorers/top_frame_index.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/top_frame_index.py:34: return 1 - top_frame_index
/ float(self.max_top_n)
On 2016/04/15 18:35:19, stgao wrote:
> I think this is reasonable, but without experiment and comparison I'm not
quite
> sure how effective this will be.

Add a todo

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/scorers/top_frame_index.py:42: return 'Top frame changed
is frame #%d' % top_frame_index
On 2016/04/15 18:35:19, stgao wrote:
> Doesn't "top frame" refer to the first frame?

Not necessarily true I guess, not sure, @mbarbella, how do you think the naming?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/test/crash_util_test.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/test/crash_util_test.py:16:
self.assertTrue(crash_util.IsSameFilePath(path1, path2))
On 2016/04/15 18:35:19, stgao wrote:
> Why these two are treated as the same file paths?

This is the method we use in clusterfuzz all the time, I just port it over. So
we assume if there are 3 parts mating, these 2 are the same. Can we rely on this
assumption?

[Sharu, 2016-04-15 23:07:11]

Patchset #4 (id:190001) has been deleted

[Martin Barbella, 2016-04-18 21:01:18]

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:131: deps_rolls =
GetChromiumDEPSRolls(old_cr_revision, new_cr_revision,
This name and |dependency_rolls| below have the same meaning, but are fairly
different. These should be updated.

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

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
I'm not fully convinced that this is right, but it may be fine for now. Is there
any more reliable way we could deal with file moves?

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:25: intersection =
set(path_parts_1).intersection(set(path_parts_2))
Set intersection might not be ideal here. I didn't dig into it, but I don't
think we have any guarantee that two directories in a path won't have the same
name.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:30: def
GetDepFileToChangeLogsAndIgnoreCls(regression_deps_rolls, stack_deps):
In general I'm not a fan of these function names. I know what they return, but I
don't really know what they do to get to that point.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:32: changelogs in regressoin range
and a set of revisions that should be ignored.
s/regressoin/regression/

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:87: dep_roll['new_revision'])
Indentation.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:238: # Right now we only care about
those deps for all platforms.
Should this be a TODO?

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/results.py:51: return self.ToString()
This doesn't seem like what would usually be expected from __repr__. Maybe
repr(self.ToDict()), but even that's a stretch. Any reason why this is needed at
all?

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/results.py:95: if region1[-1] < region2[0]:
I suspect there's a more compact way to express this, but it's fairly readable
as-is. Either way is fine.

[Sharu Jiang, 2016-04-19 20:38:36]

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

[Sharu Jiang, 2016-04-19 20:38:38]

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:131: deps_rolls =
GetChromiumDEPSRolls(old_cr_revision, new_cr_revision,
On 2016/04/18 21:01:18, Martin Barbella wrote:
> This name and |dependency_rolls| below have the same meaning, but are fairly
> different. These should be updated.

Done.

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

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
On 2016/04/18 21:01:18, Martin Barbella wrote:
> I'm not fully convinced that this is right, but it may be fine for now. Is
there
> any more reliable way we could deal with file moves?

Not a reliable way either but an alternative is to compute the difference
instead of similarity, for example if there are more than 2 parts different, we
think it's not the same path.

If we want to be very sure whether 2 paths are the same file, we may have to
track down all the file path change in very revision(implement a link list
maybe). That may be a big change. 

Add a TODO here to improve later.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:25: intersection =
set(path_parts_1).intersection(set(path_parts_2))
On 2016/04/18 21:01:18, Martin Barbella wrote:
> Set intersection might not be ideal here. I didn't dig into it, but I don't
> think we have any guarantee that two directories in a path won't have the same
> name.

Done.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:30: def
GetDepFileToChangeLogsAndIgnoreCls(regression_deps_rolls, stack_deps):
On 2016/04/18 21:01:18, Martin Barbella wrote:
> In general I'm not a fan of these function names. I know what they return, but
I
> don't really know what they do to get to that point.

How about I change the name to 'GetFileToChangelogsForDeps' ?

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:32: changelogs in regressoin range
and a set of revisions that should be ignored.
On 2016/04/18 21:01:18, Martin Barbella wrote:
> s/regressoin/regression/

Done.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:87: dep_roll['new_revision'])
On 2016/04/18 21:01:18, Martin Barbella wrote:
> Indentation.

Done.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:238: # Right now we only care about
those deps for all platforms.
On 2016/04/18 21:01:18, Martin Barbella wrote:
> Should this be a TODO?

Sorry, this is a deprecated comment, should delete it.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/results.py:51: return self.ToString()
On 2016/04/18 21:01:18, Martin Barbella wrote:
> This doesn't seem like what would usually be expected from __repr__. Maybe
> repr(self.ToDict()), but even that's a stretch. Any reason why this is needed
at
> all?

This is more for a debug purpose, but yeah, this may not be needed. Delete it.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/results.py:95: if region1[-1] < region2[0]:
On 2016/04/18 21:01:18, Martin Barbella wrote:
> I suspect there's a more compact way to express this, but it's fairly readable
> as-is. Either way is fine.

Since it's too long to fit in one line, I just keep as it is.

[Martin Barbella, 2016-04-19 22:21:29]

Mostly stylistic nits, but seems fine overall. I didn't look closely at every
file, so it may be worth doing a pass for similar issues in other places.

Once these are addressed, I'd like to do another pass.

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:30: def
GetDepFileToChangeLogsAndIgnoreCls(regression_deps_rolls, stack_deps):
On 2016/04/19 20:38:37, sharu jiang wrote:
> On 2016/04/18 21:01:18, Martin Barbella wrote:
> > In general I'm not a fan of these function names. I know what they return,
but
> I
> > don't really know what they do to get to that point.
> 
> How about I change the name to 'GetFileToChangelogsForDeps' ? 

Seems better, but potentially still problematic. This was meant as a general
comment, though.

Taking a step back, my concern is about not being able to tell what a function
is doing just by looking at it. In these cases, the return values are clear but
not what work is being done to get to that point, or why.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:126: os_platform (str): The target OS
platform of the Chrome or test binary.
|platform| would probably have the same meaning here. The os_ seems redundant.

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

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:8: def IsSameFilePath(path1, path2):
With the style change below, these look a bit awkward now. Replace with _1 and
_2 here as well.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:40: total_same_parts =
sum([min(parts_count_1[part], parts_count_2[part]) for
This is complicated enough that it warrants a comment.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:97: # If a dependency is deleted or
is a .h file, skip this file.
This is self-explanatory from the code itself.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:173: frame.file_path,
stack_deps[frame.dep_path].revision))
Blank line after this line.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:177: def
FindMatchResults(dep_file_to_changelogs,
These parameter names are a bit confusing.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/results.py:88: 
Consistent whitespace after defs/docstrings in this file.

[stgao, 2016-04-20 17:41:26]

Publish some more comments. Will take a further look later today.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:104:
ignore_cls.add(changelog.reverted_revision)
On 2016/04/15 22:59:46, Sharu wrote:
> On 2016/04/15 18:35:19, stgao wrote:
> > Should we including the reverting cl too?
> 
> I didn't add the reverting cls in dep_file_to_changelogs, so I didn't add it.

What if the reverting CLs showing up in the regression range?

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:243: start_revision, end_revision,
'all')
On 2016/04/15 22:59:46, Sharu wrote:
> On 2016/04/15 18:35:19, stgao wrote:
> > Why 'all' instead of the specific os platform?
> 
> I was supposed to set it to None :( 
> 
> Changed it.

Why set it to None?

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

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
On 2016/04/19 20:38:37, sharu jiang wrote:
> On 2016/04/18 21:01:18, Martin Barbella wrote:
> > I'm not fully convinced that this is right, but it may be fine for now. Is
> there
> > any more reliable way we could deal with file moves?
> 
> Not a reliable way either but an alternative is to compute the difference
> instead of similarity, for example if there are more than 2 parts different,
we
> think it's not the same path.
> 
> If we want to be very sure whether 2 paths are the same file, we may have to
> track down all the file path change in very revision(implement a link list
> maybe). That may be a big change. 
> 
> Add a TODO here to improve later.

Have you explored the different change types Gitiles provides?
There should be a better way to detect file move.
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...

It seems that getting more familiar with the Gitiles APIs will make the
implementation easier.

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:97: if (touched_file.change_type ==
'delete' or
Can we not hard-code it?
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:133: if frame.dep_path not in
stack_deps or frame.file_path.endswith('.h'):
Add the reason why we ignore ".h" header file?

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:142: def
GetFileToBlameForDeps(stacktrace, stack_deps):
GetBlameForFilesGroupedByDeps?

[Sharu Jiang, 2016-04-20 18:37:16]

Patchset #7 (id:270001) has been deleted

[Sharu Jiang, 2016-04-20 18:38:56]

Patchset #7 (id:290001) has been deleted

[Sharu Jiang, 2016-04-20 18:54:02]

Patchset #7 (id:310001) has been deleted

[Sharu Jiang, 2016-04-20 18:54:24]

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:104:
ignore_cls.add(changelog.reverted_revision)
On 2016/04/20 17:41:25, stgao wrote:
> On 2016/04/15 22:59:46, Sharu wrote:
> > On 2016/04/15 18:35:19, stgao wrote:
> > > Should we including the reverting cl too?
> > 
> > I didn't add the reverting cls in dep_file_to_changelogs, so I didn't add
it.
> 
> What if the reverting CLs showing up in the regression range?

All the changelogs in the regression range we are interested in are stored in
dep_to_file_to_changelogs. If I didn't include those reverting cls in
dep_to_file_to_changelogs, it won't show anywhere afterward, I think it won't be
a problem.

https://codereview.chromium.org/1861373003/diff/160001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:243: start_revision, end_revision,
'all')
On 2016/04/20 17:41:25, stgao wrote:
> On 2016/04/15 22:59:46, Sharu wrote:
> > On 2016/04/15 18:35:19, stgao wrote:
> > > Why 'all' instead of the specific os platform?
> > 
> > I was supposed to set it to None :( 
> > 
> > Changed it.
> 
> Why set it to None?

It's a mistake :(

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:126: os_platform (str): The target OS
platform of the Chrome or test binary.
On 2016/04/19 22:21:28, Martin Barbella wrote:
> |platform| would probably have the same meaning here. The os_ seems redundant.

This is to be consist with other functions in this file. Changed all.

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

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:8: def IsSameFilePath(path1, path2):
On 2016/04/19 22:21:28, Martin Barbella wrote:
> With the style change below, these look a bit awkward now. Replace with _1 and
> _2 here as well.

Done.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:40: total_same_parts =
sum([min(parts_count_1[part], parts_count_2[part]) for
On 2016/04/19 22:21:28, Martin Barbella wrote:
> This is complicated enough that it warrants a comment.

Done.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:97: # If a dependency is deleted or
is a .h file, skip this file.
On 2016/04/19 22:21:28, Martin Barbella wrote:
> This is self-explanatory from the code itself.

Done.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:173: frame.file_path,
stack_deps[frame.dep_path].revision))
On 2016/04/19 22:21:29, Martin Barbella wrote:
> Blank line after this line.

Done.

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:177: def
FindMatchResults(dep_file_to_changelogs,
On 2016/04/19 22:21:28, Martin Barbella wrote:
> These parameter names are a bit confusing.

Change it to dep_to_file_to_changelogs

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/1861373003/diff/230001/appengine/findit/crash...
appengine/findit/crash/results.py:88: 
On 2016/04/19 22:21:29, Martin Barbella wrote:
> Consistent whitespace after defs/docstrings in this file.

Done.

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:97: if (touched_file.change_type ==
'delete' or
On 2016/04/20 17:41:26, stgao wrote:
> Can we not hard-code it?
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...

Done.

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:133: if frame.dep_path not in
stack_deps or frame.file_path.endswith('.h'):
On 2016/04/20 17:41:26, stgao wrote:
> Add the reason why we ignore ".h" header file?

This is logic in findit for clusterfuzz, I just use the same logic here. However
I double checked this with sheriff, the conclusion is we shouldn't ignore those
.h files. 

Changed it.

https://codereview.chromium.org/1861373003/diff/250001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:142: def
GetFileToBlameForDeps(stacktrace, stack_deps):
On 2016/04/20 17:41:26, stgao wrote:
> GetBlameForFilesGroupedByDeps?

Done.

[Martin Barbella, 2016-04-20 22:26:17]

lgtm

[Sharu Jiang, 2016-04-21 02:06:01]

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

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
On 2016/04/20 17:41:26, stgao wrote:
> On 2016/04/19 20:38:37, sharu jiang wrote:
> > On 2016/04/18 21:01:18, Martin Barbella wrote:
> > > I'm not fully convinced that this is right, but it may be fine for now. Is
> > there
> > > any more reliable way we could deal with file moves?
> > 
> > Not a reliable way either but an alternative is to compute the difference
> > instead of similarity, for example if there are more than 2 parts different,
> we
> > think it's not the same path.
> > 
> > If we want to be very sure whether 2 paths are the same file, we may have to
> > track down all the file path change in very revision(implement a link list
> > maybe). That may be a big change. 
> > 
> > Add a TODO here to improve later.
> 
> Have you explored the different change types Gitiles provides?
> There should be a better way to detect file move.
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...
> 
> It seems that getting more familiar with the Gitiles APIs will make the
> implementation easier.

Yes, the copy/rename change type can help me detect the file move, but still, we
cannot avoid tracking all the revisions from crashed revision to the beginning
of regression range revision. (maybe use a set to group all the same file paths)

We can do this in a separate cl.

[stgao, 2016-04-21 17:31:54]

lgtm with nits.

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

https://codereview.chromium.org/1861373003/diff/210001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:10: if they match either more than 3 or min
of path lengths.
On 2016/04/21 02:06:01, sharu jiang wrote:
> On 2016/04/20 17:41:26, stgao wrote:
> > On 2016/04/19 20:38:37, sharu jiang wrote:
> > > On 2016/04/18 21:01:18, Martin Barbella wrote:
> > > > I'm not fully convinced that this is right, but it may be fine for now.
Is
> > > there
> > > > any more reliable way we could deal with file moves?
> > > 
> > > Not a reliable way either but an alternative is to compute the difference
> > > instead of similarity, for example if there are more than 2 parts
different,
> > we
> > > think it's not the same path.
> > > 
> > > If we want to be very sure whether 2 paths are the same file, we may have
to
> > > track down all the file path change in very revision(implement a link list
> > > maybe). That may be a big change. 
> > > 
> > > Add a TODO here to improve later.
> > 
> > Have you explored the different change types Gitiles provides?
> > There should be a better way to detect file move.
> >
>
https://chromium.googlesource.com/infra/infra/+/master/appengine/findit/commo...
> > 
> > It seems that getting more familiar with the Gitiles APIs will make the
> > implementation easier.
> 
> Yes, the copy/rename change type can help me detect the file move, but still,
we
> cannot avoid tracking all the revisions from crashed revision to the beginning
> of regression range revision. (maybe use a set to group all the same file
paths)
> 
> We can do this in a separate cl. 

If we plan to make this change, a bug should be created and a TODO be added
here.

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

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:8: def IsSameFilePath(path_1, path_2):
Probably, we could start with a simple strict equal, and then explore smarter
checking like this if that runs into trouble.

But I'd leave the decision to you.

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:135: if frame.dep_path not in
stack_deps or frame.file_path.endswith('.h'):
This is a general comment for this CL and all up-coming CL for the migration
from Findit for ClusterFuzz to the appengine.

Even though the features are moved over from Findit for ClusterFuzz, we'd better
add comments to explain why header files are ignored and other decisions like
this. That will help new-comers to understand the code and improve code health.

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
File appengine/findit/crash/scorers/top_frame_index.py (right):

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
appengine/findit/crash/scorers/top_frame_index.py:13: _MAX_TOP_N_FRAMES = 7
This should go into the config saved in datastore.
It could be handled in a separate CL, but a TODO with a bug is needed.

[stgao, 2016-04-21 17:32:53]

BTW, please follow other Findit CL for the format of CL description.

[Sharu Jiang, 2016-04-21 22:28:44]

Patchset #8 (id:350001) has been deleted

[Sharu Jiang, 2016-04-21 22:38:28]

Patchset #8 (id:370001) has been deleted

[Sharu Jiang, 2016-04-21 22:38:50]

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

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
appengine/findit/crash/crash_util.py:8: def IsSameFilePath(path_1, path_2):
On 2016/04/21 17:31:54, stgao wrote:
> Probably, we could start with a simple strict equal, and then explore smarter
> checking like this if that runs into trouble.
> 
> But I'd leave the decision to you.

I prefer leave this as it is now, let's experiment this after the infra done.

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
File appengine/findit/crash/findit_for_crash.py (right):

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
appengine/findit/crash/findit_for_crash.py:135: if frame.dep_path not in
stack_deps or frame.file_path.endswith('.h'):
On 2016/04/21 17:31:54, stgao wrote:
> This is a general comment for this CL and all up-coming CL for the migration
> from Findit for ClusterFuzz to the appengine.
> 
> Even though the features are moved over from Findit for ClusterFuzz, we'd
better
> add comments to explain why header files are ignored and other decisions like
> this. That will help new-comers to understand the code and improve code
health.

Actually after I double check with sheriffs, we shouldn't ignore the .h file, I
will delete this check.

Also, add more comments.

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
File appengine/findit/crash/scorers/top_frame_index.py (right):

https://codereview.chromium.org/1861373003/diff/330001/appengine/findit/crash...
appengine/findit/crash/scorers/top_frame_index.py:13: _MAX_TOP_N_FRAMES = 7
On 2016/04/21 17:31:54, stgao wrote:
> This should go into the config saved in datastore.
> It could be handled in a separate CL, but a TODO with a bug is needed.

Done.

[Sharu Jiang, 2016-04-21 22:40:15]

Description was changed from

==========
Add FindItForCrash core module to findit for crash. The algorithm finds match of
file paths in changlelogs in regression range and file paths in stacktrace. 

Added scorers to apply heuristic rules - top_frame_index, min_distance.
==========

to

==========
[Findit] Initial code of findit for crash. Add scorers to apply heuristic rules.

Add FindItForCrash core module to findit for crash. The algorithm finds match of
file paths in changlelogs in regression range and file paths in stacktrace. 

Added scorers to apply heuristic rules - top_frame_index, min_distance.
==========

[Sharu Jiang, 2016-04-21 22:48:27]

Patchset #8 (id:390001) has been deleted

[Sharu Jiang, 2016-04-21 23:03:11]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-04-21 23:03:11]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org,
mbarbella@chromium.org
Link to the patchset: https://codereview.chromium.org/1861373003/#ps410001
(title: "Fix nits and rebase")

[commit-bot: I haz the power, 2016-04-21 23:03:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861373003/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861373003/410001

[commit-bot: I haz the power, 2016-04-21 23:07:21]

Description was changed from

==========
[Findit] Initial code of findit for crash. Add scorers to apply heuristic rules.

Add FindItForCrash core module to findit for crash. The algorithm finds match of
file paths in changlelogs in regression range and file paths in stacktrace. 

Added scorers to apply heuristic rules - top_frame_index, min_distance.
==========

to

==========
[Findit] Initial code of findit for crash. Add scorers to apply heuristic rules.

Add FindItForCrash core module to findit for crash. The algorithm finds match of
file paths in changlelogs in regression range and file paths in stacktrace. 

Added scorers to apply heuristic rules - top_frame_index, min_distance.

Committed:
https://chromium.googlesource.com/infra/infra/+/194e660ea8aaad7f44ae5ad4f4eb6...
==========

[commit-bot: I haz the power, 2016-04-21 23:07:22]

Committed patchset #8 (id:410001) as
https://chromium.googlesource.com/infra/infra/+/194e660ea8aaad7f44ae5ad4f4eb6...