Python event log analyzer tool: fix of indexing issue.

Python event log analyzer tool: fix of indexing issue.

Test added to ensure it doesn't happen again.

NOTRY=True

Committed: https://crrev.com/ac398f2a4c2c8f15b3d66e3cc9722ad5a03c0a69
Cr-Commit-Position: refs/heads/master@{#14121}

[aleloi, 2016-09-06 11:08:10]

aleloi@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, phoglund@webrtc.org

[aleloi, 2016-09-06 11:08:11]

Please take a look!

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right):

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:274: grouped_delays = [[] for
_ in numpy.arange(start, stop + step, step)]
Silly indexing bug wasn't noticed because of insufficient testing.

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:55: unittest.main()
If I rename this file to end with '_test', the presubmit test will pick it up
and run it every time. This is a problem, because this test depends on numpy,
which is not a webrtc  dependency. Most probably, we don't want to make it a
webrtc dependency either. I couldn't think of anything better than leaving it as
is.

[phoglund, 2016-09-06 11:43:05]

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
Remove blank line.

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:55: unittest.main()
On 2016/09/06 11:08:11, aleloi wrote:
> If I rename this file to end with '_test', the presubmit test will pick it up
> and run it every time. This is a problem, because this test depends on numpy,
> which is not a webrtc  dependency. Most probably, we don't want to make it a
> webrtc dependency either. I couldn't think of anything better than leaving it
as
> is.

Umm, why not add numpy as a dependency? Your tool is using it, and your
production code can import it, so the test should be able to as well. How does
your production code gain access to numpy?

[aleloi, 2016-09-06 11:51:57]

On 2016/09/06 11:43:05, phoglund wrote:
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):
> 
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
> Remove blank line.
> 
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:55: unittest.main()
> On 2016/09/06 11:08:11, aleloi wrote:
> > If I rename this file to end with '_test', the presubmit test will pick it
up
> > and run it every time. This is a problem, because this test depends on
numpy,
> > which is not a webrtc  dependency. Most probably, we don't want to make it a
> > webrtc dependency either. I couldn't think of anything better than leaving
it
> as
> > is.
> 
> Umm, why not add numpy as a dependency? Your tool is using it, and your
> production code can import it, so the test should be able to as well. How does
> your production code gain access to numpy?

Currently you mostly use the tool like this: 
1. developer tries to use this tool
2. the tool crashes
3. they ask me and I show how to install numpy and matplotlib

... but I'd be happy to make it simpler!

[phoglund, 2016-09-06 11:51:59]

I looked a bit into how Chromium does this, and it's generally sys.path
manipulations. They do it in __init__.py files (which run automatically when you
import something from that directory), so if you had such an init file for your
production code, it would automatically make numpy available for the test as
well I believe. It should work even if the test imports numpy before the
production code since the test and production code are in the same dir (in which
case __init__.py runs before the test is even loaded). For an example in
Chromium, see src/tools/perf/core/__init__.py.

[phoglund, 2016-09-06 11:55:33]

On 2016/09/06 11:51:57, aleloi wrote:
> On 2016/09/06 11:43:05, phoglund wrote:
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):
> > 
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
> > Remove blank line.
> > 
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:55: unittest.main()
> > On 2016/09/06 11:08:11, aleloi wrote:
> > > If I rename this file to end with '_test', the presubmit test will pick it
> up
> > > and run it every time. This is a problem, because this test depends on
> numpy,
> > > which is not a webrtc  dependency. Most probably, we don't want to make it
a
> > > webrtc dependency either. I couldn't think of anything better than leaving
> it
> > as
> > > is.
> > 
> > Umm, why not add numpy as a dependency? Your tool is using it, and your
> > production code can import it, so the test should be able to as well. How
does
> > your production code gain access to numpy?
> 
> Currently you mostly use the tool like this: 
> 1. developer tries to use this tool
> 2. the tool crashes
> 3. they ask me and I show how to install numpy and matplotlib
> 
> ... but I'd be happy to make it simpler!

Oh, you install them into your python. All right. Another option could be
pulling down the numpy and matplotlib using DEPS and use sys.path manipulation
to add them to the pythonpath. Then again, I suspect matplotlib cannot simply be
downloaded but needs to ./configure && make, and that's hard to solve. In that
case maybe this low-fi approach is all right. You can try to import numpy and
matlab early in your main and give a more informative message though (something
like "you don't have numpy, run sudo apt-get install python-numpy" or whatever
is the right thing.

[phoglund, 2016-09-06 11:57:55]

On 2016/09/06 11:55:33, phoglund wrote:
> On 2016/09/06 11:51:57, aleloi wrote:
> > On 2016/09/06 11:43:05, phoglund wrote:
> > >
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > > File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > > webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
> > > Remove blank line.
> > > 
> > >
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > > webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:55: unittest.main()
> > > On 2016/09/06 11:08:11, aleloi wrote:
> > > > If I rename this file to end with '_test', the presubmit test will pick
it
> > up
> > > > and run it every time. This is a problem, because this test depends on
> > numpy,
> > > > which is not a webrtc  dependency. Most probably, we don't want to make
it
> a
> > > > webrtc dependency either. I couldn't think of anything better than
leaving
> > it
> > > as
> > > > is.
> > > 
> > > Umm, why not add numpy as a dependency? Your tool is using it, and your
> > > production code can import it, so the test should be able to as well. How
> does
> > > your production code gain access to numpy?
> > 
> > Currently you mostly use the tool like this: 
> > 1. developer tries to use this tool
> > 2. the tool crashes
> > 3. they ask me and I show how to install numpy and matplotlib
> > 
> > ... but I'd be happy to make it simpler!
> 
> Oh, you install them into your python. All right. Another option could be
> pulling down the numpy and matplotlib using DEPS and use sys.path manipulation
> to add them to the pythonpath. Then again, I suspect matplotlib cannot simply
be
> downloaded but needs to ./configure && make, and that's hard to solve. In that
> case maybe this low-fi approach is all right. You can try to import numpy and
> matlab early in your main and give a more informative message though
(something
> like "you don't have numpy, run sudo apt-get install python-numpy" or whatever
> is the right thing.

In that case, maybe catch the import error in the test and just skip it if deps
are missing.

Like

missing_numpy = False
try:
  import numpy
except ImportError:
  missing_numpy = True

...

if __name__ == '__main__':
  if missing_numpy:
    print 'Missing numpy, skipping test...'
    sys.exit(0)
  unittest.main()

[aleloi, 2016-09-06 11:59:05]

On 2016/09/06 11:51:59, phoglund wrote:
> I looked a bit into how Chromium does this, and it's generally sys.path
> manipulations. They do it in __init__.py files (which run automatically when
you
> import something from that directory), so if you had such an init file for
your
> production code, it would automatically make numpy available for the test as
> well I believe. It should work even if the test imports numpy before the
> production code since the test and production code are in the same dir (in
which
> case __init__.py runs before the test is even loaded). For an example in
> Chromium, see src/tools/perf/core/__init__.py.

The problem is that this tool relies on some python packages being installed
system-wide. One solution is to add the dependencies to webrtc and load it in
__init__, but as I understood it, that would require lots of extra maintenance
work. Weighting extra work to convenience of use, I lean towards thinking that
the current way the tool is used is quite fine. There hasn't been any complaints
yet:)

[phoglund, 2016-09-06 12:07:55]

On 2016/09/06 11:59:05, aleloi wrote:
> On 2016/09/06 11:51:59, phoglund wrote:
> > I looked a bit into how Chromium does this, and it's generally sys.path
> > manipulations. They do it in __init__.py files (which run automatically when
> you
> > import something from that directory), so if you had such an init file for
> your
> > production code, it would automatically make numpy available for the test as
> > well I believe. It should work even if the test imports numpy before the
> > production code since the test and production code are in the same dir (in
> which
> > case __init__.py runs before the test is even loaded). For an example in
> > Chromium, see src/tools/perf/core/__init__.py.
> 
> The problem is that this tool relies on some python packages being installed
> system-wide. One solution is to add the dependencies to webrtc and load it in
> __init__, but as I understood it, that would require lots of extra maintenance
> work. Weighting extra work to convenience of use, I lean towards thinking that
> the current way the tool is used is quite fine. There hasn't been any
complaints
> yet:)

OK. Do consider the solution above for the test though, it will have little to
no value
if people need to run it by hand. The above will run it only when numpy is
available.

[aleloi, 2016-09-06 12:15:30]

On 2016/09/06 11:57:55, phoglund wrote:
> On 2016/09/06 11:55:33, phoglund wrote:
> > On 2016/09/06 11:51:57, aleloi wrote:
> > > On 2016/09/06 11:43:05, phoglund wrote:
> > > >
> > >
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > > > File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > > > webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
> > > > Remove blank line.
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> > > > webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:55:
unittest.main()
> > > > On 2016/09/06 11:08:11, aleloi wrote:
> > > > > If I rename this file to end with '_test', the presubmit test will
pick
> it
> > > up
> > > > > and run it every time. This is a problem, because this test depends on
> > > numpy,
> > > > > which is not a webrtc  dependency. Most probably, we don't want to
make
> it
> > a
> > > > > webrtc dependency either. I couldn't think of anything better than
> leaving
> > > it
> > > > as
> > > > > is.
> > > > 
> > > > Umm, why not add numpy as a dependency? Your tool is using it, and your
> > > > production code can import it, so the test should be able to as well.
How
> > does
> > > > your production code gain access to numpy?
> > > 
> > > Currently you mostly use the tool like this: 
> > > 1. developer tries to use this tool
> > > 2. the tool crashes
> > > 3. they ask me and I show how to install numpy and matplotlib
> > > 
> > > ... but I'd be happy to make it simpler!
> > 
> > Oh, you install them into your python. All right. Another option could be
> > pulling down the numpy and matplotlib using DEPS and use sys.path
manipulation
> > to add them to the pythonpath. Then again, I suspect matplotlib cannot
simply
> be
> > downloaded but needs to ./configure && make, and that's hard to solve. In
that
> > case maybe this low-fi approach is all right. You can try to import numpy
and
> > matlab early in your main and give a more informative message though
> (something
> > like "you don't have numpy, run sudo apt-get install python-numpy" or
whatever
> > is the right thing.
> 
> In that case, maybe catch the import error in the test and just skip it if
deps
> are missing.
> 
> Like
> 
> missing_numpy = False
> try:
>   import numpy
> except ImportError:
>   missing_numpy = True
> 
> ...
> 
> if __name__ == '__main__':
>   if missing_numpy:
>     print 'Missing numpy, skipping test...'
>     sys.exit(0)
>   unittest.main()

Good idea! Will do!

[aleloi, 2016-09-06 12:15:37]

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):

https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
On 2016/09/06 11:43:05, phoglund wrote:
> Remove blank line.

Ok! But gpylint complains if there is no blank line. git cl presubmit is fine
with it, though.

Is the rule mostly consistency with other code, or is there a policy somewhere?

[phoglund, 2016-09-06 13:05:33]

On 2016/09/06 12:15:37, aleloi wrote:
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> File webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py (right):
> 
>
https://codereview.webrtc.org/2316573003/diff/1/webrtc/tools/py_event_log_ana...
> webrtc/tools/py_event_log_analyzer/rtp_analyzertest.py:28: 
> On 2016/09/06 11:43:05, phoglund wrote:
> > Remove blank line.
> 
> Ok! But gpylint complains if there is no blank line. git cl presubmit is fine
> with it, though.
> 
> Is the rule mostly consistency with other code, or is there a policy
somewhere?

gpylint is for google3 I believe. Obey git cl presubmit. The rule of thumb is to
be consistent with adjacent code.

[hlundin-webrtc, 2016-09-07 14:23:19]

Rubberstamp LGTM, with one nit. And pending phoglund's approval.

https://codereview.webrtc.org/2316573003/diff/20001/webrtc/tools/py_event_log...
File webrtc/tools/py_event_log_analyzer/misc_test.py (left):

https://codereview.webrtc.org/2316573003/diff/20001/webrtc/tools/py_event_log...
webrtc/tools/py_event_log_analyzer/misc_test.py:25: 
This is an unnecessary change in this CL.

[aleloi, 2016-09-07 14:28:52]

https://codereview.webrtc.org/2316573003/diff/20001/webrtc/tools/py_event_log...
File webrtc/tools/py_event_log_analyzer/misc_test.py (left):

https://codereview.webrtc.org/2316573003/diff/20001/webrtc/tools/py_event_log...
webrtc/tools/py_event_log_analyzer/misc_test.py:25: 
On 2016/09/07 14:23:19, hlundin-webrtc wrote:
> This is an unnecessary change in this CL.

As phoglund@ pointed out, our python style is no spaces between class name
header and first def. I fixed it in TestDelay, and wanted to fix it here as
well.

[hlundin-webrtc, 2016-09-07 14:38:38]

https://codereview.webrtc.org/2316573003/diff/20001/webrtc/tools/py_event_log...
File webrtc/tools/py_event_log_analyzer/misc_test.py (left):

https://codereview.webrtc.org/2316573003/diff/20001/webrtc/tools/py_event_log...
webrtc/tools/py_event_log_analyzer/misc_test.py:25: 
On 2016/09/07 14:28:52, aleloi wrote:
> On 2016/09/07 14:23:19, hlundin-webrtc wrote:
> > This is an unnecessary change in this CL.
> 
> As phoglund@ pointed out, our python style is no spaces between class name
> header and first def. I fixed it in TestDelay, and wanted to fix it here as
> well.

Fair enough. It just felt odd to include one more file in the CL to make this
orthogonal change. But keep it now.

[phoglund, 2016-09-07 15:20:28]

lgtm

[aleloi, 2016-09-07 15:23:23]

The CQ bit was checked by aleloi@webrtc.org

[commit-bot: I haz the power, 2016-09-07 15:23:27]

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

[commit-bot: I haz the power, 2016-09-07 15:33:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 15:33:11]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8133)

[aleloi, 2016-09-08 08:28:27]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-09-08 08:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, phoglund@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2316573003/#ps40001 (title:
"Presubmit complaint.")

[commit-bot: I haz the power, 2016-09-08 08:28:31]

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

[commit-bot: I haz the power, 2016-09-08 08:30:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-08 08:30:39]

Description was changed from

==========
Python event log analyzer tool: fix of indexing issue.

Test added to ensure it doesn't happen again.

NOTRY=True
==========

to

==========
Python event log analyzer tool: fix of indexing issue.

Test added to ensure it doesn't happen again.

NOTRY=True

Committed: https://crrev.com/ac398f2a4c2c8f15b3d66e3cc9722ad5a03c0a69
Cr-Commit-Position: refs/heads/master@{#14121}
==========

[commit-bot: I haz the power, 2016-09-08 08:30:40]

Patchset 3 (id:??) landed as
https://crrev.com/ac398f2a4c2c8f15b3d66e3cc9722ad5a03c0a69
Cr-Commit-Position: refs/heads/master@{#14121}