rebaseline_server: add pixel diffs, and sorting by diff metrics

rebaseline_server: add pixel diffs, and sorting by diff metrics
(SkipBuildbotRuns)

R=bsalomon@google.com, jcgregorio@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12193

[epoger, 2013-11-06 17:58:38]

Brian (or any volunteer)- please review the UI

Joe (or any volunteer)- please review the code

Ready for review at patchset 2, and running for your examination at:
http://172.24.179.183:9133

[jcgregorio, 2013-11-06 18:47:27]

Nice working on keeping the view.html clean!

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:18: from cStringIO import StringIO
unused import

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:29: 
2 lines

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:57:
mkdir_unless_exists(os.path.join(storage_root, IMAGES_SUBDIR))
Repeated code 58-69. Can you create a function load_image() which does the path
calculation, downloading and Image.open()ing?

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:136: TODO(epoger): Make this asynchronously
download images, rather than blocking
I don't know if the async belongs in at this level, maybe just make ImageDiffDB
thread-safe and create a thread-pool/worker queue at a higher level that just
uses ImageDiffDB?

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:155: if not self._diff_dict.get(key):
if not key in self._diff_dict:

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:195: def generate_image_diff(image1,
image2):
Document args and returns, here and below.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:222: file_handle.write(url_handle.read())
Consider shutil.copyfileobj for this:

http://docs.python.org/2/library/shutil.html#shutil.copyfileobj

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:232: logging.basicConfig(level=logging.INFO)
Break tests out into a separate imagediffdb_test.py, then remove svn:executable
bit from this file.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:262: 
2 lines

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
File gm/rebaseline_server/results.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:270: if ((not expected_hashtype) or (not
expected_hashdigest) or
are you checking for None's?

if None in [expected_hashtype, expected_hashdigest, actual_hashtype,
actual_hashdigest]:

might be clearer.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:381: test=test, expected_image=expected_image,
self._generate_diffs_if_needed(test, expected_image, actual_image)

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:408: except KeyError:
log the error?

[rmistry, 2013-11-06 19:12:51]

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:10: Requires PIL to be installed; see
http://www.pythonware.com/products/pil/
Optional: You could also output this line by wrapping 'from PIL import Image,
ImageChops' in a try/except.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:13: # System-level imports
Nit: Not required since you do not have any other imports.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:210: def mkdir_unless_exists(path):
Make it private? this and some of the other top level functions here.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/serv...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:160: 'Parsing results from actuals in %s and
expectations in %s, and generating pixel diffs (may take a while) ...' % (
nit: line > 80

[epoger, 2013-11-07 21:11:53]

Thanks, guys!  Please see patchset 3.

Updated server is running for your perusal at http://172.24.179.183:9133/

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:10: Requires PIL to be installed; see
http://www.pythonware.com/products/pil/
On 2013/11/06 19:12:52, rmistry wrote:
> Optional: You could also output this line by wrapping 'from PIL import Image,
> ImageChops' in a try/except.

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:13: # System-level imports
On 2013/11/06 19:12:52, rmistry wrote:
> Nit: Not required since you do not have any other imports.

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:18: from cStringIO import StringIO
On 2013/11/06 18:47:28, jcgregorio wrote:
> unused import

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:29: 
On 2013/11/06 18:47:28, jcgregorio wrote:
> 2 lines

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:57:
mkdir_unless_exists(os.path.join(storage_root, IMAGES_SUBDIR))
On 2013/11/06 18:47:28, jcgregorio wrote:
> Repeated code 58-69. Can you create a function load_image() which does the
path
> calculation, downloading and Image.open()ing?

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:136: TODO(epoger): Make this asynchronously
download images, rather than blocking
On 2013/11/06 18:47:28, jcgregorio wrote:
> I don't know if the async belongs in at this level, maybe just make
ImageDiffDB
> thread-safe and create a thread-pool/worker queue at a higher level that just
> uses ImageDiffDB?

Added to TODO, thanks.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:155: if not self._diff_dict.get(key):
On 2013/11/06 18:47:28, jcgregorio wrote:
> if not key in self._diff_dict:

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:195: def generate_image_diff(image1,
image2):
On 2013/11/06 18:47:28, jcgregorio wrote:
> Document args and returns, here and below.

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:210: def mkdir_unless_exists(path):
On 2013/11/06 19:12:52, rmistry wrote:
> Make it private? this and some of the other top level functions here.

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:222: file_handle.write(url_handle.read())
On 2013/11/06 18:47:28, jcgregorio wrote:
> Consider shutil.copyfileobj for this:
> 
> http://docs.python.org/2/library/shutil.html#shutil.copyfileobj

Thanks, good advice!  See
http://stackoverflow.com/questions/8220669/python-saving-large-web-page-to-file

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:232: logging.basicConfig(level=logging.INFO)
On 2013/11/06 18:47:28, jcgregorio wrote:
> Break tests out into a separate imagediffdb_test.py, then remove
svn:executable
> bit from this file.

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/imag...
gm/rebaseline_server/imagediffdb.py:262: 
On 2013/11/06 18:47:28, jcgregorio wrote:
> 2 lines

Done (in the new test file)

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
File gm/rebaseline_server/results.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:270: if ((not expected_hashtype) or (not
expected_hashdigest) or
On 2013/11/06 18:47:28, jcgregorio wrote:
> are you checking for None's?
> 
> if None in [expected_hashtype, expected_hashdigest, actual_hashtype,
> actual_hashdigest]:
> 
> might be clearer.

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:381: test=test, expected_image=expected_image,
On 2013/11/06 18:47:28, jcgregorio wrote:
> self._generate_diffs_if_needed(test, expected_image, actual_image)

I've become a fan of named arguments (I like that if someone changes/reorders
the function arguments later, the callsites don't have to change).

The extra verbosity is disappointing, but in Python's world where there is no
compile-time checking, these safeguards help me sleep easier.

Is there a way we can better balance those two conflicting desires?

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:408: except KeyError:
On 2013/11/06 18:47:28, jcgregorio wrote:
> log the error?

Done.

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/serv...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:160: 'Parsing results from actuals in %s and
expectations in %s, and generating pixel diffs (may take a while) ...' % (
On 2013/11/06 19:12:52, rmistry wrote:
> nit: line > 80

Done.

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagediffdb-test.py (right):

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb-test.py:9: Test imagediffdb.py
Moved the self-test code into this new file.

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:163: logging.exception('got exception while
creating new DiffRecord')
As I was testing these changes, I started getting exceptions because a single
image expectation (
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d...
) is broken.

Added exception handling so the server keeps going, but informs the user.  (Plus
it sticks out like a sore thumb in the UI)

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/res...
File gm/rebaseline_server/results.py (right):

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/res...
gm/rebaseline_server/results.py:403:
results_for_this_test['percentDifferingPixels'] = 0
There is no diff record in the nocomparison or succeeded case, but the succeeded
case should definitely show up as having 0 pixel diffs!

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/view.html (right):

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:313: <div
ng-hide="result.expectedHashDigest == result.actualHashDigest">
Improved display of diff column cells for succeeded tests.

[bsalomon, 2013-11-07 21:26:10]

On 2013/11/06 17:58:38, epoger wrote:
> Brian (or any volunteer)- please review the UI
> 
ui lgtm

[jcgregorio, 2013-11-08 02:02:29]

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
File gm/rebaseline_server/results.py (right):

https://codereview.chromium.org/59283006/diff/30001/gm/rebaseline_server/resu...
gm/rebaseline_server/results.py:381: test=test, expected_image=expected_image,
NP, if you're willing to do the typing :-)

On 2013/11/07 21:11:53, epoger wrote:
> On 2013/11/06 18:47:28, jcgregorio wrote:
> > self._generate_diffs_if_needed(test, expected_image, actual_image)
> 
> I've become a fan of named arguments (I like that if someone changes/reorders
> the function arguments later, the callsites don't have to change).
> 
> The extra verbosity is disappointing, but in Python's world where there is no
> compile-time checking, these safeguards help me sleep easier.
> 
> Is there a way we can better balance those two conflicting desires?

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagediffdb-test.py (right):

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb-test.py:9: Test imagediffdb.py
Please rename to imagediffdb_test.py. A file imagediffdb-test.py isn't
importable as a module, something we may want to do when we get more unit tests.
See below on using Python's unittest module.


On 2013/11/07 21:11:54, epoger wrote:
> Moved the self-test code into this new file.

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb-test.py:47: for selftest in selftests:
Either convert tests to use Python's unittest framework, or add a TODO to that
effect.

[epoger, 2013-11-08 16:11:27]

Please see patchset 4.

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagediffdb-test.py (right):

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb-test.py:9: Test imagediffdb.py
On 2013/11/08 02:02:29, jcgregorio wrote:
> Please rename to imagediffdb_test.py. A file imagediffdb-test.py isn't
> importable as a module, something we may want to do when we get more unit
tests.
> See below on using Python's unittest module.
> 
> 
> On 2013/11/07 21:11:54, epoger wrote:
> > Moved the self-test code into this new file.
> 

Done.

https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb-test.py:47: for selftest in selftests:
On 2013/11/08 02:02:29, jcgregorio wrote:
> Either convert tests to use Python's unittest framework, or add a TODO to that
> effect.

Added a TODO at the top of the renamed file.

[jcgregorio, 2013-11-08 16:24:14]

On 2013/11/08 16:11:27, epoger wrote:
> Please see patchset 4.
> 
>
https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
> File gm/rebaseline_server/imagediffdb-test.py (right):
> 
>
https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
> gm/rebaseline_server/imagediffdb-test.py:9: Test imagediffdb.py
> On 2013/11/08 02:02:29, jcgregorio wrote:
> > Please rename to imagediffdb_test.py. A file imagediffdb-test.py isn't
> > importable as a module, something we may want to do when we get more unit
> tests.
> > See below on using Python's unittest module.
> > 
> > 
> > On 2013/11/07 21:11:54, epoger wrote:
> > > Moved the self-test code into this new file.
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/59283006/diff/170001/gm/rebaseline_server/ima...
> gm/rebaseline_server/imagediffdb-test.py:47: for selftest in selftests:
> On 2013/11/08 02:02:29, jcgregorio wrote:
> > Either convert tests to use Python's unittest framework, or add a TODO to
that
> > effect.
> 
> Added a TODO at the top of the renamed file.

LGTM

[epoger, 2013-11-08 16:25:30]

Committed patchset #4 manually as r12193 (presubmit successful).