teach rebaseline_server to generate diffs of rendered SKPs

teach rebaseline_server to generate diffs of rendered SKPs

Creates a new live-view.html page, served by the rebaseline_server, that will display diffs between two sets of rendered SKP images.

BUG=skia:1942
NOTRY=true

Committed: https://skia.googlesource.com/skia/+/0b7127635d8245de7ac704080d722d06e47621d0

[epoger, 2014-07-31 15:18:31]

This is NOT ready for review yet... I'll let you know when it is.

As of patchset 1, this can successfully generate diffs between 2 production SKP
runs!  Sort of.


Here's how I am able to run it:

1. Copy the RenderSKP results down to local disk (/tmp/skp-actuals/linux and
/tmp/skp-actuals/mac) and trim those files WAY down so they don't take a long
time to load.

2. launch the server:
gm/rebaseline_server/server.py --export --builders '.*Mac10.6.*' --boto
~/.boto-REAL
(the --builders bit is just so it doesn't take a long time to launch,
pregenerating GM diffs I'm not looking at right now)

3. view this page in my browser:
http://172.24.179.183:8888/static/live-view.html#/live-view.html?&actualsDir=...



Major items that I still need to address:

- Allow caller to specify actualsDir/expectationsDir as gs:// URLs rather than
local filepaths, so the server can go fetch production results to compare

- Pixel diffs display properly in the web page, but the expected/actual images
show up as broken links (my browser is not allowed to access the images even
though I am logged in with google.com credentials, not sure how to fix that)

- See how terribly long it takes to generate diffs between two full production
SKP runs.  It will probably take a long time.  I will probably need to
parallelize the fetching/diffing of source images.

- Everything marked with "EPOGER" in the CL.

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/base_un...
File gm/rebaseline_server/base_unittest.py (right):

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/base_un...
gm/rebaseline_server/base_unittest.py:24: sys.path.insert(0, TOOLS_DIR)  #
EPOGER: CLEAN THIS UP!  The import of tests.base_unittest fails unless I do
this.
Throughout this patchset, there are these "EPOGER" notes that I intend to
resolve before committing the CL.

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

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/server....
gm/rebaseline_server/server.py:469: def do_GET_precomputed_results(self,
results_type):
For now, leaving this one in place to fetch the latest GM results (which are
pregenerated every 5 minutes using the --reload option, for fastest user
response).

Eventually, I hope we can get rid of this and use live result generation for all
rebaseline_server activity.  That depends on its performance being reasonable...

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/server....
gm/rebaseline_server/server.py:518: actuals_dirs=param_dict[PARAM_ACTUALS_DIR],
The live results are nice and flexible, because the caller can pass in arbitrary
actual/expectations directories.  We use the long-lived image_diff_db instance,
so any image diffs which have already been calculated don't need to be
calculated again.

But the first time a bunch of image pairs are diffed, this will take some
time...

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/static/...
File gm/rebaseline_server/static/live-loader.js (right):

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/static/...
gm/rebaseline_server/static/live-loader.js:130: $scope.actualsDir =
$location.search().actualsDir;
Copied this file from the original loader.js and modified it.

Hopefully, eventually we can delete loader.js and just use this one.

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/static/...
File gm/rebaseline_server/static/live-view.html (right):

https://codereview.chromium.org/424263005/diff/1/gm/rebaseline_server/static/...
gm/rebaseline_server/static/live-view.html:22: Loading results of query:
Similar to loader.js vs live-loader.js... this file was copied from view.html
and modified.  Eventually we should delete view.html in favor of this one.

[epoger, 2014-07-31 21:25:42]

In patchset 2, I added a --threads option to download the images and generate
diffs in parallel (as well as a temporary --truncate option so my manual tests
are mercifully fast).  It's definitely faster, but for a large number of test
results I think we would need a monster machine to get results in a reasonable
time.


If I launch the server like this on my Ubiquity instance (note the "rm" to start
fresh every time):

rm -rf gm/rebaseline_server/static/generated-* && \
  gm/rebaseline_server/server.py --export --builders '.*Mac10.6.*' \
  --boto ~/.boto-REAL --truncate --threads $THREADS


And then query it from a browser like so:

http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...


Here's how long it took to fulfill the query in 3 runs:
For THREADS=1: 54 sec, 32 sec, 29 sec
For THREADS=8: 8 sec, 9 sec, 10 sec


Note that if I send the same query to an already-running server, it takes <1
second regardless of THREADS, because all results are cached within the
ImageDiffDB object.

[epoger, 2014-08-01 15:17:55]

I ran some additional tests as of patchset 2 to figure out how much of the time
goes to downloading images from Google Storage, and how much is used for
generating the diffs.


I launched the server like this, on my Ubiquity instance:

rm -rf gm/rebaseline_server/static/generated-* && \
  gm/rebaseline_server/server.py --export --builders '.*Mac10.6.*' \
  --boto ~/.boto-REAL --truncate --threads 8

I varied the number of threads, and whether I ran the "rm" command or not. 
(Runs without the "rm" command don't download the images from Google Storage,
but they still generate diff images and pixel-diff statistics.)

For each configuration, I restarted the server 3 times, and queried it with:

http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...


Here are the results:

--threads 1:
WITH rm: 28 sec, 26 sec, 26 sec
WITHOUT rm: 11 sec, 11 sec, 11 sec

--threads 2:
WITH rm: 17 sec, 15 sec, 17 sec
WITHOUT rm: 6 sec, 6 sec, 6 sec

--threads 4:
WITH rm: 9 sec, 9 sec, 9 sec
WITHOUT rm: 5 sec, 4 sec, 5 sec

--threads 8:
WITH rm: 9 sec, 8 sec, 9 sec
WITHOUT rm: 4 sec, 5 sec, 5 sec

--threads 16:
WITH rm: 10 sec, 10 sec, 10 sec
WITHOUT rm: 4 sec, 4 sec, 4 sec


Conclusions (all for the optimally-multithreaded case):

A. We are spending about half of the time downloading images and the remaining
half generating the diffs.

B. It takes the server ~4.5 seconds to download 242 images from Google Storage
to my Ubiquity instance (total size about 4 Mbytes).
I will look into ways of downloading that same set of image data from Google
Storage faster (maybe figure out a way to batch up the downloads).

C. It takes the server ~4.5 seconds to generate 121 DiffRecords (launching the
skpdiff binary separately each time).
I will look into ways of generating a large number of DiffRecords more quickly
(probably batching up requests so there are fewer launches of the skpdiff
binary).

[rmistry, 2014-08-04 11:25:59]

On 2014/08/01 15:17:55, epoger wrote:
> I ran some additional tests as of patchset 2 to figure out how much of the
time
> goes to downloading images from Google Storage, and how much is used for
> generating the diffs.
> 
> 
> I launched the server like this, on my Ubiquity instance:
> 
> rm -rf gm/rebaseline_server/static/generated-* && \
>   gm/rebaseline_server/server.py --export --builders '.*Mac10.6.*' \
>   --boto ~/.boto-REAL --truncate --threads 8
> 
> I varied the number of threads, and whether I ran the "rm" command or not. 
> (Runs without the "rm" command don't download the images from Google Storage,
> but they still generate diff images and pixel-diff statistics.)
> 
> For each configuration, I restarted the server 3 times, and queried it with:
> 
>
http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...
> 
> 
> Here are the results:
> 
> --threads 1:
> WITH rm: 28 sec, 26 sec, 26 sec
> WITHOUT rm: 11 sec, 11 sec, 11 sec
> 
> --threads 2:
> WITH rm: 17 sec, 15 sec, 17 sec
> WITHOUT rm: 6 sec, 6 sec, 6 sec
> 
> --threads 4:
> WITH rm: 9 sec, 9 sec, 9 sec
> WITHOUT rm: 5 sec, 4 sec, 5 sec
> 
> --threads 8:
> WITH rm: 9 sec, 8 sec, 9 sec
> WITHOUT rm: 4 sec, 5 sec, 5 sec
> 
> --threads 16:
> WITH rm: 10 sec, 10 sec, 10 sec
> WITHOUT rm: 4 sec, 4 sec, 4 sec
> 
> 
> Conclusions (all for the optimally-multithreaded case):
> 
> A. We are spending about half of the time downloading images and the remaining
> half generating the diffs.
> 
> B. It takes the server ~4.5 seconds to download 242 images from Google Storage
> to my Ubiquity instance (total size about 4 Mbytes).
> I will look into ways of downloading that same set of image data from Google
> Storage faster (maybe figure out a way to batch up the downloads).
> 
> C. It takes the server ~4.5 seconds to generate 121 DiffRecords (launching the
> skpdiff binary separately each time).
> I will look into ways of generating a large number of DiffRecords more quickly
> (probably batching up requests so there are fewer launches of the skpdiff
> binary).

We should really make the rebaseline server a lot more beefier, it is only
g1-small (1 vCPU, 1.7 GB memory) right now. We should be able to make it atleast
4 cores.

[epoger, 2014-08-04 12:50:04]

> We should really make the rebaseline server a lot more beefier, it is only
> g1-small (1 vCPU, 1.7 GB memory) right now. We should be able to make it
atleast
> 4 cores.

Yes, agreed.  Please see my email thread ("added .boto file to
skia-rebaseline-server-1-b ... and we should migrate it to a heftier GCE
instance"), and set up a beefier instance for us.  In the meanwhile, I'll see
what I can do to make it more efficient.

Mmm, beef.

[epoger, 2014-08-04 14:57:27]

ALMOST ready for review as of patchset 3.  More cleanup coming.

[epoger, 2014-08-04 16:05:13]

OK, Ravi, ready for review at patchset 4!

With this change, the rebaseline_server now provides a "live" query interface
which callers can use to generate diffs between pairs of result sets upon
demand.

As discussed earlier, this is too slow to be useful for much in production
yet... but I think this is the way to go, and we can improve its performance in
various ways.

The big advantage is that it allows users to generate diffs between whatever
result sets they want.

I've left it running... you can try it out with a query like this:
http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...
(which gives you the diffs in JSON form)

or a query like this:
http://172.24.179.183:8888/static/live-view.html#/live-view.html?actualsDir=%...
(which displays them in the nice UI)


One big problem I hope you can help me figure out: when I go to that UI page, my
browser refuses to load the actuals and expectations images from Google Storage
(403 Access Denied).  I had thought my @google.com credentials would allow those
to be displayed...

[epoger, 2014-08-04 16:07:07]

On 2014/08/04 16:05:13, epoger wrote:
> I've left it running... you can try it out with a query like this:

Oops, sorry, I had those URLs wrong.  Here are the correct ones:

http://172.24.179.183:8888/live-results/actualsDir=gs%3a%2f%2fchromium-skia-s...
(displays the diffs in JSON format)

http://172.24.179.183:8888/static/live-view.html#/live-view.html?actualsDir=g...
(displays them in the UI)

[rmistry, 2014-08-04 18:47:53]

On 2014/08/04 16:05:13, epoger wrote:
> OK, Ravi, ready for review at patchset 4!
> 
> With this change, the rebaseline_server now provides a "live" query interface
> which callers can use to generate diffs between pairs of result sets upon
> demand.
> 
> As discussed earlier, this is too slow to be useful for much in production
> yet... but I think this is the way to go, and we can improve its performance
in
> various ways.
> 
> The big advantage is that it allows users to generate diffs between whatever
> result sets they want.
> 
> I've left it running... you can try it out with a query like this:
>
http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...
> (which gives you the diffs in JSON form)
> 
> or a query like this:
>
http://172.24.179.183:8888/static/live-view.html#/live-view.html?actualsDir=%...
> (which displays them in the nice UI)
> 
> 
> One big problem I hope you can help me figure out: when I go to that UI page,
my
> browser refuses to load the actuals and expectations images from Google
Storage
> (403 Access Denied).  I had thought my @google.com credentials would allow
those
> to be displayed...

Picking on desk_blogger_skp/bitmap-64bitMD5_9084154871324098052.png

$ gsutil getacl
gs://chromium-skia-skp-images/desk_blogger_skp/bitmap-64bitMD5_9084154871324098052.png
Gives
        <Entry>
            <Scope type="GroupByDomain">
                <Domain>google.com</Domain>
            </Scope>
            <Permission>READ</Permission>
        </Entry>


which looks correct right. After poking around some this is the URL that works
for me:
http://storage.cloud.google.com/chromium-skia-skp-images/desk_blogger_skp/bit...

Does this URL look different than the one you are using?

[epoger, 2014-08-04 18:54:13]

On 2014/08/04 18:47:53, rmistry wrote:
> On 2014/08/04 16:05:13, epoger wrote:
> > OK, Ravi, ready for review at patchset 4!
> > 
> > With this change, the rebaseline_server now provides a "live" query
interface
> > which callers can use to generate diffs between pairs of result sets upon
> > demand.
> > 
> > As discussed earlier, this is too slow to be useful for much in production
> > yet... but I think this is the way to go, and we can improve its performance
> in
> > various ways.
> > 
> > The big advantage is that it allows users to generate diffs between whatever
> > result sets they want.
> > 
> > I've left it running... you can try it out with a query like this:
> >
>
http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...
> > (which gives you the diffs in JSON form)
> > 
> > or a query like this:
> >
>
http://172.24.179.183:8888/static/live-view.html#/live-view.html?actualsDir=%...
> > (which displays them in the nice UI)
> > 
> > 
> > One big problem I hope you can help me figure out: when I go to that UI
page,
> my
> > browser refuses to load the actuals and expectations images from Google
> Storage
> > (403 Access Denied).  I had thought my @google.com credentials would allow
> those
> > to be displayed...
> 
> Picking on desk_blogger_skp/bitmap-64bitMD5_9084154871324098052.png
> 
> $ gsutil getacl
>
gs://chromium-skia-skp-images/desk_blogger_skp/bitmap-64bitMD5_9084154871324098052.png
> Gives
>         <Entry>
>             <Scope type="GroupByDomain">
>                 <Domain>google.com</Domain>
>             </Scope>
>             <Permission>READ</Permission>
>         </Entry>
> 
> 
> which looks correct right. After poking around some this is the URL that works
> for me:
>
http://storage.cloud.google.com/chromium-skia-skp-images/desk_blogger_skp/bit...
> 
> Does this URL look different than the one you are using?

I can view the image at the URL you gave.

Here's a sample image URL from my CL, which I cannot load:
http://chromium-skia-skp-images.commondatastorage.googleapis.com/desk_baidu_s...

Manually changing it to look like yours...
http://storage.cloud.google.com/chromium-skia-skp-images/desk_baidu_skp/bitma...
which works for me!

I will change the URL construction and upload the modified CL.  Thanks!  In the
meanwhile, I'll be happy to hear any other feedback on the CL.

[rmistry, 2014-08-04 18:55:12]

On 2014/08/04 18:54:13, epoger wrote:
> On 2014/08/04 18:47:53, rmistry wrote:
> > On 2014/08/04 16:05:13, epoger wrote:
> > > OK, Ravi, ready for review at patchset 4!
> > > 
> > > With this change, the rebaseline_server now provides a "live" query
> interface
> > > which callers can use to generate diffs between pairs of result sets upon
> > > demand.
> > > 
> > > As discussed earlier, this is too slow to be useful for much in production
> > > yet... but I think this is the way to go, and we can improve its
performance
> > in
> > > various ways.
> > > 
> > > The big advantage is that it allows users to generate diffs between
whatever
> > > result sets they want.
> > > 
> > > I've left it running... you can try it out with a query like this:
> > >
> >
>
http://172.24.179.183:8888/live-results/actualsDir=%2ftmp%2ffull-skp-actuals%...
> > > (which gives you the diffs in JSON form)
> > > 
> > > or a query like this:
> > >
> >
>
http://172.24.179.183:8888/static/live-view.html#/live-view.html?actualsDir=%...
> > > (which displays them in the nice UI)
> > > 
> > > 
> > > One big problem I hope you can help me figure out: when I go to that UI
> page,
> > my
> > > browser refuses to load the actuals and expectations images from Google
> > Storage
> > > (403 Access Denied).  I had thought my @google.com credentials would allow
> > those
> > > to be displayed...
> > 
> > Picking on desk_blogger_skp/bitmap-64bitMD5_9084154871324098052.png
> > 
> > $ gsutil getacl
> >
>
gs://chromium-skia-skp-images/desk_blogger_skp/bitmap-64bitMD5_9084154871324098052.png
> > Gives
> >         <Entry>
> >             <Scope type="GroupByDomain">
> >                 <Domain>google.com</Domain>
> >             </Scope>
> >             <Permission>READ</Permission>
> >         </Entry>
> > 
> > 
> > which looks correct right. After poking around some this is the URL that
works
> > for me:
> >
>
http://storage.cloud.google.com/chromium-skia-skp-images/desk_blogger_skp/bit...
> > 
> > Does this URL look different than the one you are using?
> 
> I can view the image at the URL you gave.
> 
> Here's a sample image URL from my CL, which I cannot load:
>
http://chromium-skia-skp-images.commondatastorage.googleapis.com/desk_baidu_s...
> 
> Manually changing it to look like yours...
>
http://storage.cloud.google.com/chromium-skia-skp-images/desk_baidu_skp/bitma...
> which works for me!
> 
> I will change the URL construction and upload the modified CL.  Thanks!  In
the
> meanwhile, I'll be happy to hear any other feedback on the CL.

Yes reviewing now, I will continue to review once I am home.

[epoger, 2014-08-04 19:05:34]

> > I will change the URL construction and upload the modified CL.  Thanks!  In
> the
> > meanwhile, I'll be happy to hear any other feedback on the CL.
> 
> Yes reviewing now, I will continue to review once I am home.

Patchset 5 fixes the image URLs!  Much better.  Thank you!

[rmistry, 2014-08-04 20:52:50]

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

https://codereview.chromium.org/424263005/diff/60001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:39: # the "too many open files" bug we saw
in http://skbug.com/2423
Is this a TODO or a comment?

https://codereview.chromium.org/424263005/diff/60001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:59: # the same file in multiple threads.
TODO or comment?

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

https://codereview.chromium.org/424263005/diff/60001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagepairset.py:221: (bucket, path) =
gs_utils.GSUtils.split_gs_url(gs_url)
Can directly do:
bucket, path = gs_utils.GSUtils... (do not need the parenthesis)

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
File gm/rebaseline_server/compare_rendered_pictures.py (right):

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:86: truncate_results: FOR
TESTING ONLY: if True, truncate the set of images
Another way to do this instead of making it part of the constructor's signature
is to make a module level constant TRUNCATE_RESULTS=False that is modified by
the test during setup and changed back in teardown. This does not apply if you
only meant to use this during manual startup of the rebaseline server.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:174:
self._validate_dict_version(dictA)
We should be able to extract out 174-178 to 180-186 into a function.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:183: dictB_results =
dictB[gm_json.JSONKEY_ACTUALRESULTS]
Why is this not JSONKEY_EXPECTEDRESULTS ?

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:190: skp_names =
skp_names[1:3]
Could you add a comment explaining the 1:3

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:295:
extra_columns_dict[COLUMN__TILED_OR_WHOLE] = 'whole'
Change 'whole' and 'tiled' into a module level constant?

[rmistry, 2014-08-04 20:53:38]

Lets also start CCing Stephan on rebaseline server's codereviews.

[epoger, 2014-08-05 03:34:09]

Thanks, Ravi.  Please see patchset 6.

Now that I have changed the "actuals"/"expectations" naming to "setA"/"setB" in
response to one of your comments, the test URLs have changed:

http://172.24.179.183:8888/live-results/setADir=gs%3a%2f%2fchromium-skia-skp-...
(to see the JSON)

http://172.24.179.183:8888/static/live-view.html#/live-view.html?setADir=gs%3...
(to see the UI)

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

https://codereview.chromium.org/424263005/diff/60001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:39: # the "too many open files" bug we saw
in http://skbug.com/2423
On 2014/08/04 20:52:49, rmistry wrote:
> Is this a TODO or a comment?

Done.

https://codereview.chromium.org/424263005/diff/60001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:59: # the same file in multiple threads.
On 2014/08/04 20:52:49, rmistry wrote:
> TODO or comment?

Done.

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

https://codereview.chromium.org/424263005/diff/60001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagepairset.py:221: (bucket, path) =
gs_utils.GSUtils.split_gs_url(gs_url)
On 2014/08/04 20:52:49, rmistry wrote:
> Can directly do:
> bucket, path = gs_utils.GSUtils... (do not need the parenthesis)

Done.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
File gm/rebaseline_server/compare_rendered_pictures.py (right):

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:86: truncate_results: FOR
TESTING ONLY: if True, truncate the set of images
On 2014/08/04 20:52:49, rmistry wrote:
> Another way to do this instead of making it part of the constructor's
signature
> is to make a module level constant TRUNCATE_RESULTS=False that is modified by
> the test during setup and changed back in teardown. This does not apply if you
> only meant to use this during manual startup of the rebaseline server.

Yeah, I meant manual testing.  Modified the comment.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:174:
self._validate_dict_version(dictA)
On 2014/08/04 20:52:50, rmistry wrote:
> We should be able to extract out 174-178 to 180-186 into a function.

That's a little tricky, because that extracted function would be similar to the
existing get_multilevel() method, but it would need to call
_validate_dict_version() for one of the *intermediate* dictionaries along the
way.

I replaced get_multilevel() with a new get_default() method that makes this
shorter, at least... seem reasonable now?

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:183: dictB_results =
dictB[gm_json.JSONKEY_ACTUALRESULTS]
On 2014/08/04 20:52:50, rmistry wrote:
> Why is this not JSONKEY_EXPECTEDRESULTS ?

Because, for now, I set this up to compare two sets of ACTUAL results, not 1 set
of actuals vs 1 set of expectations.

I did this for 2 reasons:
1. We don't have any expectations yet to try this with
2. We want to support comparing actuals from two different runs anyway.

I have changed all "actuals"/"expectations" to instead be "setA"/"setB", to
indicate that it can be any two sets of results.

We'll have to do a bit more to get expectations to show up properly... I think
that's for another CL later on.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:190: skp_names =
skp_names[1:3]
On 2014/08/04 20:52:50, rmistry wrote:
> Could you add a comment explaining the 1:3

Done.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:295:
extra_columns_dict[COLUMN__TILED_OR_WHOLE] = 'whole'
On 2014/08/04 20:52:50, rmistry wrote:
> Change 'whole' and 'tiled' into a module level constant?

For these column VALUES (as opposed to KEYS), I'd rather just leave them set
inline if that's OK with you.  These values are just for human consumption;
there is no other code that expects to find particular values in these fields. 
So there is no code to get out of sync if these values change.

[rmistry, 2014-08-05 13:46:54]

LGTM

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
File gm/rebaseline_server/compare_rendered_pictures.py (right):

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:86: truncate_results: FOR
TESTING ONLY: if True, truncate the set of images
On 2014/08/05 03:34:09, epoger wrote:
> On 2014/08/04 20:52:49, rmistry wrote:
> > Another way to do this instead of making it part of the constructor's
> signature
> > is to make a module level constant TRUNCATE_RESULTS=False that is modified
by
> > the test during setup and changed back in teardown. This does not apply if
you
> > only meant to use this during manual startup of the rebaseline server.
> 
> Yeah, I meant manual testing.  Modified the comment.

Acknowledged.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:174:
self._validate_dict_version(dictA)
On 2014/08/05 03:34:09, epoger wrote:
> On 2014/08/04 20:52:50, rmistry wrote:
> > We should be able to extract out 174-178 to 180-186 into a function.
> 
> That's a little tricky, because that extracted function would be similar to
the
> existing get_multilevel() method, but it would need to call
> _validate_dict_version() for one of the *intermediate* dictionaries along the
> way.
> 
> I replaced get_multilevel() with a new get_default() method that makes this
> shorter, at least... seem reasonable now?

Yes it does. Thanks.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:183: dictB_results =
dictB[gm_json.JSONKEY_ACTUALRESULTS]
On 2014/08/05 03:34:09, epoger wrote:
> On 2014/08/04 20:52:50, rmistry wrote:
> > Why is this not JSONKEY_EXPECTEDRESULTS ?
> 
> Because, for now, I set this up to compare two sets of ACTUAL results, not 1
set
> of actuals vs 1 set of expectations.
> 
> I did this for 2 reasons:
> 1. We don't have any expectations yet to try this with
> 2. We want to support comparing actuals from two different runs anyway.
> 
> I have changed all "actuals"/"expectations" to instead be "setA"/"setB", to
> indicate that it can be any two sets of results.
> 
> We'll have to do a bit more to get expectations to show up properly... I think
> that's for another CL later on.

SetA and SetB makes things a lot more intuitive for me. Thanks.

https://codereview.chromium.org/424263005/diff/80001/gm/rebaseline_server/com...
gm/rebaseline_server/compare_rendered_pictures.py:295:
extra_columns_dict[COLUMN__TILED_OR_WHOLE] = 'whole'
On 2014/08/05 03:34:09, epoger wrote:
> On 2014/08/04 20:52:50, rmistry wrote:
> > Change 'whole' and 'tiled' into a module level constant?
> 
> For these column VALUES (as opposed to KEYS), I'd rather just leave them set
> inline if that's OK with you.  These values are just for human consumption;
> there is no other code that expects to find particular values in these fields.

> So there is no code to get out of sync if these values change.

sgtm

[epoger, 2014-08-05 17:05:13]

The CQ bit was checked by epoger@google.com

[epoger, 2014-08-05 17:05:23]

The CQ bit was unchecked by epoger@google.com

[epoger, 2014-08-05 17:06:05]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-08-05 17:07:05]

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

[commit-bot: I haz the power, 2014-08-05 17:07:27]

Change committed as 0b7127635d8245de7ac704080d722d06e47621d0