Implementing a new LogLinearModel-based CL classifier

Implementing a new LogLinearModel-based CL classifier

This CL redesigns the old ChangelistClassifier to a new one based on loglinear models

BUG=
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2560723005
Committed: https://chromium.googlesource.com/infra/infra/+/8e31edb8f2f5d295657748a033d3bbe1fc03008e

[wrengr, 2016-12-14 20:21:56]

Description was changed from

==========
[WIP] Implementing a new LogLinearModel-based CL classifier

This CL is still in progress, so not ready for review.

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

to

==========
Implementing a new LogLinearModel-based CL classifier

This CL redesigns the old ChangelistClassifier to a new one based on loglinear
models

BUG=
TBR=stgao.chromium.org
==========

[wrengr, 2016-12-14 20:21:56]

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

[wrengr, 2016-12-14 20:22:31]

Got the new LLM-based CL classifier to pass all tests etc. PTAL

[Sharu Jiang, 2016-12-15 20:11:33]

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_classifier.py (right):

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:21: def __init__(self,
repository, weights, top_n_frames=7, top_n_results=3):
We can pass the already trained model in, and let this model take care of weight
training and features.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:26: for. We take this
argument as a dict rather than as a list so rhat
typo: rhat -> that

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:72: it to the classes
defining loglinear models.
I think this is very hacky, we should move it to loglinear model class.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:82: def __call__(self,
report):
we have some duplicate code with ChangeLogClassifier here, can we define a base
case? so they can share those code. 

Or, we can keep the ChangeLogClassifier general and pass whatever model, and let
the model take care of various scoring methods.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:132:
changelist_classifier.GetChangeLogsForFilesGroupedByDeps(
Now both changelist_classifier and loglinear changelist classifier use these
functions, we'd better factor them into a common module.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:135:
changelist_classifier.GetStackInfosForFilesGroupedByDeps(
Ditto.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:139: results =
changelist_classifier.FindMatchResults(
Ditto.

[wrengr, 2016-12-15 21:35:08]

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_classifier.py (right):

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:21: def __init__(self,
repository, weights, top_n_frames=7, top_n_results=3):
On 2016/12/15 20:11:32, Sharu Jiang wrote:
> We can pass the already trained model in, and let this model take care of
weight
> training and features.

Sure, but someone has to construct that model. For now I think it's easiest to
have the classifier construct it. If in the future it would be helpful to
explicitly construct the model first, we can make the change then

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:26: for. We take this
argument as a dict rather than as a list so rhat
On 2016/12/15 20:11:32, Sharu Jiang wrote:
> typo: rhat -> that

Done.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:72: it to the classes
defining loglinear models.
On 2016/12/15 20:11:32, Sharu Jiang wrote:
> I think this is very hacky, we should move it to loglinear model class.

I agree. Since it'll be fairly substantial, will do it in a separate CL

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:82: def __call__(self,
report):
On 2016/12/15 20:11:32, Sharu Jiang wrote:
> we have some duplicate code with ChangeLogClassifier here, can we define a
base
> case? so they can share those code. 

Yeah, there's a lot of duplication between the LLM-based classifier and the
scorer-based classifier. I think for now that's okay. Once we do some delta
tests to verify that things are working as expected, when we can (a) get rid of
the obsolete scorer-based stuff, or (b) refactor to share code if we really want
to keep both versions around.

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:132:
changelist_classifier.GetChangeLogsForFilesGroupedByDeps(
On 2016/12/15 20:11:32, Sharu Jiang wrote:
> Now both changelist_classifier and loglinear changelist classifier use these
> functions, we'd better factor them into a common module.

Agree, but ditto what I said above.

[Sharu Jiang, 2016-12-16 19:23:20]

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_classifier.py (right):

https://codereview.chromium.org/2560723005/diff/200001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:82: def __call__(self,
report):
On 2016/12/15 21:35:07, wrengr wrote:
> On 2016/12/15 20:11:32, Sharu Jiang wrote:
> > we have some duplicate code with ChangeLogClassifier here, can we define a
> base
> > case? so they can share those code. 
> 
> Yeah, there's a lot of duplication between the LLM-based classifier and the
> scorer-based classifier. I think for now that's okay. Once we do some delta
> tests to verify that things are working as expected, when we can (a) get rid
of
> the obsolete scorer-based stuff, or (b) refactor to share code if we really
want
> to keep both versions around.

We may try out other models in the further even though we get rid of the
obsolete scorer-based stuff, so separating changelist classifier with the model
would be a good way to solve this. :)

[wrengr, 2016-12-16 19:38:17]

On 2016/12/16 19:23:20, Sharu Jiang wrote:
> We may try out other models in the further even though we get rid of the
> obsolete scorer-based stuff, so separating changelist classifier with the
model
> would be a good way to solve this. :)

...Except that this implementation of the CL classifier relies on the API of
LLMs, so wouldn't work for other "models". We might could generalize
ChangelistClassifier to handle other ways of doing things, but I'd like to see a
more concrete proposal about the sorts of things it actually needs to support,
so we don't waste time overengineering it to handle possibilities we never
actually use/care about.

[wrengr, 2016-12-16 19:40:44]

On 2016/12/16 19:38:17, wrengr wrote:
> On 2016/12/16 19:23:20, Sharu Jiang wrote:
> > We may try out other models in the further even though we get rid of the
> > obsolete scorer-based stuff, so separating changelist classifier with the
> model
> > would be a good way to solve this. :)
> 
> ...Except that this implementation of the CL classifier relies on the API of
> LLMs, so wouldn't work for other "models". We might could generalize
> ChangelistClassifier to handle other ways of doing things, but I'd like to see
a
> more concrete proposal about the sorts of things it actually needs to support,
> so we don't waste time overengineering it to handle possibilities we never
> actually use/care about.

Also, what the ChangelistClassifier class is doing isn't really independent of
the model used to score things. Most of the code in the class is doing
preprocessing to compute things particular features care about. Really, that
code should be moved to the features themselves, in order to reduce coupling
between the ChangelistClassifier class and the particular model or set of
features it's using.

[Martin Barbella, 2016-12-16 23:28:37]

As a general note, I think TODOs are overused in this CL, but I don't have a
particularly strong opinion on it. LGTM with a question.

https://codereview.chromium.org/2560723005/diff/260001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_classifier.py (right):

https://codereview.chromium.org/2560723005/diff/260001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:50: #
TODO(crbug.com/673964): something better for detecting "close to log(0)".
I'm not sure that a very clean solution to this would ever be possible without
over-engineering this. I'm not really suggesting changing what we're doing here
(I would imagine this is good enough in most cases), just want to point it out
if any future changes are being planned.

https://codereview.chromium.org/2560723005/diff/260001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:230: assert
accumulated_changed_file.blame_url == changed_file.blame_url, (
Is this based on something from the existing code? I'm a bit curious about why
we have this check.

[wrengr, 2016-12-17 00:24:54]

Re todos, would you prefer I remove the crbug references from the code?

https://codereview.chromium.org/2560723005/diff/260001/appengine/findit/crash...
File appengine/findit/crash/loglinear/changelist_classifier.py (right):

https://codereview.chromium.org/2560723005/diff/260001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:50: #
TODO(crbug.com/673964): something better for detecting "close to log(0)".
On 2016/12/16 23:28:37, Martin Barbella wrote:
> I'm not sure that a very clean solution to this would ever be possible without
> over-engineering this. I'm not really suggesting changing what we're doing
here
> (I would imagine this is good enough in most cases), just want to point it out
> if any future changes are being planned.

Yeah, it's not clear there's anything much better than this. The cleanest thing
would be to have some score/probability cutoff, and to count everything under
that threshold as bad. (I think I mentioned that in the bug report?) Of course,
a score-based threshold would be dependent on the scores themselves, so not
stable under rescaling the weights (e.g., multiplying all weights by 2). But
computing probabilities is rather expensive if this is the only thing we want
them for...

https://codereview.chromium.org/2560723005/diff/260001/appengine/findit/crash...
appengine/findit/crash/loglinear/changelist_classifier.py:230: assert
accumulated_changed_file.blame_url == changed_file.blame_url, (
On 2016/12/16 23:28:37, Martin Barbella wrote:
> Is this based on something from the existing code? I'm a bit curious about why
> we have this check.

The scorer-based ChangelistClassifier has a sort of silent bug here. The
relevant code is in aggregators.py. The old code just grabs the first file name
an url and assumes everything else matches, with a note about how this is
buggy/a limitation. Here I generalize that code so it works appropriately no
matter how many files we have nor how they're arranged. So far at least, we have
no idea what it would mean for a file to have more than one blame_url; so
there's no clear way to "combine" them. AFAICT, the assertion should never be
triggered; but if it is, that's a warning that we need to figure out what to do
with files that have more than one blame_url.

Since it's an assumption we're making, I'd like things to fail loudly if we ever
actually violate it.

[Sharu Jiang, 2016-12-18 22:39:09]

lgtm

[wrengr, 2016-12-19 22:24:45]

The CQ bit was checked by wrengr@chromium.org

[wrengr, 2016-12-19 22:24:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, katesonia@chromium.org
Link to the patchset: https://codereview.chromium.org/2560723005/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-19 22:24:51]

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

[wrengr, 2016-12-19 22:25:01]

Description was changed from

==========
Implementing a new LogLinearModel-based CL classifier

This CL redesigns the old ChangelistClassifier to a new one based on loglinear
models

BUG=
TBR=stgao.chromium.org
==========

to

==========
Implementing a new LogLinearModel-based CL classifier

This CL redesigns the old ChangelistClassifier to a new one based on loglinear
models

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

[commit-bot: I haz the power, 2016-12-19 22:33:39]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1482186285403410,
"parent_rev": "0dc1789058ea0aa395270d32b7bceca58b72ae1b", "commit_rev":
"8e31edb8f2f5d295657748a033d3bbe1fc03008e"}

[commit-bot: I haz the power, 2016-12-19 22:33:43]

Description was changed from

==========
Implementing a new LogLinearModel-based CL classifier

This CL redesigns the old ChangelistClassifier to a new one based on loglinear
models

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

to

==========
Implementing a new LogLinearModel-based CL classifier

This CL redesigns the old ChangelistClassifier to a new one based on loglinear
models

BUG=
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2560723005
Committed:
https://chromium.googlesource.com/infra/infra/+/8e31edb8f2f5d295657748a033d3b...
==========

[commit-bot: I haz the power, 2016-12-19 22:33:44]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/infra/infra/+/8e31edb8f2f5d295657748a033d3b...