rebaseline_server: create ImagePairSet-- holds a number of ImagePairs to examine

rebaseline_server: create ImagePairSet-- holds a number of ImagePairs to examine

See https://goto.google.com/ChangingRbsJson and bug 1919 for additional context

BUG=skia:1919
NOTRY=True

Committed: http://code.google.com/p/skia/source/detail?r=13434

[epoger, 2014-02-11 06:15:40]

Ready for review at patchset 1.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepair.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepair.py:16: KEY__EXPECTATIONS_DATA = 'expectations'
I started using two underscores to delineate between "this is a dictionary key"
and "which field does it refer to" (since there are already single underscores
within the "which field" part)

[rmistry, 2014-02-12 19:23:55]

Apologies for all the nits but since we decided to use the style guide I am
nitier than usual.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column.py
File gm/rebaseline_server/column.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:22: Assembles the header for a single column of
data.
Nit:

From the python style guide:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

Snippet:
A doc string should be organized as a summary line (one physical line)
terminated by a period, question mark, or exclamation point, followed by a blank
line, followed by the rest of the doc string starting at the same cursor
position as the first quote of the first line.

My example:

"""one line description.

More doc string.
Even more doc string.
"""

We have not been doing this in Skia Infra, we should start. I frequently see
things like:
"""one line
same line continued."""

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:30: header_text: string; text the client should
display within column header
Nit:
Missing ending period. Some lines in this args section have periods and other do
not.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:47: """
Nit:

See above comment. Maybe do:
""" Creates the header in dictionary form.

 Creates the header for this column in dictionary form, as needed when
constructing the JSON representation.  Uses the KEY__* constants as keys.
...
"""

This makes it more readable when looking at generated pydocs.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepair.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepair.py:16: KEY__EXPECTATIONS_DATA = 'expectations'
On 2014/02/11 06:15:40, epoger wrote:
> I started using two underscores to delineate between "this is a dictionary
key"
> and "which field does it refer to" (since there are already single underscores
> within the "which field" part)

Sorry I do not follow, single underscores within which "which field" part?

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:23: """ A collection of ImagePairs,
representing two arbitrary sets of images we
Nit: Single line description please.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:35: descriptions: a (string, string) tuple
describing the two image sets
Should we mention here what is used if descriptions is not provided?

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:70: header for a particular extraColumn.
Nit: Single line please

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:77: return
self._column_header_factories[column_id]
How about something like this instead:

if not self._column_header_factories.has_key(column_id):
  self._column_header_factories[column_id] =
column.ColumnHeaderFactory(header_text=column_id)
return self._column_header_factories[column_id]

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:85: We use this information to generate
tallies within the column header
Nit: newline between the single line description and the larger one please.

[epoger, 2014-02-12 19:59:15]

PTAL.  Actually, I appreciate you calling these deviations to my attention.  I
endeavor to be less of a deviant.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column.py
File gm/rebaseline_server/column.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:22: Assembles the header for a single column of
data.
On 2014/02/12 19:23:56, rmistry wrote:
> Nit:
> 
> From the python style guide:
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments
> 
> Snippet:
> A doc string should be organized as a summary line (one physical line)
> terminated by a period, question mark, or exclamation point, followed by a
blank
> line, followed by the rest of the doc string starting at the same cursor
> position as the first quote of the first line.
> 
> My example:
> 
> """one line description.
> 
> More doc string.
> Even more doc string.
> """
> 
> We have not been doing this in Skia Infra, we should start. I frequently see
> things like:
> """one line
> same line continued."""

Done throughout.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:30: header_text: string; text the client should
display within column header
On 2014/02/12 19:23:56, rmistry wrote:
> Nit:
> Missing ending period. Some lines in this args section have periods and other
do
> not.

Done.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:47: """
On 2014/02/12 19:23:56, rmistry wrote:
> Nit:
> 
> See above comment. Maybe do:
> """ Creates the header in dictionary form.
> 
>  Creates the header for this column in dictionary form, as needed when
> constructing the JSON representation.  Uses the KEY__* constants as keys.
> ...
> """
> 
> This makes it more readable when looking at generated pydocs.

Done.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepair.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepair.py:16: KEY__EXPECTATIONS_DATA = 'expectations'
On 2014/02/12 19:23:56, rmistry wrote:
> On 2014/02/11 06:15:40, epoger wrote:
> > I started using two underscores to delineate between "this is a dictionary
> key"
> > and "which field does it refer to" (since there are already single
underscores
> > within the "which field" part)
> 
> Sorry I do not follow, single underscores within which "which field" part?

I mean, so it's clear that this should be parsed as ['KEY',
'EXPECTATIONS_DATA'], not ['KEY', 'EXPECTATIONS', 'DATA'].

This is more important in imagepairset,py, where I have
KEY__IMAGESETS__BASE_URL, which should be parsed as ['KEY', 'IMAGESETS',
'BASE_URL']   (BASE_URL key within IMAGESETS dict)

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:23: """ A collection of ImagePairs,
representing two arbitrary sets of images we
On 2014/02/12 19:23:56, rmistry wrote:
> Nit: Single line description please.

Done.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:35: descriptions: a (string, string) tuple
describing the two image sets
On 2014/02/12 19:23:56, rmistry wrote:
> Should we mention here what is used if descriptions is not provided?

Done.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:35: descriptions: a (string, string) tuple
describing the two image sets
On 2014/02/12 19:23:56, rmistry wrote:
> Should we mention here what is used if descriptions is not provided?

Done.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:70: header for a particular extraColumn.
On 2014/02/12 19:23:56, rmistry wrote:
> Nit: Single line please

Done.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:77: return
self._column_header_factories[column_id]
On 2014/02/12 19:23:56, rmistry wrote:
> How about something like this instead:
> 
> if not self._column_header_factories.has_key(column_id):
>   self._column_header_factories[column_id] =
> column.ColumnHeaderFactory(header_text=column_id)
> return self._column_header_factories[column_id]

I can do that if you like, but I figured the existing pattern might be more
resilient against race conditions.  (For example, if another thread removed this
column_id from the dictionary AFTER the has_key() check but BEFORE the return, a
KeyError would be raised.)

But I'm sure there are plenty of other parts of this code that are subject to
race conditions anyway.

Do you think there is a significant performance penalty associated with the
exception handling?  If so, I could do it this way:

column_header_factory = self._column_header_factories.get(column_id, None)
if not column_header_factory:
  column_header_factory = column.ColumnHeaderFactory(header_text=column_id)
  self._column_header_factories[column_id] = column_header_factory
return column_header_factory


Actually, that's one line shorter than what I have now.  I'll do that, unless
you disapprove.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:85: We use this information to generate
tallies within the column header
On 2014/02/12 19:23:56, rmistry wrote:
> Nit: newline between the single line description and the larger one please.

Done.

[rmistry, 2014-02-13 12:10:02]

LGTM

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepair.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepair.py:16: KEY__EXPECTATIONS_DATA = 'expectations'
On 2014/02/12 19:59:16, epoger wrote:
> On 2014/02/12 19:23:56, rmistry wrote:
> > On 2014/02/11 06:15:40, epoger wrote:
> > > I started using two underscores to delineate between "this is a dictionary
> > key"
> > > and "which field does it refer to" (since there are already single
> underscores
> > > within the "which field" part)
> > 
> > Sorry I do not follow, single underscores within which "which field" part?
> 
> I mean, so it's clear that this should be parsed as ['KEY',
> 'EXPECTATIONS_DATA'], not ['KEY', 'EXPECTATIONS', 'DATA'].
> 
> This is more important in imagepairset,py, where I have
> KEY__IMAGESETS__BASE_URL, which should be parsed as ['KEY', 'IMAGESETS',
> 'BASE_URL']   (BASE_URL key within IMAGESETS dict)

Thanks for the explanation.

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/139343018/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset.py:77: return
self._column_header_factories[column_id]
On 2014/02/12 19:59:16, epoger wrote:
> On 2014/02/12 19:23:56, rmistry wrote:
> > How about something like this instead:
> > 
> > if not self._column_header_factories.has_key(column_id):
> >   self._column_header_factories[column_id] =
> > column.ColumnHeaderFactory(header_text=column_id)
> > return self._column_header_factories[column_id]
> 
> I can do that if you like, but I figured the existing pattern might be more
> resilient against race conditions.  (For example, if another thread removed
this
> column_id from the dictionary AFTER the has_key() check but BEFORE the return,
a
> KeyError would be raised.)
> 
> But I'm sure there are plenty of other parts of this code that are subject to
> race conditions anyway.
> 
> Do you think there is a significant performance penalty associated with the
> exception handling?  If so, I could do it this way:

No I do not think there is significant performance penalty associated with the
exception handling. I recommended the other block just because it was one line
shorter and your new block is one line shorter so that LGTM.

> 
> column_header_factory = self._column_header_factories.get(column_id, None)
> if not column_header_factory:
>   column_header_factory = column.ColumnHeaderFactory(header_text=column_id)
>   self._column_header_factories[column_id] = column_header_factory
> return column_header_factory
> 
> 
> Actually, that's one line shorter than what I have now.  I'll do that, unless
> you disapprove.

https://codereview.chromium.org/139343018/diff/80001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/139343018/diff/80001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagepairset.py:46: """Adds an ImagePair; this may be
repeated any number of times.
Another style comment (last one I promise). I have typically seen comments like
this when they are used in a single line:

"""One line comment here."""

I have not seen much of:

"""One line comment here.
"""

(It makes the file one line larger).

I found http://www.python.org/dev/peps/pep-0257/#one-line-docstrings that is
referred to in the style guide, looks like they are using:
"""One line comment."""

[epoger, 2014-02-13 17:16:31]

https://codereview.chromium.org/139343018/diff/80001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagepairset.py (right):

https://codereview.chromium.org/139343018/diff/80001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagepairset.py:46: """Adds an ImagePair; this may be
repeated any number of times.
On 2014/02/13 12:10:02, rmistry wrote:
> Another style comment (last one I promise). I have typically seen comments
like
> this when they are used in a single line:
> 
> """One line comment here."""
> 
> I have not seen much of:
> 
> """One line comment here.
> """
> 
> (It makes the file one line larger).
> 
> I found http://www.python.org/dev/peps/pep-0257/#one-line-docstrings that is
> referred to in the style guide, looks like they are using:
> """One line comment."""

Done throughout.  Makes sense.

[epoger, 2014-02-13 17:16:36]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-02-13 17:16:42]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/epoger@google.com/139343018/200001

[commit-bot: I haz the power, 2014-02-13 17:17:09]

Change committed as 13434