Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 6 months ago by epoger Modified:
6 years, 6 months ago Reviewers:
rmistry, benchen CC:
skia-review_googlegroups.com, borenet, benchen Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
Descriptiondownload_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
Patch Set 1 #
Total comments: 15
Patch Set 2 : Ravi comments #Patch Set 3 : expand list comprehensions to be more readable #
Total comments: 1
Created: 6 years, 6 months ago
Messages
Total messages: 10 (0 generated)
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.
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.
ravi: ping
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.
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.
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.
LGTM
The CQ bit was checked by epoger@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/epoger@google.com/309653005/40001
Message was sent while issue was closed.
Change committed as 55ada0630e1ce1bfd6e6e60b70370384530622cc |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
