Local/Remote benchmarking tool #810

lissyx · 2017-09-01T19:11:17Z

Fixes #684

lissyx · 2017-09-06T10:23:11Z

Tooling ready, with TaskCluster execution: https://public-artifacts.taskcluster.net/HTRYR3AhQfy9qMm_-K8Zvw/0/public/benchmark.png

Sadly, it's going to be too much tricky to be able to test remote SSH from TaskCluster.

lissyx · 2017-09-06T13:18:05Z

FTR, basic RPi3 password-based SSH auth with host defined in ~/.ssh/config gets as:

python bin/benchmark_nc.py [...] --target MozRPi3-ARMv6-Paris --autotrust --no-allowagent --no-lookforkeys [...]

lissyx · 2017-09-06T18:47:01Z

Looks good so far.

reuben

I have a bunch of nits and comments, mostly about Python 2/3 compatibility, but one larger comment is that I think you should split benchmark_nc (what's "nc"?) into two separate scripts, one for the benchmark and one for plotting.

I wish we didn't have to do all the SSH stuff, but I can see how it makes life easier for benchmarking on a RPi.

Oh, and remember to remove the "hack" commit and fix the commit messages.

reuben · 2017-09-07T09:28:00Z

.taskcluster.yml

@@ -327,6 +327,7 @@ tasks:
            export TASKCLUSTER_TASK_DIR="$(find $(dirname `pwd`) -name "task-*" -type d -mindepth 1 -maxdepth 1)" &&
            git clone --quiet {{event.head.repo.url}} ${TASKCLUSTER_TASK_DIR}/DeepSpeech/ds/ &&
            cd ${TASKCLUSTER_TASK_DIR}/DeepSpeech/ds && git checkout --quiet {{event.head.sha}} &&
+            patch -d ${TASKCLUSTER_TASK_DIR}/DeepSpeech/tf -p1 < brew.patch &&


Can't we just fix this in mozilla/tensorflow?

That's already taken care of by mozilla/tensorflow#26 and #820 :)

reuben · 2017-09-07T09:33:17Z

tc-benchmark-tests.sh

+# We still need to get model, wav and alphabet
+download_data
+
+# Follow benchmark naming from parameters in bin/run-tc-ldc93s1.sh


I don't understand this comment. What benchmark naming? Which parameters in run-tc-ldc93s1.sh?

Model names are being used to extract dimensions informations. Clearly, your comments later shows it's not made clear enough :)

reuben · 2017-09-07T09:33:43Z

tc-benchmark-tests.sh

+
+# Follow benchmark naming from parameters in bin/run-tc-ldc93s1.sh
+# Okay, it's not really the real LSTM sizes, just a way to verify how things
+# actually behaves.


nit: behave.

reuben · 2017-09-07T09:35:04Z

tc-benchmark-tests.sh

+done;
+
+# Let's prepare another model for single-model codepath
+mv /tmp/${model_name} /tmp/test.frozen.e75.lstm494.ldc93s1.pb


Maybe name it lstmdefault instead of lstm494, to avoid tying this code to the n_hidden value from bin/run-tc-ldc93s1.sh?

Because the 494 dimension will be used when plotting later :)

reuben · 2017-09-07T09:39:21Z

tc-benchmark-tests.sh

+        --dataset "TaskCluster model" ${csv} \
+        --title "TaskCluster model benchmark" \
+        --plot ${png} \
+        --size 1280x720


It's confusing that the same script behaves as two entirely different programs here. My suggestion would be to split it in two and change them to read/write from/to stdin/stdout, but I haven't reviewed benchmark_nc.py yet, so maybe I'm missing something.

reuben · 2017-09-08T13:33:35Z

util/tc.py

@@ -0,0 +1,47 @@
+#!/usr/bin/env python
+from __future__ import print_function
+from __future__ import absolute_import


nit: combine these lines.
Import division here as well, so you don't need to explicitly type hint divisions below.

reuben · 2017-09-08T13:35:42Z

util/tc.py

+import sys
+import os
+import stat
+import urllib


This needs to be six.moves.urllib for Python 2 and 3 compat.

Pydoc says there is no urlretrieve in six.moves.urllib :/

reuben · 2017-09-08T13:36:26Z

util/tc.py

+    target_file = os.path.join(target_dir, tc_filename)
+    if not os.path.isfile(target_file):
+        print('Downloading %s ...' % tc_url)
+        urllib.urlretrieve(tc_url, target_file, reporthook=(report_progress if progress else None))


six.moves.urllib mimics the Python 3 structure, so this needs to be urllib.request.urlretrieve.

Answers above then :)

reuben · 2017-09-08T13:37:41Z

util/tc.py

+
+def maybe_download_tc(target_dir=None, tc_url=None, progress=True):
+    def report_progress(count, block_size, total_size):
+        percent = int((count * block_size * 100) / total_size)


Don't need the int() here with __future__.division, just use //.

reuben · 2017-09-08T13:37:56Z

util/tc.py

+                               'https://index.taskcluster.net/v1/task/project.deepspeech.deepspeech.native_client.master.%(arch_string)s/artifacts/public/native_client.tar.xz')
+
+def get_tc_url(arch_string=None):
+


nit: too much whitespace.

lissyx · 2017-09-08T13:45:19Z

About splitting, I thought about that, but I felt it would not really help that much and might complexify things later.

lissyx · 2017-09-11T12:55:19Z

@reuben Previous push with the default arguments cleanup was good, current push is running and it includes splitting into two scripts :)

reuben

r=me with those two comments addressed.

reuben · 2017-09-11T14:07:53Z

bin/benchmark_nc.py

+
+    sftp = ssh_conn.open_sftp()
+    if not stat.S_ISDIR(sftp.stat(dir).st_mode):
+        print('No remote directory: %s' % dir)


It's still missing from this call.

reuben · 2017-09-11T14:10:33Z

util/benchmark.py

+    '''
+    fs = ''
+    for c in s:
+        if ord(c) >= ord('0') and ord(c) <= ord('9'):


Please use c.isdigit().

Both are done :)

Fixes mozilla#684

lissyx · 2017-09-11T15:07:12Z

It's all green except on OSX because a lot of other TaskCluster (big) tasks are pending there. I'm merging anyway, since benchmark code is not exercized there :)

lock · 2019-01-03T08:05:49Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lissyx self-assigned this Sep 1, 2017

lissyx force-pushed the benchmark-tool branch 8 times, most recently from 44919a4 to b46526f Compare September 5, 2017 15:51

lissyx mentioned this pull request Sep 5, 2017

Score CTC prefix beams with KenLM #805

Merged

lissyx force-pushed the benchmark-tool branch 8 times, most recently from db7c656 to 4e113b5 Compare September 6, 2017 09:31

lissyx force-pushed the benchmark-tool branch 4 times, most recently from 12f6cbe to 3855be3 Compare September 6, 2017 13:11

lissyx force-pushed the benchmark-tool branch 6 times, most recently from fb3e52f to 8cca1bd Compare September 6, 2017 17:10

lissyx requested a review from reuben September 6, 2017 18:47

lissyx mentioned this pull request Sep 7, 2017

Issue818+819 mozilla/tensorflow#26

Merged

reuben suggested changes Sep 8, 2017

View reviewed changes

lissyx force-pushed the benchmark-tool branch 9 times, most recently from bcd066e to 5bc32e9 Compare September 11, 2017 12:53

reuben approved these changes Sep 11, 2017

View reviewed changes

Local/Remote benchmarking tool

ccecc62

Fixes mozilla#684

lissyx force-pushed the benchmark-tool branch from 5bc32e9 to ccecc62 Compare September 11, 2017 14:28

lissyx merged commit b1229c0 into mozilla:master Sep 11, 2017

lissyx deleted the benchmark-tool branch September 11, 2017 15:07

lock bot locked and limited conversation to collaborators Jan 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local/Remote benchmarking tool #810

Local/Remote benchmarking tool #810

lissyx commented Sep 1, 2017

lissyx commented Sep 6, 2017

lissyx commented Sep 6, 2017

lissyx commented Sep 6, 2017

reuben left a comment

reuben Sep 7, 2017

lissyx Sep 8, 2017

reuben Sep 7, 2017

lissyx Sep 8, 2017

reuben Sep 7, 2017

lissyx Sep 8, 2017

reuben Sep 7, 2017

lissyx Sep 8, 2017

reuben Sep 7, 2017

reuben Sep 8, 2017

lissyx Sep 8, 2017

reuben Sep 8, 2017

lissyx Sep 8, 2017

lissyx Sep 8, 2017

reuben Sep 8, 2017

lissyx Sep 8, 2017

reuben Sep 8, 2017

lissyx Sep 8, 2017

reuben Sep 8, 2017

lissyx Sep 8, 2017

lissyx commented Sep 8, 2017

lissyx commented Sep 11, 2017

reuben left a comment

reuben Sep 11, 2017

reuben Sep 11, 2017

lissyx Sep 11, 2017

lissyx commented Sep 11, 2017

lock bot commented Jan 3, 2019

		'https://index.taskcluster.net/v1/task/project.deepspeech.deepspeech.native_client.master.%(arch_string)s/artifacts/public/native_client.tar.xz')

		def get_tc_url(arch_string=None):

Local/Remote benchmarking tool #810

Local/Remote benchmarking tool #810

Conversation

lissyx commented Sep 1, 2017

lissyx commented Sep 6, 2017

lissyx commented Sep 6, 2017

lissyx commented Sep 6, 2017

reuben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lissyx commented Sep 8, 2017

lissyx commented Sep 11, 2017

reuben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lissyx commented Sep 11, 2017

lock bot commented Jan 3, 2019