[Predator] Add ``Project`` class and ``ClassifySuspect`` method to project and component classifier

[Predator] Add ``Project`` class and ``ClassifySuspect`` method to project classifier and component classifier.

Add ``Project`` class and ``ClassifySuspect`` method to project classifier and component classifier.
 
Also add validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the validations in this cl.

BUG=657177, 685448
TBR=wrengr@chromium.org

Review-Url: https://codereview.chromium.org/2657913002
Committed: https://chromium.googlesource.com/infra/infra/+/c6075535547c4e038d4466680d90479388581625

[Sharu Jiang, 2017-01-25 22:40:01]

Description was changed from

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project classifier.

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

to

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project. classifier.

Add ``Project`` class and ``ClassifySuspect`` to project. classifier, also add
validation for component classifier and project classifier.

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

[Sharu Jiang, 2017-01-25 22:40:01]

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

[Sharu Jiang, 2017-01-25 22:42:10]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2017-01-25 22:42:17]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2017-01-26 01:13:18]

Description was changed from

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project. classifier.

Add ``Project`` class and ``ClassifySuspect`` to project. classifier, also add
validation for component classifier and project classifier.

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

to

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project. classifier.

Add ``Project`` class and ``ClassifySuspect`` to project. classifier, also add
validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
==========

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

Patchset #1 (id:40001) has been deleted

[Sharu Jiang, 2017-01-26 01:13:31]

Patchset #1 (id:60001) has been deleted

[Sharu Jiang, 2017-01-26 01:14:10]

PTAL.

[Sharu Jiang, 2017-01-26 01:14:34]

Description was changed from

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project. classifier.

Add ``Project`` class and ``ClassifySuspect`` to project. classifier, also add
validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
==========

to

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project. classifier.

Add ``Project`` class and ``ClassifySuspect`` to project. classifier, also add
validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
TBR=wrengr@chromium.org
==========

[Sharu Jiang, 2017-01-26 01:50:15]

Description was changed from

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` to project. classifier.

Add ``Project`` class and ``ClassifySuspect`` to project. classifier, also add
validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
TBR=wrengr@chromium.org
==========

to

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` method to project
classifier and component classifier.

Add ``Project`` class and ``ClassifySuspect`` method to project classifier and
component classifier.
 
Also add validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
TBR=wrengr@chromium.org
==========

[chanli, 2017-01-26 21:31:07]

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/component.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component.py:39: def MatchesTouchedFile(self, dep_path,
touched_file):
This function is similar to MatchesTouchedFile in project, is it possible to
merge them somehow?

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/component_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component_classifier.py:41: 
Add docstring for top_n

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component_classifier.py:75: """ Classifies components of
a change log.
Nit: update docstring and args

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component_classifier.py:89: def
_GetClassFromTouchedFile(dep_path, touched_file):
How does GetClassFromTouchedFile get touched_file?

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
Nit: 2017

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:26: path_regexs] if path_regexs else None
how about else []? same for function_regexs. so in MatchesStackFrame you can
have  for path_regex in self.path_regexs directly

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:41: print frame.function,
function_regex.pattern
Nit:remove this

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:45: return False
This can be removed if you use for host_directory in self.host_directories or []
like line 67

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:73: def GetName(self, dep_path):
Based on returned value, it's not just project's name?

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/project_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project_classifier.py:50: def
_RankFunctionForJava(occurrence):
Nit: since this function is within another function, '_' can be removed

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project_classifier.py:58: index =
self.non_chromium_project_rank_priority[project_name]
.get() to be safe?

[Martin Barbella, 2017-01-26 23:59:48]

Thanks for working on this. Looks like it'll make things easier to understand
when it lands.

If any of these are significant work or existing issues feel free to push back,
but in general I'd like to see fewer TODOs.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/component.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component.py:42: # need to return two (old path new path
may have different components)
I suspect that new path will usually make the most sense. It might not be worth
the complexity of returning both.

Have we done any analysis here? Are there any bugs tracking this?

Seems fine to remove the TODO to me, we can always revisit it later.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/component_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:45: def
_GetClassFromStackFrame(frame):
s/Class/Component/ seems a bit clearer to me.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:53: classes =
map(_GetClassFromStackFrame,
Similarly, classes is a bit too generic to be easily understood just from
glancing over the code.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:87: # TODO(ymzhang): use
component of new path as default. RENAME might
Same concern mentioned on this TODO in the previous file. Also, if it does seem
worth keeping, it feels unnecessary to have it in two places.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/project.py:11: # TODO(http://crbug.com/659346): write the
coverage tests.
Is there anything still blocking it? Would be nice to see this addressed.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/project_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/project_classifier.py:63: #
TODO(http://crbug.com/657177): refactor this into
Is there something specific that makes this difficult? Seems like the right time
to take care of it.

[Sharu Jiang, 2017-01-27 03:20:42]

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/component.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component.py:39: def MatchesTouchedFile(self, dep_path,
touched_file):
On 2017/01/26 21:31:06, chanli wrote:
> This function is similar to MatchesTouchedFile in project, is it possible to
> merge them somehow?

Since the ``Project`` and ``Component`` are 2 fundamentally different things and
they are namedtuples too, so I cannot define a base class for these two,
considered the similarity is limited, I would prefer to leave them as it is.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/component_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component_classifier.py:41: 
On 2017/01/26 21:31:06, chanli wrote:
> Add docstring for top_n

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component_classifier.py:75: """ Classifies components of
a change log.
On 2017/01/26 21:31:06, chanli wrote:
> Nit: update docstring and args

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/component_classifier.py:89: def
_GetClassFromTouchedFile(dep_path, touched_file):
On 2017/01/26 21:31:06, chanli wrote:
> How does GetClassFromTouchedFile get touched_file?

it is the parameter of the function.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2017/01/26 21:31:06, chanli wrote:
> Nit: 2017

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:26: path_regexs] if path_regexs else None
On 2017/01/26 21:31:06, chanli wrote:
> how about else []? same for function_regexs. so in MatchesStackFrame you can
> have  for path_regex in self.path_regexs directly

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:41: print frame.function,
function_regex.pattern
On 2017/01/26 21:31:06, chanli wrote:
> Nit:remove this 

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:45: return False
On 2017/01/26 21:31:06, chanli wrote:
> This can be removed if you use for host_directory in self.host_directories or
[]
> like line 67

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project.py:73: def GetName(self, dep_path):
On 2017/01/26 21:31:07, chanli wrote:
> Based on returned value, it's not just project's name?

Right, should add more doc about that.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/project_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project_classifier.py:50: def
_RankFunctionForJava(occurrence):
On 2017/01/26 21:31:07, chanli wrote:
> Nit: since this function is within another function, '_' can be removed

Done.

https://codereview.chromium.org/2657913002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/project_classifier.py:58: index =
self.non_chromium_project_rank_priority[project_name]
On 2017/01/26 21:31:07, chanli wrote:
> .get() to be safe?

If the key is not in the dict, then this is a bug in config and we'd better let
it fail loudly.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/component.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component.py:42: # need to return two (old path new path
may have different components)
On 2017/01/26 23:59:48, Martin Barbella wrote:
> I suspect that new path will usually make the most sense. It might not be
worth
> the complexity of returning both.
> 
> Have we done any analysis here? Are there any bugs tracking this?
> 
> Seems fine to remove the TODO to me, we can always revisit it later.

There is no bug tracking this before, but I just filed one :) 
Updated this TODO with the bug, and I think we'd better leave this TODO here, so
later if we want to revisit, we can find the right place quickly.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/component_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:45: def
_GetClassFromStackFrame(frame):
On 2017/01/26 23:59:48, Martin Barbella wrote:
> s/Class/Component/ seems a bit clearer to me.

Done.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:53: classes =
map(_GetClassFromStackFrame,
On 2017/01/26 23:59:48, Martin Barbella wrote:
> Similarly, classes is a bit too generic to be easily understood just from
> glancing over the code.

Done.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:87: # TODO(ymzhang): use
component of new path as default. RENAME might
On 2017/01/26 23:59:48, Martin Barbella wrote:
> Same concern mentioned on this TODO in the previous file. Also, if it does
seem
> worth keeping, it feels unnecessary to have it in two places.

Done.

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/project.py:11: # TODO(http://crbug.com/659346): write the
coverage tests.
On 2017/01/26 23:59:48, Martin Barbella wrote:
> Is there anything still blocking it? Would be nice to see this addressed.

Forgot to add it...
Done

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
File appengine/findit/crash/project_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/100001/appengine/findit/crash...
appengine/findit/crash/project_classifier.py:63: #
TODO(http://crbug.com/657177): refactor this into
On 2017/01/26 23:59:48, Martin Barbella wrote:
> Is there something specific that makes this difficult? Seems like the right
time
> to take care of it.

Forgot to remove it, this is what this cl does.

[Sharu Jiang, 2017-01-27 03:25:01]

Patchset #3 (id:120001) has been deleted

[Sharu Jiang, 2017-01-27 03:28:26]

Patchset #3 (id:140001) has been deleted

[Sharu Jiang, 2017-01-27 08:13:58]

Patchset #4 (id:180001) has been deleted

[lijeffrey, 2017-01-27 15:42:20]

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/component.py (right):

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
appengine/findit/crash/component.py:49: if
self.path_regex.match(os.path.join(dep_path, path)):
how about just return path.regex.match(...)

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
appengine/findit/crash/project.py:82: # if the path matches the path patterns.
nit: If

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
appengine/findit/crash/project.py:90: return True
I think this can be simplified slightly to:

return (
    any(path_regex.match(
        os.path.join(dep_path, path) for path_regex in self.path_regexs)) or
    any(dep_path.startswith(
        host_directory) for host_direcotry in self.host_directories)))

[Sharu Jiang, 2017-01-27 18:39:04]

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/component.py (right):

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
appengine/findit/crash/component.py:49: if
self.path_regex.match(os.path.join(dep_path, path)):
On 2017/01/27 15:42:20, lijeffrey wrote:
> how about just return path.regex.match(...)

Done.

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
appengine/findit/crash/project.py:82: # if the path matches the path patterns.
On 2017/01/27 15:42:20, lijeffrey wrote:
> nit: If

Done.

https://codereview.chromium.org/2657913002/diff/200001/appengine/findit/crash...
appengine/findit/crash/project.py:90: return True
On 2017/01/27 15:42:20, lijeffrey wrote:
> I think this can be simplified slightly to:
> 
> return (
>     any(path_regex.match(
>         os.path.join(dep_path, path) for path_regex in self.path_regexs)) or
>     any(dep_path.startswith(
>         host_directory) for host_direcotry in self.host_directories)))

Hm... I want to keep these two conditions separate to have more readability.

[Sharu Jiang, 2017-01-27 18:41:48]

Patchset #5 (id:220001) has been deleted

[Sharu Jiang, 2017-01-27 22:44:09]

Ping :)

[Martin Barbella, 2017-01-27 23:30:58]

lgtm

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:11: # Some projects like "chromium", it has
many dependency projects like
Nit: s/it has/have/

[wrengr, 2017-01-30 19:14:19]

lgtm with nits/comments. In particular, the stuff about None is concerning

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/component_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:30: self.components = [] if
components is None else components
Can simplify that to ``self.components = components or []``

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:40: top_n (int): The number of
top components for the stack
would help make the code below more legible to call this ``top_n_results`` or
``top_n_components`` etc

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:43: List of top 2 components.
Should say returns the top ``top_n`` components

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:45: def
GetComponentFromStackFrame(frame):
Since this doesn't close over any local variables, it'd be cleaner to have it as
a top-level method rather than nested in here.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:55: return
RankByOccurrence(components, top_n) or None
What's the point of returning ``None`` rather than ``[]``? Using the former
introduces places where we'd have to check whether the result is None before we
can iterate over the results. We can always check for the falsiness of the empty
list in order to detect failure.

Also, if None can be returned, this should be noted in the docstring.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:63: suspects (list of Suspect):
Culprit suspects.
missing documentation for ``top_n``

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:66: List of top 2 components.
should say ``top_n`` since not hard-coded to 2

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:70:
components.extend(self.ClassifySuspect(suspect))
Pretty sure this will break whenever ClassifySuspect returns None

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:73: rank_function=lambda x:
-len(x)) or None
Again, what's the point of returning ``None`` rather than just ``[]``?

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:88: def
GetComponentFromTouchedFile(touched_file):
Again, would be simpler to float this out. Can be made private if you don't want
client code to use it

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:60: projects = [Project(name,
path_regexs, function_regexs, host_directories)
Since the order of the tuple/arguments is the same, I think you should be able
to say ``map(Project,
project_classifier_config['project_path_function_hosts'])`` instead

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:22: ['name', 'path_regexs', 'function_regexs',
Nit: "regexes" is the standard way to pluralize "regex"

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:24: """A representation of a "project".
This line doesn't explain anything beyond what the class name does

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:36: host_directories (list of str): The root
directoris of this project and its
(s/directoris/directories/ && s/and its/and their/) || s/directoris/directory/

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:38: N.B. If ``host_directories`` is availabe,
this project can match
s/availabe/available/

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:40: from this project, else if this
information is missing, the project cannot
s/, else if/. If/

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:41: tell that from ``dep_path``, however that
doesn't mean the suspect or
s/, however/; however,/

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:49: path_regexs = [re.compile(path_regex) for
path_regex in
can replace this line with ``map(re.compile, path_regexs or [])``

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:52: function_regexs] if function_regexs else
[]
ditto

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:82: if touched_file.change_type ==
ChangeType.DELETE:
Since this conditional shows up repeatedly, should probably turn it into a
property of the touched_file

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/test/project_classifier_test.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/test/project_classifier_test.py:24: for name,
path_regexs, function_regexs, host_directories
again, should be able to ``map(Project, config['project_path_function_hosts'])``

[Sharu Jiang, 2017-01-30 22:23:52]

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/component_classifier.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:30: self.components = [] if
components is None else components
On 2017/01/30 19:14:18, wrengr wrote:
> Can simplify that to ``self.components = components or []``

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:40: top_n (int): The number of
top components for the stack
On 2017/01/30 19:14:18, wrengr wrote:
> would help make the code below more legible to call this ``top_n_results`` or
> ``top_n_components`` etc

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:43: List of top 2 components.
On 2017/01/30 19:14:19, wrengr wrote:
> Should say returns the top ``top_n`` components

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:45: def
GetComponentFromStackFrame(frame):
On 2017/01/30 19:14:18, wrengr wrote:
> Since this doesn't close over any local variables, it'd be cleaner to have it
as
> a top-level method rather than nested in here.

I hide this because I don't want client to use it and ``ClassifyCallStack`` is a
very short function.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:55: return
RankByOccurrence(components, top_n) or None
On 2017/01/30 19:14:18, wrengr wrote:
> What's the point of returning ``None`` rather than ``[]``? Using the former
> introduces places where we'd have to check whether the result is None before
we
> can iterate over the results. We can always check for the falsiness of the
empty
> list in order to detect failure.
> 
> Also, if None can be returned, this should be noted in the docstring.

Acknowledged.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:63: suspects (list of Suspect):
Culprit suspects.
On 2017/01/30 19:14:19, wrengr wrote:
> missing documentation for ``top_n``

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:66: List of top 2 components.
On 2017/01/30 19:14:19, wrengr wrote:
> should say ``top_n`` since not hard-coded to 2

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:70:
components.extend(self.ClassifySuspect(suspect))
On 2017/01/30 19:14:19, wrengr wrote:
> Pretty sure this will break whenever ClassifySuspect returns None

Oops, return [] instead of None.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:73: rank_function=lambda x:
-len(x)) or None
On 2017/01/30 19:14:19, wrengr wrote:
> Again, what's the point of returning ``None`` rather than just ``[]``?

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:88: def
GetComponentFromTouchedFile(touched_file):
On 2017/01/30 19:14:19, wrengr wrote:
> Again, would be simpler to float this out. Can be made private if you don't
want
> client code to use it

This one use local variable suspect, so I'd like to keep it here.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:60: projects = [Project(name,
path_regexs, function_regexs, host_directories)
On 2017/01/30 19:14:19, wrengr wrote:
> Since the order of the tuple/arguments is the same, I think you should be able
> to say ``map(Project,
> project_classifier_config['project_path_function_hosts'])`` instead

It turned out that we cannot do that, but we can do something like:
map(Project, *zip(*project_classifier_config['project_path_function_hosts']))

But I don't think this is very clear and readable.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/project.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:11: # Some projects like "chromium", it has
many dependency projects like
On 2017/01/27 23:30:58, Martin Barbella wrote:
> Nit: s/it has/have/

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:22: ['name', 'path_regexs', 'function_regexs',
On 2017/01/30 19:14:19, wrengr wrote:
> Nit: "regexes" is the standard way to pluralize "regex"

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:24: """A representation of a "project".
On 2017/01/30 19:14:19, wrengr wrote:
> This line doesn't explain anything beyond what the class name does

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:36: host_directories (list of str): The root
directoris of this project and its
On 2017/01/30 19:14:19, wrengr wrote:
> (s/directoris/directories/ && s/and its/and their/) || s/directoris/directory/

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:38: N.B. If ``host_directories`` is availabe,
this project can match
On 2017/01/30 19:14:19, wrengr wrote:
> s/availabe/available/

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:40: from this project, else if this
information is missing, the project cannot
On 2017/01/30 19:14:19, wrengr wrote:
> s/, else if/. If/

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:41: tell that from ``dep_path``, however that
doesn't mean the suspect or
On 2017/01/30 19:14:19, wrengr wrote:
> s/, however/; however,/

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:49: path_regexs = [re.compile(path_regex) for
path_regex in
On 2017/01/30 19:14:19, wrengr wrote:
> can replace this line with ``map(re.compile, path_regexs or [])``

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:52: function_regexs] if function_regexs else
[]
On 2017/01/30 19:14:19, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/project.py:82: if touched_file.change_type ==
ChangeType.DELETE:
On 2017/01/30 19:14:19, wrengr wrote:
> Since this conditional shows up repeatedly, should probably turn it into a
> property of the touched_file

Done.

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/test/project_classifier_test.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/test/project_classifier_test.py:24: for name,
path_regexs, function_regexs, host_directories
On 2017/01/30 19:14:19, wrengr wrote:
> again, should be able to ``map(Project,
config['project_path_function_hosts'])``

As above, we cannot do that :(

[Sharu Jiang, 2017-01-30 22:24:33]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2017-01-30 22:24:33]

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

[commit-bot: I haz the power, 2017-01-30 22:24:44]

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

[wrengr, 2017-01-30 22:30:58]

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
File appengine/findit/crash/findit_for_chromecrash.py (right):

https://codereview.chromium.org/2657913002/diff/220002/appengine/findit/crash...
appengine/findit/crash/findit_for_chromecrash.py:60: projects = [Project(name,
path_regexs, function_regexs, host_directories)
On 2017/01/30 22:23:52, Sharu Jiang wrote:
> On 2017/01/30 19:14:19, wrengr wrote:
> > Since the order of the tuple/arguments is the same, I think you should be
able
> > to say ``map(Project,
> > project_classifier_config['project_path_function_hosts'])`` instead
> 
> It turned out that we cannot do that, but we can do something like:
> map(Project, *zip(*project_classifier_config['project_path_function_hosts']))
> 
> But I don't think this is very clear and readable.

yeah, that's pretty gross. oh well

[commit-bot: I haz the power, 2017-01-30 22:34:00]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1485815073464950,
"parent_rev": "f9b54cdcf8079c9e977ffff48fda1ef5f0925f9d", "commit_rev":
"c6075535547c4e038d4466680d90479388581625"}

[commit-bot: I haz the power, 2017-01-30 22:34:03]

Description was changed from

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` method to project
classifier and component classifier.

Add ``Project`` class and ``ClassifySuspect`` method to project classifier and
component classifier.
 
Also add validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
TBR=wrengr@chromium.org
==========

to

==========
[Predator] Add ``Project`` class and ``ClassifySuspect`` method to project
classifier and component classifier.

Add ``Project`` class and ``ClassifySuspect`` method to project classifier and
component classifier.
 
Also add validation for component classifier and project classifier.

N.B. Since I changed the config format of project classifier, so I included the
validations in this cl.

BUG=657177, 685448
TBR=wrengr@chromium.org

Review-Url: https://codereview.chromium.org/2657913002
Committed:
https://chromium.googlesource.com/infra/infra/+/c6075535547c4e038d4466680d904...
==========

[commit-bot: I haz the power, 2017-01-30 22:34:04]

Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/infra/infra/+/c6075535547c4e038d4466680d904...