Converting various classes to namedtuples

Converting various classes to namedtuples

Also re-removing callstack.py, since a previous CL moved it into
stacktrace.py but somehow that got rebased improperly.

BUG=661822
Committed: https://chromium.googlesource.com/infra/infra/+/9cacec9460f8fbcbb6cfe2f4bb4def59c3fc6386

[wrengr, 2016-11-18 20:37:15]

PTAL

[wrengr, 2016-11-21 19:18:59]

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

[wrengr, 2016-11-21 19:19:00]

PTAL

[chromium-reviews, 2016-11-21 22:48:46]

Somehow I only see the second publish today, reviewing :)

On Mon, Nov 21, 2016 at 11:19 AM, <wrengr@chromium.org> wrote:

> PTAL
>
> https://codereview.chromium.org/2518663002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sharu Jiang, 2016-11-21 23:31:57]

lgtm with some comments.

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
appengine/findit/crash/results.py:14: # it's necessary, given that every use in
this file discards it.
The priority is priority of callstack, this is conception from clusterfuzz, for
example for a msan crash, like in
https://cluster-fuzz.appspot.com/testcase?key=5293890036039680. the allocation
stack should have priority 0, and others have priority 1.

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
appengine/findit/crash/results.py:23: # busted. Do we actually need/want that
method? If so, to what end?
We are currently matching file changed by changelog and file crashed in
stacktrace, however the MatchResult is per changelog, say a changelog touches 3
files in the stacktrace, the ``Update`` needs to be called 3 times to update the
MatchResult for this changelog. 

So ``update`` is needed. 

Also, the reasons and confidence needs to be filled later based on the
changed_files, so I don't think this should be converted to a namedtuple.

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/stac...
File appengine/findit/crash/stacktrace.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/stac...
appengine/findit/crash/stacktrace.py:48: crashed_line_numbers, repo_url=None):
the repo_url can be changed to required argument.

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/test...
File appengine/findit/crash/test/stacktrace_test.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/test...
appengine/findit/crash/test/stacktrace_test.py:33: frame =
frame._replace(crashed_line_numbers = [9, 10])
Instead of changing fields of frame afterward, we can create new frames, since
they are namedtuples right now :)

[wrengr, 2016-11-22 18:33:41]

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
appengine/findit/crash/results.py:14: # it's necessary, given that every use in
this file discards it.
On 2016/11/21 23:31:57, Sharu Jiang wrote:
> The priority is priority of callstack, this is conception from clusterfuzz,
for
> example for a msan crash, like in
> https://cluster-fuzz.appspot.com/testcase?key=5293890036039680. the allocation
> stack should have priority 0, and others have priority 1.

That's what I was thinking. But since nothing here uses it (that I can see), why
keep it around? Why not just store the frames directly rather than making
StackInfo pairs?

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
appengine/findit/crash/results.py:23: # busted. Do we actually need/want that
method? If so, to what end?
On 2016/11/21 23:31:57, Sharu Jiang wrote:
> We are currently matching file changed by changelog and file crashed in
> stacktrace, however the MatchResult is per changelog, say a changelog touches
3
> files in the stacktrace, the ``Update`` needs to be called 3 times to update
the
> MatchResult for this changelog. 
> 
> So ``update`` is needed. 
> 
> Also, the reasons and confidence needs to be filled later based on the
> changed_files, so I don't think this should be converted to a namedtuple.

Acknowledged.

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/stac...
File appengine/findit/crash/stacktrace.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/stac...
appengine/findit/crash/stacktrace.py:48: crashed_line_numbers, repo_url=None):
On 2016/11/21 23:31:57, Sharu Jiang wrote:
> the repo_url can be changed to required argument.

That'll be a pretty invasive change, since it'll affect a lot of test files. I'd
rather do it in a separate CL.

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/test...
File appengine/findit/crash/test/stacktrace_test.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/test...
appengine/findit/crash/test/stacktrace_test.py:33: frame =
frame._replace(crashed_line_numbers = [9, 10])
On 2016/11/21 23:31:57, Sharu Jiang wrote:
> Instead of changing fields of frame afterward, we can create new frames, since
> they are namedtuples right now :)

Sure. Using ``_replace`` just means all the other fields of the new namedtuple
will be inherited form the old one; so we don't have to repeat 'func', 'f.cc',
etc

[Sharu Jiang, 2016-11-22 18:53:52]

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
File appengine/findit/crash/results.py (right):

https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
appengine/findit/crash/results.py:14: # it's necessary, given that every use in
this file discards it.
On 2016/11/22 18:33:40, wrengr wrote:
> On 2016/11/21 23:31:57, Sharu Jiang wrote:
> > The priority is priority of callstack, this is conception from clusterfuzz,
> for
> > example for a msan crash, like in
> > https://cluster-fuzz.appspot.com/testcase?key=5293890036039680. the
allocation
> > stack should have priority 0, and others have priority 1.
> 
> That's what I was thinking. But since nothing here uses it (that I can see),
why
> keep it around? Why not just store the frames directly rather than making
> StackInfo pairs?

For Fracas/Crasas, this is not needed since they only provide one callstack.
However this part of code will be shared by cracas/fracas/clusterfuzz, so I
think we can keep it.

[wrengr, 2016-11-22 19:19:44]

On 2016/11/22 18:53:52, Sharu Jiang wrote:
>
https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
> File appengine/findit/crash/results.py (right):
> 
>
https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
> appengine/findit/crash/results.py:14: # it's necessary, given that every use
in
> this file discards it.
> On 2016/11/22 18:33:40, wrengr wrote:
> > On 2016/11/21 23:31:57, Sharu Jiang wrote:
> > > The priority is priority of callstack, this is conception from
clusterfuzz,
> > for
> > > example for a msan crash, like in
> > > https://cluster-fuzz.appspot.com/testcase?key=5293890036039680. the
> allocation
> > > stack should have priority 0, and others have priority 1.
> > 
> > That's what I was thinking. But since nothing here uses it (that I can see),
> why
> > keep it around? Why not just store the frames directly rather than making
> > StackInfo pairs?
> 
> For Fracas/Crasas, this is not needed since they only provide one callstack.
> However this part of code will be shared by cracas/fracas/clusterfuzz, so I
> think we can keep it.

But what is the purpose? What will Clusterfuzz need it for? If the purpose is to
keep track of where the frame came from, then I have in mind better ways to do
that.

[Sharu Jiang, 2016-11-22 19:59:56]

On 2016/11/22 19:19:44, wrengr wrote:
> On 2016/11/22 18:53:52, Sharu Jiang wrote:
> >
>
https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
> > File appengine/findit/crash/results.py (right):
> > 
> >
>
https://codereview.chromium.org/2518663002/diff/1/appengine/findit/crash/resu...
> > appengine/findit/crash/results.py:14: # it's necessary, given that every use
> in
> > this file discards it.
> > On 2016/11/22 18:33:40, wrengr wrote:
> > > On 2016/11/21 23:31:57, Sharu Jiang wrote:
> > > > The priority is priority of callstack, this is conception from
> clusterfuzz,
> > > for
> > > > example for a msan crash, like in
> > > > https://cluster-fuzz.appspot.com/testcase?key=5293890036039680. the
> > allocation
> > > > stack should have priority 0, and others have priority 1.
> > > 
> > > That's what I was thinking. But since nothing here uses it (that I can
see),
> > why
> > > keep it around? Why not just store the frames directly rather than making
> > > StackInfo pairs?
> > 
> > For Fracas/Crasas, this is not needed since they only provide one callstack.
> > However this part of code will be shared by cracas/fracas/clusterfuzz, so I
> > think we can keep it.
> 
> But what is the purpose? What will Clusterfuzz need it for? If the purpose is
to
> keep track of where the frame came from, then I have in mind better ways to do
> that.

The purpose is this information can be useful in the scoring, for example if a
changelog changes frames in the priority 0 callstack, then it should have higher
score.

[stgao, 2016-11-22 21:22:59]

lgtm with a nit.

https://codereview.chromium.org/2518663002/diff/20001/appengine/findit/lib/gi...
File appengine/findit/lib/gitiles/change_log.py (right):

https://codereview.chromium.org/2518663002/diff/20001/appengine/findit/lib/gi...
appengine/findit/lib/gitiles/change_log.py:46: return {
IIRC, namedtuple has an equivalent func of this, but I can't find it out
immediately. Mind a check?

[wrengr, 2016-11-22 21:32:39]

https://codereview.chromium.org/2518663002/diff/20001/appengine/findit/lib/gi...
File appengine/findit/lib/gitiles/change_log.py (right):

https://codereview.chromium.org/2518663002/diff/20001/appengine/findit/lib/gi...
appengine/findit/lib/gitiles/change_log.py:46: return {
On 2016/11/22 21:22:59, stgao (slow on Monday) wrote:
> IIRC, namedtuple has an equivalent func of this, but I can't find it out
> immediately. Mind a check?

There's the ``_asdict`` method, which returns an ``OrderedDict``. (Just as one
can use ``MyNamedTuple(**some_dict)`` to implement something like ``FromDict``.)
However I'm a bit wary of using it directly because of semantic mismatch. The
goal of ``_asdict`` is to convert a namedtuple into a Python ``dict`` for
introspection purposes; whereas, the goal of ToDict is to convert the namedtuple
into JSON for sending over the wire. The names of fields in Python need not be
the same as the names of the fields in JSON, and indeed we already have cases
where they differ.

[wrengr, 2016-11-22 21:32:52]

The CQ bit was checked by wrengr@chromium.org

[wrengr, 2016-11-22 21:32:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
katesonia@chromium.org
Link to the patchset: https://codereview.chromium.org/2518663002/#ps20001
(title: "addressing nits")

[commit-bot: I haz the power, 2016-11-22 21:33:00]

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

[commit-bot: I haz the power, 2016-11-22 21:48:16]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479850372262060,
"parent_rev": "e5d7862c9beb05650a4405574c2cb28f33e87a3a", "commit_rev":
"9cacec9460f8fbcbb6cfe2f4bb4def59c3fc6386"}

[commit-bot: I haz the power, 2016-11-22 21:48:23]

Description was changed from

==========
Converting various classes to namedtuples

Also re-removing callstack.py, since a previous CL moved it into
stacktrace.py but somehow that got rebased improperly.

BUG=661822
==========

to

==========
Converting various classes to namedtuples

Also re-removing callstack.py, since a previous CL moved it into
stacktrace.py but somehow that got rebased improperly.

BUG=661822

Committed:
https://chromium.googlesource.com/infra/infra/+/9cacec9460f8fbcbb6cfe2f4bb4de...
==========

[commit-bot: I haz the power, 2016-11-22 21:48:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/infra/infra/+/9cacec9460f8fbcbb6cfe2f4bb4de...

[stgao, 2016-11-22 21:50:40]

https://codereview.chromium.org/2518663002/diff/20001/appengine/findit/lib/gi...
File appengine/findit/lib/gitiles/change_log.py (right):

https://codereview.chromium.org/2518663002/diff/20001/appengine/findit/lib/gi...
appengine/findit/lib/gitiles/change_log.py:46: return {
On 2016/11/22 21:32:39, wrengr wrote:
> On 2016/11/22 21:22:59, stgao (slow on Monday) wrote:
> > IIRC, namedtuple has an equivalent func of this, but I can't find it out
> > immediately. Mind a check?
> 
> There's the ``_asdict`` method, which returns an ``OrderedDict``. (Just as one
> can use ``MyNamedTuple(**some_dict)`` to implement something like
``FromDict``.)
> However I'm a bit wary of using it directly because of semantic mismatch. The
> goal of ``_asdict`` is to convert a namedtuple into a Python ``dict`` for
> introspection purposes; whereas, the goal of ToDict is to convert the
namedtuple
> into JSON for sending over the wire. The names of fields in Python need not be
> the same as the names of the fields in JSON, and indeed we already have cases
> where they differ.

Good point. Sgtm!