new tool: download all GM images for a given builder, ready for skdiff

new tool: download all GM images for a given builder, ready for skdiff
BUG=skia:1919
Committed: http://code.google.com/p/skia/source/detail?r=13172

[epoger, 2014-01-22 19:58:10]

Ready for review at patchset 1.

Brian, please just look at:
1. gm/rebaseline_server/download.py , for the user interface
2. arrangement of downloaded contents within
gm/rebaseline_server/tests/outputs/expected/download_test.DownloadTest.test_fetch/
 (you will see that the images are arranged in per-config directories)

Ravi, please review the whole CL (but see my notes below--you can probably skip
most/all of the self-test results)

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download_test.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_test.py:9: Test download.py
git decided that this file is a modified version of results.py ... and it was
right.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/tests/....
File gm/rebaseline_server/tests/.gitattributes (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/tests/....
gm/rebaseline_server/tests/.gitattributes:1: # All self-test PNG files are fake
(human-readable, diffable plaintext files).
From here, you can skip ahead to gm/rebaseline_server/url_or_path.py ; all the
intervening files are new self-test expectations.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/tests/....
gm/rebaseline_server/tests/.gitattributes:2: *.png    text diff -binary
In spite of this git-attr directive, Rietveld won't show the (human readable)
contents of the test .png files.

But if you look at
https://codereview.chromium.org/download/issue143653006_1.diff , you will see
that each .png file contains a single line like this:
contents of bitmap-64bitMD5/3x3bitmaprect/2054956815327187963.png

I have confirmed that the appropriate files are being copied from
gm/rebaseline_server/tests/inputs/fake-gm-imagefiles to
gm/rebaseline_server/tests/outputs/expected/download_test.DownloadTest.test_fetch
, as directed by
gm/rebaseline_server/tests/inputs/gm-actuals/Test-Android-GalaxyNexus-SGX540-Arm7-Release/actual-results.json
.

For example, as directed by this line:
    "bigblurs_565.png" : [ "bitmap-64bitMD5", 2422083043229439955 ],
gm/rebaseline_server/tests/outputs/expected/download_test.DownloadTest.test_fetch/565/bigblurs.png
reads:
    contents of bitmap-64bitMD5/bigblurs/2422083043229439955.png

[rmistry, 2014-01-22 22:00:30]

Initial comments.. more coming tomorrow.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:1: #!/usr/bin/python
The name of this file is very generic (download.py). Should we make it specific
by calling it 'download_builder_gms.py'.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:32: DEFAULT_ACTUALS_BASE_URL =
'http://skia-autogen.googlecode.com/svn/gm-actual'
'https://skia-autogen.googlecode.com/svn' exists in global_variables.json, can
we instead read it from there?

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
File gm/rebaseline_server/url_or_path.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:1: #!/usr/bin/python
The file name is confusing to me, how about something like
'url_filepath_utils.py'.
How about creating two subclasses UrlUtils and FilepathUtils (or better names)
and a factory that instantiates and returns the right one to the caller based on
is_url == True. That would avoid the many 'if is_url'. All these classes could
live in the same module.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:26: scheme = urlparse.urlparse(path).scheme
return True if urlparse.urlparse(path).scheme else False
or
return urlparse.urlparse(path).scheme != None (assuming scheme is None for file
paths).

[bsalomon, 2014-01-22 22:03:52]

On 2014/01/22 19:58:10, epoger wrote:
> Ready for review at patchset 1.
> 
> Brian, please just look at:
> 1. gm/rebaseline_server/download.py , for the user interface
> 2. arrangement of downloaded contents within
>
gm/rebaseline_server/tests/outputs/expected/download_test.DownloadTest.test_fetch/
>  (you will see that the images are arranged in per-config directories)
> 

This all lgtm. Is it possible for it to be able to list the set of builders as
the old rebaseline script did?

[epoger, 2014-01-23 04:15:31]

Patchset 2 addresses Brian's request.

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:104: 'To see a list of builders, run "svn ls
%s".' %
On 2014/01/22 22:03:52, bsalomon wrote:
> Is it possible for it to be able to list the set of builders as
> the old rebaseline script did?

At a cost, yes.

If you look at
https://code.google.com/p/skia/source/browse/trunk/tools/rebaseline.py?r=1271...
, you will see that rebaseline.py doesn't provide an up-to-date list of
builders, and an indication of why not.

For now, I added information that the caller can use to easily generate the list
herself... does that seem sufficient?

If you'd like me to add a TODO to provide more, I'm all for it--please tell me
what would be your preferred methodology (display the builder list within --help
output, add a --list-builders flag that lists builders instead of doing anything
else, or ???)

[epoger, 2014-01-23 05:00:52]

Patchset 3 addresses Ravi's comments; Ravi, PTAL.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:1: #!/usr/bin/python
On 2014/01/22 22:00:30, rmistry wrote:
> The name of this file is very generic (download.py). Should we make it
specific
> by calling it 'download_builder_gms.py'.

A fair point.  How about download_gm_actuals.py ?  (I can imagine a desire for
an analogous tool, download_gm_expectations.py, which would share most of the
code.)

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:32: DEFAULT_ACTUALS_BASE_URL =
'http://skia-autogen.googlecode.com/svn/gm-actual'
On 2014/01/22 22:00:30, rmistry wrote:
> 'https://skia-autogen.googlecode.com/svn' exists in global_variables.json, can
> we instead read it from there?

Done.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
File gm/rebaseline_server/url_or_path.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:1: #!/usr/bin/python
On 2014/01/22 22:00:30, rmistry wrote:
> The file name is confusing to me, how about something like
> 'url_filepath_utils.py'.
> How about creating two subclasses UrlUtils and FilepathUtils (or better names)
> and a factory that instantiates and returns the right one to the caller based
on
> is_url == True. That would avoid the many 'if is_url'. All these classes could
> live in the same module.
> 

Good thoughts; let's discuss live.

One more option to consider: throw away all of this module except for
create_filepath_url(), and use URLs everywhere.  If we ever want to access a
file on local disk, call create_filepath_url() first thing to get the filepath
in URL form.

I think that would work, perhaps with a performance penalty (copying from one
file to another on local disk will require a trip through urllib).

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:26: scheme = urlparse.urlparse(path).scheme
On 2014/01/22 22:00:30, rmistry wrote:
> return True if urlparse.urlparse(path).scheme else False
> or
> return urlparse.urlparse(path).scheme != None (assuming scheme is None for
file
> paths).

Done.

[rmistry, 2014-01-23 12:48:22]

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:1: #!/usr/bin/python
On 2014/01/23 05:00:53, epoger wrote:
> On 2014/01/22 22:00:30, rmistry wrote:
> > The name of this file is very generic (download.py). Should we make it
> specific
> > by calling it 'download_builder_gms.py'.
> 
> A fair point.  How about download_gm_actuals.py ?  (I can imagine a desire for
> an analogous tool, download_gm_expectations.py, which would share most of the
> code.)

download_gm_actuals.py SGTM

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
File gm/rebaseline_server/url_or_path.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:1: #!/usr/bin/python
On 2014/01/23 05:00:53, epoger wrote:
> On 2014/01/22 22:00:30, rmistry wrote:
> > The file name is confusing to me, how about something like
> > 'url_filepath_utils.py'.
> > How about creating two subclasses UrlUtils and FilepathUtils (or better
names)
> > and a factory that instantiates and returns the right one to the caller
based
> on
> > is_url == True. That would avoid the many 'if is_url'. All these classes
could
> > live in the same module.
> > 
> 
> Good thoughts; let's discuss live.

Feel free to grab me anytime today.

> 
> One more option to consider: throw away all of this module except for
> create_filepath_url(), and use URLs everywhere.  If we ever want to access a
> file on local disk, call create_filepath_url() first thing to get the filepath
> in URL form.
> 
> I think that would work, perhaps with a performance penalty (copying from one
> file to another on local disk will require a trip through urllib).

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:104: 'To see a list of builders, run "svn ls
%s".' %
On 2014/01/23 04:15:32, epoger wrote:
> On 2014/01/22 22:03:52, bsalomon wrote:
> > Is it possible for it to be able to list the set of builders as
> > the old rebaseline script did?
> 
> At a cost, yes.
> 
> If you look at
>
https://code.google.com/p/skia/source/browse/trunk/tools/rebaseline.py?r=1271...
> , you will see that rebaseline.py doesn't provide an up-to-date list of
> builders, and an indication of why not.
> 
> For now, I added information that the caller can use to easily generate the
list
> herself... does that seem sufficient?
> 
> If you'd like me to add a TODO to provide more, I'm all for it--please tell me
> what would be your preferred methodology (display the builder list within
--help
> output, add a --list-builders flag that lists builders instead of doing
anything
> else, or ???)

IMO having a --list-builders step would make it the easiest for users to list
everything out and then pick and choose as needed. This would be consistent with
the submit_try tool.

https://codereview.chromium.org/143653006/diff/490001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/490001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:136: remaining_args)
This is a good way to make sure all required options were set. I prefer another
method-
I explicitly check all the params:
if (not params.actuals_base_url or not params.builder or not....):

This makes it easier to add optional params in the future. This is purely
optional just wanted to mention it.

Eg:
https://skia.googlesource.com/buildbot/+/master/compute_engine_scripts/teleme...

[bsalomon, 2014-01-23 14:20:32]

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:104: 'To see a list of builders, run "svn ls
%s".' %
This seems totally sufficient. Thanks!

[epoger, 2014-01-23 15:34:18]

Notes from live discussion with Ravi

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:1: #!/usr/bin/python
On 2014/01/23 12:48:23, rmistry wrote:
> On 2014/01/23 05:00:53, epoger wrote:
> > On 2014/01/22 22:00:30, rmistry wrote:
> > > The name of this file is very generic (download.py). Should we make it
> > specific
> > > by calling it 'download_builder_gms.py'.
> > 
> > A fair point.  How about download_gm_actuals.py ?  (I can imagine a desire
for
> > an analogous tool, download_gm_expectations.py, which would share most of
the
> > code.)
> 
> download_gm_actuals.py SGTM

Discussed live: will name it download_actuals.py

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
File gm/rebaseline_server/url_or_path.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:1: #!/usr/bin/python
On 2014/01/23 05:00:53, epoger wrote:
> On 2014/01/22 22:00:30, rmistry wrote:
> > The file name is confusing to me, how about something like
> > 'url_filepath_utils.py'.
> > How about creating two subclasses UrlUtils and FilepathUtils (or better
names)
> > and a factory that instantiates and returns the right one to the caller
based
> on
> > is_url == True. That would avoid the many 'if is_url'. All these classes
could
> > live in the same module.
> > 
> 
> Good thoughts; let's discuss live.
> 
> One more option to consider: throw away all of this module except for
> create_filepath_url(), and use URLs everywhere.  If we ever want to access a
> file on local disk, call create_filepath_url() first thing to get the filepath
> in URL form.
> 
> I think that would work, perhaps with a performance penalty (copying from one
> file to another on local disk will require a trip through urllib).

Discussed live: I will go with file:/// URLs, and get rid of most of this file.

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:104: 'To see a list of builders, run "svn ls
%s".' %
On 2014/01/23 12:48:23, rmistry wrote:
> On 2014/01/23 04:15:32, epoger wrote:
> > On 2014/01/22 22:03:52, bsalomon wrote:
> > > Is it possible for it to be able to list the set of builders as
> > > the old rebaseline script did?
> > 
> > At a cost, yes.
> > 
> > If you look at
> >
>
https://code.google.com/p/skia/source/browse/trunk/tools/rebaseline.py?r=1271...
> > , you will see that rebaseline.py doesn't provide an up-to-date list of
> > builders, and an indication of why not.
> > 
> > For now, I added information that the caller can use to easily generate the
> list
> > herself... does that seem sufficient?
> > 
> > If you'd like me to add a TODO to provide more, I'm all for it--please tell
me
> > what would be your preferred methodology (display the builder list within
> --help
> > output, add a --list-builders flag that lists builders instead of doing
> anything
> > else, or ???)
> 
> IMO having a --list-builders step would make it the easiest for users to list
> everything out and then pick and choose as needed. This would be consistent
with
> the submit_try tool.

Discussed live: I will add this as a TODO, but "svn ls" is the best way to go
for now anyway because the builders are spread across multiple masters anyway.

https://codereview.chromium.org/143653006/diff/490001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/490001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:136: remaining_args)
On 2014/01/23 12:48:23, rmistry wrote:
> This is a good way to make sure all required options were set. I prefer
another
> method-
> I explicitly check all the params:
> if (not params.actuals_base_url or not params.builder or not....):
> 
> This makes it easier to add optional params in the future. This is purely
> optional just wanted to mention it.
> 
> Eg:
>
https://skia.googlesource.com/buildbot/+/master/compute_engine_scripts/teleme...

Discussed live: I will make a list of required options, and iterate over the
list and check each one.

[epoger, 2014-01-24 02:00:45]

Ravi, PTAL...

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download.py:1: #!/usr/bin/python
On 2014/01/23 15:34:19, epoger wrote:
> On 2014/01/23 12:48:23, rmistry wrote:
> > On 2014/01/23 05:00:53, epoger wrote:
> > > On 2014/01/22 22:00:30, rmistry wrote:
> > > > The name of this file is very generic (download.py). Should we make it
> > > specific
> > > > by calling it 'download_builder_gms.py'.
> > > 
> > > A fair point.  How about download_gm_actuals.py ?  (I can imagine a desire
> for
> > > an analogous tool, download_gm_expectations.py, which would share most of
> the
> > > code.)
> > 
> > download_gm_actuals.py SGTM
> 
> Discussed live: will name it download_actuals.py

Done.

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
File gm/rebaseline_server/url_or_path.py (right):

https://codereview.chromium.org/143653006/diff/1/gm/rebaseline_server/url_or_...
gm/rebaseline_server/url_or_path.py:1: #!/usr/bin/python
On 2014/01/23 15:34:19, epoger wrote:
> Discussed live: I will go with file:/// URLs, and get rid of most of this
file.

Done.  I use file:/// URLs for the unittest.  I kept create_filepath_url() in
the "real code" (not the unittest), in case we want to enable reading results
from local disk (which we would like the tool to be able to do).

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/290001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:104: 'To see a list of builders, run "svn ls
%s".' %
On 2014/01/23 15:34:19, epoger wrote:
> On 2014/01/23 12:48:23, rmistry wrote:
> > On 2014/01/23 04:15:32, epoger wrote:
> > > On 2014/01/22 22:03:52, bsalomon wrote:
> > > > Is it possible for it to be able to list the set of builders as
> > > > the old rebaseline script did?
> > > 
> > > At a cost, yes.
> > > 
> > > If you look at
> > >
> >
>
https://code.google.com/p/skia/source/browse/trunk/tools/rebaseline.py?r=1271...
> > > , you will see that rebaseline.py doesn't provide an up-to-date list of
> > > builders, and an indication of why not.
> > > 
> > > For now, I added information that the caller can use to easily generate
the
> > list
> > > herself... does that seem sufficient?
> > > 
> > > If you'd like me to add a TODO to provide more, I'm all for it--please
tell
> me
> > > what would be your preferred methodology (display the builder list within
> > --help
> > > output, add a --list-builders flag that lists builders instead of doing
> > anything
> > > else, or ???)
> > 
> > IMO having a --list-builders step would make it the easiest for users to
list
> > everything out and then pick and choose as needed. This would be consistent
> with
> > the submit_try tool.
> 
> Discussed live: I will add this as a TODO, but "svn ls" is the best way to go
> for now anyway because the builders are spread across multiple masters anyway.

Done.

https://codereview.chromium.org/143653006/diff/490001/gm/rebaseline_server/do...
File gm/rebaseline_server/download.py (right):

https://codereview.chromium.org/143653006/diff/490001/gm/rebaseline_server/do...
gm/rebaseline_server/download.py:136: remaining_args)
On 2014/01/23 15:34:19, epoger wrote:
> On 2014/01/23 12:48:23, rmistry wrote:
> > This is a good way to make sure all required options were set. I prefer
> another
> > method-
> > I explicitly check all the params:
> > if (not params.actuals_base_url or not params.builder or not....):
> > 
> > This makes it easier to add optional params in the future. This is purely
> > optional just wanted to mention it.
> > 
> > Eg:
> >
>
https://skia.googlesource.com/buildbot/+/master/compute_engine_scripts/teleme...
> 
> Discussed live: I will make a list of required options, and iterate over the
> list and check each one.

Done.

[rmistry, 2014-01-24 18:07:35]

LGTM

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
gm/rebaseline_server/download_actuals.py:103: def create_filepath_url(filepath):
I do not see this used in this module, it is here and not in the unittest
instead because you are anticipating some other callers using it?
In the future when you have a download_expectations you may need this similarly
and it may need to be moved to a utils module.

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
gm/rebaseline_server/download_actuals.py:122: 
nit: add an extra newline

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
gm/rebaseline_server/download_actuals.py:144: 
nit: add an extra newline

[epoger, 2014-01-24 18:23:49]

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
gm/rebaseline_server/download_actuals.py:103: def create_filepath_url(filepath):
On 2014/01/24 18:07:35, rmistry wrote:
> I do not see this used in this module, it is here and not in the unittest
> instead because you are anticipating some other callers using it?

Yes. Added a comment along those lines.

> In the future when you have a download_expectations you may need this
similarly
> and it may need to be moved to a utils module.

Agreed.

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
gm/rebaseline_server/download_actuals.py:122: 
On 2014/01/24 18:07:35, rmistry wrote:
> nit: add an extra newline

Done.

https://codereview.chromium.org/143653006/diff/600002/gm/rebaseline_server/do...
gm/rebaseline_server/download_actuals.py:144: 
On 2014/01/24 18:07:35, rmistry wrote:
> nit: add an extra newline

Done.

[commit-bot: I haz the power, 2014-01-24 18:25:45]

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

[commit-bot: I haz the power, 2014-01-24 18:34:25]

Change committed as 13172