download_actuals.py: download JSON files from Google Storage instead of skia-autogen

download_actuals.py: download JSON files from Google Storage instead of skia-autogen

uses google-api-python-client instead of gsutil binary to interact with Google Storage

BUG=skia:553
Committed: https://skia.googlesource.com/skia/+/55ada0630e1ce1bfd6e6e60b70370384530622cc

[epoger, 2014-05-30 22:11:35]

Ravi- PTAL.

Eric/Ben- I'm trying a new way of accessing Google Storage
(google-api-python-client instead of going through gsutil).  I think it may be
generally useful.

https://codereview.chromium.org/309653005/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/309653005/diff/1/DEPS#newcode11
DEPS:11: "third_party/externals/google-api-python-client" :
"https://github.com/google/google-api-python-client.git@56557e2c1d2cbce0d2de26e3a7f32f836b8f5eb2",
https://code.google.com/p/google-api-python-client/ tells me that this github
repo is now the official home of the google-api-python-client .  I don't see a
mirror of it on chromium.googlesource.com, so I just pointed at the official
home on github.

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

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:165: def
gcs_list_bucket_contents(bucket, subdir=None):
I plan to use this to replace the use of the gsutil binary in server.py, solving
the problem we discussed in
https://codereview.chromium.org/303223006/diff/1/gm/rebaseline_server/server....
.

The GM summary dirs in Google Storage are publicly readable, so you don't even
need any .boto credentials to use the API.

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:197: '"%default".'))
Unfortunately, this tool has lost the ability to read old revisions of the JSON
file.  Since we use versioned storage, that information is available for
download... but I haven't figured out how the user should specify the revision
to retrieve.

[benchen, 2014-05-30 23:45:03]

Thanks for trying this out! Using an api certainly sounds better than calling a
binary..

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

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:165: def
gcs_list_bucket_contents(bucket, subdir=None):
This looks promising. If we use the API for restricted ops, it looks like the
.boto auth will be replaced by a client_secrets.json file
https://developers.google.com/api-client-library/python/samples/samples#Cloud...

On 2014/05/30 22:11:36, epoger wrote:
> I plan to use this to replace the use of the gsutil binary in server.py,
solving
> the problem we discussed in
>
https://codereview.chromium.org/303223006/diff/1/gm/rebaseline_server/server....
> .
> 
> The GM summary dirs in Google Storage are publicly readable, so you don't even
> need any .boto credentials to use the API.

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:197: '"%default".'))
Joe must know the solution in details.
On 2014/05/30 22:11:36, epoger wrote:
> Unfortunately, this tool has lost the ability to read old revisions of the
JSON
> file.  Since we use versioned storage, that information is available for
> download... but I haven't figured out how the user should specify the revision
> to retrieve.

[epoger, 2014-06-03 12:01:53]

ravi: ping

[rmistry, 2014-06-03 12:23:39]

Kudos for trying out this approach.

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

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:174: subdir += '/'
Can replace 173 and 174 with:
posixpath.join(subdir, '')
and could do:
subdir_length = len(subdir) if subdir else 0

This changes 6 lines into 3 lines:
if subdir:
  posixpath.join(subdir, '')
subdir_length = len(subdir) if subdir else 0

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:179: storage = build_service('storage',
'v1')
google-api-python-client is cool, I wonder who wrote it (though
google-api-java-client is cooler).

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:181: bucket=bucket, delimiter='/',
fields='items(name),prefixes',
Can also use posixpath.sep instead of '/' though I am not sure which is better.

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:185: files =
[item['name'][subdir_length:] for item in results.get('items', [])]
Lines 184 and 185 are a little confusing could you add some documentation or
break up the lines to make them more self-explanatory?

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:197: '"%default".'))
On 2014/05/30 23:45:03, benchen wrote:
> Joe must know the solution in details.
> On 2014/05/30 22:11:36, epoger wrote:
> > Unfortunately, this tool has lost the ability to read old revisions of the
> JSON
> > file.  Since we use versioned storage, that information is available for
> > download... but I haven't figured out how the user should specify the
revision
> > to retrieve.
> 

Or Joe will know who to ask from the Cloud Storage API team.

[epoger, 2014-06-03 15:27:52]

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

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:174: subdir += '/'
On 2014/06/03 12:23:39, rmistry wrote:
> Can replace 173 and 174 with:
> posixpath.join(subdir, '')
> and could do:
> subdir_length = len(subdir) if subdir else 0
> 
> This changes 6 lines into 3 lines:
> if subdir:
>   posixpath.join(subdir, '')
> subdir_length = len(subdir) if subdir else 0

Took half of the suggestion, thanks.  I would prefer to be clear about exactly
what the trailing slash is about, so kept it explicit and added a comment.

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:179: storage = build_service('storage',
'v1')
On 2014/06/03 12:23:39, rmistry wrote:
> google-api-python-client is cool, I wonder who wrote it (though
> google-api-java-client is cooler).

I heard that the guy who wrote google-api-java-client is a bit unpredictable. 
Rumor has it they won't even let him eat solid food.

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:181: bucket=bucket, delimiter='/',
fields='items(name),prefixes',
On 2014/06/03 12:23:39, rmistry wrote:
> Can also use posixpath.sep instead of '/' though I am not sure which is
better.

I think it's 50/50, so went with the shorter one.  I am happy to change it if
you prefer.

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:185: files =
[item['name'][subdir_length:] for item in results.get('items', [])]
On 2014/06/03 12:23:39, rmistry wrote:
> Lines 184 and 185 are a little confusing could you add some documentation or
> break up the lines to make them more self-explanatory?

You're right.  Do you think the comments are sufficient?  Or do you think I
should also break up the list comprehension into more comprehensible code?

https://codereview.chromium.org/309653005/diff/1/gm/rebaseline_server/downloa...
gm/rebaseline_server/download_actuals.py:197: '"%default".'))
On 2014/06/03 12:23:39, rmistry wrote:
> On 2014/05/30 23:45:03, benchen wrote:
> > Joe must know the solution in details.
> > On 2014/05/30 22:11:36, epoger wrote:
> > > Unfortunately, this tool has lost the ability to read old revisions of the
> > JSON
> > > file.  Since we use versioned storage, that information is available for
> > > download... but I haven't figured out how the user should specify the
> revision
> > > to retrieve.
> > 
> 
> Or Joe will know who to ask from the Cloud Storage API team.

Sorry, I wasn't clear.  It isn't a Cloud Storage API question.  It's a matter of
figuring out which timestamped version of the actual-results.json file we want
to retrieve from Google Storage.

[epoger, 2014-06-03 17:08:31]

https://codereview.chromium.org/309653005/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/309653005/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:191: for dir_fullpath in
results.get('prefixes', []):
Ravi- I hope this is easier to follow.

[rmistry, 2014-06-03 17:23:15]

LGTM

[epoger, 2014-06-03 17:23:45]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-06-03 17:24:09]

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

[commit-bot: I haz the power, 2014-06-03 17:35:32]

Change committed as 55ada0630e1ce1bfd6e6e60b70370384530622cc