Issue 1914113002: [Findit] Enable project classifier and component classifier

Issue 1914113002: [Findit] Enable project classifier and component classifier (Closed)

Created:
4 years, 8 months ago by Sharu Jiang

Modified:
4 years, 7 months ago

Reviewers:
stgao, mbarbella (wrong one), Martin Barbella

CC:
chromium-reviews, infra-reviews+infra_chromium.org, aarya, mmandlis

Base URL:
https://chromium.googlesource.com/infra/infra.git@master

Target Ref:
refs/heads/master

Project:
infra

Visibility:
Public.

More Reviews

Description

[Findit] Enable project classifier and component classifier Enable the project classifier and component classifier. Set the classifier configuration in CrashConfig model from config page. BUG=605355 Committed: https://chromium.googlesource.com/infra/infra/+/bf6da84ffe7d2bf4029b7286af17cb2aede16a4d

Patch Set 1 : #

Total comments: 20

Patch Set 2 : Address comments and add tests. #

Total comments: 10

Patch Set 3 : Address comments #

Patch Set 4 : Add classifiers to fracas.py #

Total comments: 4

Patch Set 5 : Address comments. #

Total comments: 38

Patch Set 6 : Rebase and address comments. #

Total comments: 7

Patch Set 7 : Address comments. #

Total comments: 2

Patch Set 8 : Rebase and address comments. #

Total comments: 4

Patch Set 9 : Rebase #

Created: 4 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+991 lines, -105 lines)			Patch
M	appengine/findit/crash/callstack.py	View	1 2 3 4 5	5 chunks	+14 lines, -14 lines	0 comments	Download
A	appengine/findit/crash/classifier.py	View	1 2 3 4 5	1 chunk	+99 lines, -0 lines	0 comments	Download
A	appengine/findit/crash/component_classifier.py	View	1 2 3 4 5 6 7 8	1 chunk	+68 lines, -0 lines	0 comments	Download
M	appengine/findit/crash/fracas.py	View	1 2 3 4 5	2 chunks	+7 lines, -3 lines	0 comments	Download
M	appengine/findit/crash/fracas_parser.py	View	1 2 3 4 5	2 chunks	+4 lines, -7 lines	0 comments	Download
A	appengine/findit/crash/project_classifier.py	View	1 2 3 4 5 6 7 8	1 chunk	+101 lines, -0 lines	0 comments	Download
M	appengine/findit/crash/results.py	View	1 2 3 4 5 6 7 8	3 chunks	+4 lines, -8 lines	0 comments	Download
M	appengine/findit/crash/scorers/test/aggregator_test.py	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	appengine/findit/crash/test/callstack_filters_test.py	View	1 2 3 4 5	2 chunks	+9 lines, -7 lines	0 comments	Download
M	appengine/findit/crash/test/callstack_test.py	View	1 2 3 4 5	4 chunks	+9 lines, -7 lines	0 comments	Download
A	appengine/findit/crash/test/classifier_test.py	View	1 2 3 4 5	1 chunk	+77 lines, -0 lines	0 comments	Download
A	appengine/findit/crash/test/component_classifier_test.py	View	1 2 3 4 5 6 7 8	1 chunk	+90 lines, -0 lines	0 comments	Download
M	appengine/findit/crash/test/crash_test_suite.py	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	appengine/findit/crash/test/crash_testcase.py	View	1 2 3 4 5 6	3 chunks	+60 lines, -1 line	0 comments	Download
M	appengine/findit/crash/test/findit_for_crash_test.py	View	1 2 3 4 5 6 7 8	9 chunks	+17 lines, -18 lines	0 comments	Download
M	appengine/findit/crash/test/fracas_crash_pipeline_test.py	View	1 2 3 4 5	1 chunk	+0 lines, -3 lines	0 comments	Download
M	appengine/findit/crash/test/fracas_parser_test.py	View	1 2 3 4 5	4 chunks	+13 lines, -11 lines	0 comments	Download
M	appengine/findit/crash/test/fracas_test.py	View	1 2 3	2 chunks	+4 lines, -1 line	0 comments	Download
A	appengine/findit/crash/test/project_classifier_test.py	View	1 2 3 4 5 6 7 8	1 chunk	+184 lines, -0 lines	0 comments	Download
M	appengine/findit/crash/test/results_test.py	View	1 2 3 4 5 6 7 8	4 chunks	+14 lines, -12 lines	0 comments	Download
M	appengine/findit/crash/test/stacktrace_test.py	View	1 2 3 4 5 6 7 8	3 chunks	+12 lines, -9 lines	0 comments	Download
M	appengine/findit/crash/test/stacktrace_test_suite.py	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	appengine/findit/handlers/crash/crash_config.py	View	1 2 3 4 5 6	2 chunks	+7 lines, -1 line	0 comments	Download
M	appengine/findit/model/crash/crash_config.py	View	1 2 3 4 5 6 7 8	3 chunks	+85 lines, -0 lines	0 comments	Download
A	appengine/findit/model/crash/test/crash_config_test.py	View	1 2 3 4 5 6 7	1 chunk	+98 lines, -0 lines	0 comments	Download
M	appengine/findit/templates/crash/crash_config.html	View	1	4 chunks	+14 lines, -0 lines	0 comments	Download

Messages

Total messages: 44 (24 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Sharu Jiang

Functional code done, however not 100% tests coverage yet, since I switched to more high ...

4 years, 7 months ago (2016-05-05 17:54:50 UTC) #9

Martin Barbella

Some initial comments. Will take a closer look at the next patch set, but it's ...

4 years, 7 months ago (2016-05-05 18:33:41 UTC) #12

Sharu Jiang

PTAL :) https://codereview.chromium.org/1914113002/diff/100001/appengine/findit/common/constants.py File appengine/findit/common/constants.py (right): https://codereview.chromium.org/1914113002/diff/100001/appengine/findit/common/constants.py#newcode35 appengine/findit/common/constants.py:35: INFINITY = 1000 On 2016/05/05 18:33:41, Martin ...

4 years, 7 months ago (2016-05-10 03:11:03 UTC) #14

Martin Barbella

https://codereview.chromium.org/1914113002/diff/100001/appengine/findit/crash/classifier.py File appengine/findit/crash/classifier.py (right): https://codereview.chromium.org/1914113002/diff/100001/appengine/findit/crash/classifier.py#newcode25 appengine/findit/crash/classifier.py:25: def DefaultRankFunction(class_info): On 2016/05/10 03:11:02, sharu jiang wrote: > ...

4 years, 7 months ago (2016-05-10 06:00:41 UTC) #20

Sharu Jiang

sorry for the delay. PTAL :) https://codereview.chromium.org/1914113002/diff/200001/appengine/findit/common/constants.py File appengine/findit/common/constants.py (right): https://codereview.chromium.org/1914113002/diff/200001/appengine/findit/common/constants.py#newcode35 appengine/findit/common/constants.py:35: INFINITY = 1000 ...

4 years, 7 months ago (2016-05-12 23:58:00 UTC) #21

Martin Barbella

LGTM with nits. https://codereview.chromium.org/1914113002/diff/260001/appengine/findit/crash/project_classifier.py File appengine/findit/crash/project_classifier.py (right): https://codereview.chromium.org/1914113002/diff/260001/appengine/findit/crash/project_classifier.py#newcode29 appengine/findit/crash/project_classifier.py:29: """Returns the component name of a ...

4 years, 7 months ago (2016-05-13 23:05:07 UTC) #23

Sharu Jiang

4 years, 7 months ago (2016-05-17 19:27:54 UTC) #24

stgao

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/callstack.py File appengine/findit/crash/callstack.py (right): https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/callstack.py#newcode35 appengine/findit/crash/callstack.py:35: raw_file_path (str): Normalized original path of the crashed file. ...

4 years, 7 months ago (2016-05-17 21:40:58 UTC) #25

Sharu Jiang

sorry for the bigger cl, since I did a rebase and have to resolve some ...

4 years, 7 months ago (2016-05-20 23:16:33 UTC) #27

stgao

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/component_classifier.py File appengine/findit/crash/component_classifier.py (right): https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/component_classifier.py#newcode26 appengine/findit/crash/component_classifier.py:26: for path, function, component in config['path_function_component']: On 2016/05/20 23:16:33, ...

4 years, 7 months ago (2016-05-21 00:54:03 UTC) #30

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

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:26: for path, function, component
in config['path_function_component']:
On 2016/05/20 23:16:33, sharu jiang wrote:
> On 2016/05/17 21:40:57, stgao wrote:
> > Could this be cached in the CrashConfig instance?
> > 
> > This will be executed for each analyzed crash. The overall overhead might be
> > high.
> 
> I do a CrashConfig.Get() for every crash, so I don't think cache it in the
> instance would help, or you mean the instance in datastore? then yes, I
changed
> this to a BlobProperty there:), but I still need to decompress and unpickle
for
> every crashes, but the overhead shouldn't be big I think. 

On appengine, NDB has an app-instance cache for NDB data, before it reaches to
memcache or NDB.
A non-transactional Model.get() is not necessarily to retrieve data from NDB.

> 
> Another option is to cache compiled settings as a class variable of
CrashConfig
> class, and every time we click save button, it will update this class
variable,
> but this would cause race condition, and I don't think it's very necessary.
> 

I don't quite understand how this will work.

To be more specific,  my suggestion is something like this (cache in an
instance):

@property
def processed_data(self):
   if not self.cached_data:
      # build the pattern and cache it.
      self.cached_data = ...
   return self.cached_data

This is an optimization to avoid overhead of computing the pattern, and also
save memory for string operations. We have a lot of OOMs recently.

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

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash...
appengine/findit/crash/project_classifier.py:36: for host_directory in
self.config['host_directories']:
On 2016/05/20 23:16:33, sharu jiang wrote:
> On 2016/05/17 21:40:57, stgao wrote:
> > should the host directories be sorted like the one in ClusterFuzz?
> 
> The order of host_directories is exactly the same as in clusterfuzz.

Are they always sorted in the config?

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:28: re.compile("src/comp1.*"),
This is an interesting idea. If they are compiled and saved in NDB, how should
we display them on the config page?

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:79: compressed_component_classifier
= ndb.BlobProperty(indexed=False,
For compress, an alternative is:
ndb.JsonProperty(compressed=True)

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:101: for path, function, component
in component_config.get(
This is a little tricky, especially the code for the encoding is in
handlers/crash/crash_config.py

If this is the way to compress and avoid recompiling, is it possible to put the
two piece of code in the same place?

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:101: for path, function, component
in component_config.get(
Personally, it'd avoid this complexity.

I left a comment on how to cache computed pattern in the CrashConfig instance in
original code.

Sharu Jiang

4 years, 7 months ago (2016-05-23 23:54:50 UTC) #31

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

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash...
appengine/findit/crash/component_classifier.py:26: for path, function, component
in config['path_function_component']:
On 2016/05/21 00:54:03, stgao wrote:
> On 2016/05/20 23:16:33, sharu jiang wrote:
> > On 2016/05/17 21:40:57, stgao wrote:
> > > Could this be cached in the CrashConfig instance?
> > > 
> > > This will be executed for each analyzed crash. The overall overhead might
be
> > > high.
> > 
> > I do a CrashConfig.Get() for every crash, so I don't think cache it in the
> > instance would help, or you mean the instance in datastore? then yes, I
> changed
> > this to a BlobProperty there:), but I still need to decompress and unpickle
> for
> > every crashes, but the overhead shouldn't be big I think. 
> 
> On appengine, NDB has an app-instance cache for NDB data, before it reaches to
> memcache or NDB.
> A non-transactional Model.get() is not necessarily to retrieve data from NDB.
> 
> > 
> > Another option is to cache compiled settings as a class variable of
> CrashConfig
> > class, and every time we click save button, it will update this class
> variable,
> > but this would cause race condition, and I don't think it's very necessary.
> > 
> 
> I don't quite understand how this will work.
> 
> To be more specific,  my suggestion is something like this (cache in an
> instance):
> 
> @property
> def processed_data(self):
>    if not self.cached_data:
>       # build the pattern and cache it.
>       self.cached_data = ...
>    return self.cached_data
> 
> This is an optimization to avoid overhead of computing the pattern, and also
> save memory for string operations. We have a lot of OOMs recently.

Done.

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

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash...
appengine/findit/crash/project_classifier.py:36: for host_directory in
self.config['host_directories']:
On 2016/05/21 00:54:03, stgao wrote:
> On 2016/05/20 23:16:33, sharu jiang wrote:
> > On 2016/05/17 21:40:57, stgao wrote:
> > > should the host directories be sorted like the one in ClusterFuzz?
> > 
> > The order of host_directories is exactly the same as in clusterfuzz.
> 
> Are they always sorted in the config?
I am a little confused, they are always in this order in clusterfuzz, not
sorted:
    'host_directories': [
        'src/chrome/browser/resources/',
        'src/chrome/test/data/layout_tests/',
        'src/media/',
        'src/sdch/',
        'src/testing/',
        'src/third_party/WebKit/',
        'src/third_party/',
        'src/tools/',
        'src/'
    ]

So I just keep the same order here.

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:28: re.compile("src/comp1.*"),
On 2016/05/21 00:54:03, stgao wrote:
> This is an interesting idea. If they are compiled and saved in NDB, how should
> we display them on the config page?

I think we should still display them as strings and just do the compiling in the
background.

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:79: compressed_component_classifier
= ndb.BlobProperty(indexed=False,
On 2016/05/21 00:54:03, stgao wrote:
> For compress, an alternative is:
> ndb.JsonProperty(compressed=True)

I found out there is a PickleProperty...

https://codereview.chromium.org/1914113002/diff/360001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:101: for path, function, component
in component_config.get(
On 2016/05/21 00:54:03, stgao wrote:
> This is a little tricky, especially the code for the encoding is in
> handlers/crash/crash_config.py
> 
> If this is the way to compress and avoid recompiling, is it possible to put
the
> two piece of code in the same place?

I can move them to the same place, but it's not necessary now :)

stgao

lgtm once comments are addressed. https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/project_classifier.py File appengine/findit/crash/project_classifier.py (right): https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/project_classifier.py#newcode36 appengine/findit/crash/project_classifier.py:36: for host_directory in self.config['host_directories']: ...

4 years, 7 months ago (2016-05-24 00:10:58 UTC) #32

Sharu Jiang

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/project_classifier.py File appengine/findit/crash/project_classifier.py (right): https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash/project_classifier.py#newcode36 appengine/findit/crash/project_classifier.py:36: for host_directory in self.config['host_directories']: On 2016/05/24 00:10:58, stgao wrote: ...

4 years, 7 months ago (2016-05-24 22:22:44 UTC) #33

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

https://codereview.chromium.org/1914113002/diff/280001/appengine/findit/crash...
appengine/findit/crash/project_classifier.py:36: for host_directory in
self.config['host_directories']:
On 2016/05/24 00:10:58, stgao wrote:
> On 2016/05/23 23:54:50, sharu jiang wrote:
> > On 2016/05/21 00:54:03, stgao wrote:
> > > On 2016/05/20 23:16:33, sharu jiang wrote:
> > > > On 2016/05/17 21:40:57, stgao wrote:
> > > > > should the host directories be sorted like the one in ClusterFuzz?
> > > > 
> > > > The order of host_directories is exactly the same as in clusterfuzz.
> > > 
> > > Are they always sorted in the config?
> > I am a little confused, they are always in this order in clusterfuzz, not
> > sorted:
> >     'host_directories': [
> >         'src/chrome/browser/resources/',
> >         'src/chrome/test/data/layout_tests/',
> >         'src/media/',
> >         'src/sdch/',
> >         'src/testing/',
> >         'src/third_party/WebKit/',
> >         'src/third_party/',
> >         'src/tools/',
> >         'src/'
> >     ]
> > 
> > So I just keep the same order here.
> 
> Yes, they are in order in clusterfuzz, but they are not necessarily in order
in
> the Findit config page, because it is updated manually.
> We can not assume the admin will keep the list in order manually before
updating
> it.

get it, that makes sense.

https://codereview.chromium.org/1914113002/diff/380001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1914113002/diff/380001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:82: cached_component_classifier =
ndb.PickleProperty(indexed=False, default=None,
On 2016/05/24 00:10:58, stgao wrote:
> Do we plan to save this in NDB? If not, let's not make it a NDB property.
> 
> It is set in "def compiled_component_classifier" below, but never saved to NDB
> through a put.

Done.

stgao

lgtm https://codereview.chromium.org/1914113002/diff/420001/appengine/findit/crash/project_classifier.py File appengine/findit/crash/project_classifier.py (right): https://codereview.chromium.org/1914113002/diff/420001/appengine/findit/crash/project_classifier.py#newcode22 appengine/findit/crash/project_classifier.py:22: cmp=self._CompareHosts) This could be simplified by just comparing ...

4 years, 7 months ago (2016-05-24 22:28:48 UTC) #35

Sharu Jiang

https://codereview.chromium.org/1914113002/diff/420001/appengine/findit/crash/project_classifier.py File appengine/findit/crash/project_classifier.py (right): https://codereview.chromium.org/1914113002/diff/420001/appengine/findit/crash/project_classifier.py#newcode22 appengine/findit/crash/project_classifier.py:22: cmp=self._CompareHosts) On 2016/05/24 22:28:48, stgao wrote: > This could ...

4 years, 7 months ago (2016-05-24 23:30:25 UTC) #36

stgao

4 years, 7 months ago (2016-05-24 23:35:36 UTC) #37

Sharu Jiang

4 years, 7 months ago (2016-05-25 00:08:07 UTC) #38

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-25 01:13:30 UTC) #41

commit-bot: I haz the power

4 years, 7 months ago (2016-05-25 01:21:26 UTC) #43

Message was sent while issue was closed.

Committed patchset #10 (id:460001) as
https://chromium.googlesource.com/infra/infra/+/bf6da84ffe7d2bf4029b7286af17c...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages