Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157593006:
rebaseline_server: add ImagePair class, a step towards new intermediate JSON schema (Closed)
Descriptionrebaseline_server: add ImagePair class, a step towards new intermediate JSON schema
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=13385
Patch Set 1 #
Total comments: 21
Patch Set 2 : rmistry comments #
Total comments: 2
Patch Set 3 : style fix #
Messages
Total messages: 8 (0 generated)
Ready for review at patchset 1 https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:193: expected_image_locator = _sanitize_locator(expected_image_locator) I needed to start removing slashes from the image locators, because in the new approach we will use the partial URLs as image locators... "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”, Now, difference images will be stored with names like this: gradients_local_perspective_111111.png-vs-gradients_local_perspective_22222.png.png The extra ".png"s are kind of silly, but they won't hurt anyone. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb_test.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb_test.py:29: self.maxDiff = None don't truncate the diffs displayed by unittest in case of failure https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb_test.py:49: TODO(epoger): Instead of hitting Google Storage, we should read image If you think it's important, I can tackle this before committing; or we can leave it as a todo for later. It's already been running like this since it was first created. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... File gm/rebaseline_server/imagepair_test.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair_test.py:29: self.maxDiff = None don't truncate the diffs displayed by unittest in case of failure https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair_test.py:41: TODO(epoger): Either in addition to or instead of this end-to-end test, If you think it's important, I can tackle this before committing; or we can leave it as a todo for later. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair_test.py:126: 'imageAUrl': 'gradients_degenerate_2pt/10552995703607727960.png', yes, it's just over 80 chars, but I think wrapping it would be a cure worse than the disease. :-)
https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:193: expected_image_locator = _sanitize_locator(expected_image_locator) On 2014/02/07 21:36:17, epoger wrote: > I needed to start removing slashes from the image locators, because in the new > approach we will use the partial URLs as image locators... > > "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”, > > Now, difference images will be stored with names like this: > gradients_local_perspective_111111.png-vs-gradients_local_perspective_22222.png.png > > The extra ".png"s are kind of silly, but they won't hurt anyone. Yes it will not hurt, but out of curiosity what would it take to remove the extra '.png's? Is the 'image_suffix=DEFAULT_IMAGE_SUFFIX' causing complications with this? without it we should be able to strip out the IMAGE_SUFFIX? https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:351: return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator)) Isn't the locator always a string? Did it complain about unicode? https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb_test.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb_test.py:49: TODO(epoger): Instead of hitting Google Storage, we should read image On 2014/02/07 21:36:17, epoger wrote: > If you think it's important, I can tackle this before committing; or we can > leave it as a todo for later. > > It's already been running like this since it was first created. Does this hit the HTTP Google Storage or Google Storage with gsutil? I think it hits the HTTP Google Storage in which case I do not think its that important to fix. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... File gm/rebaseline_server/imagepair.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair.py:66: asdict['expectationsData'] = self.expectations_dict Keep the names of the keys as top-level constants? https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... File gm/rebaseline_server/imagepair_test.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair_test.py:41: TODO(epoger): Either in addition to or instead of this end-to-end test, On 2014/02/07 21:36:17, epoger wrote: > If you think it's important, I can tackle this before committing; or we can > leave it as a todo for later. TODO is fine with me https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair_test.py:126: 'imageAUrl': 'gradients_degenerate_2pt/10552995703607727960.png', On 2014/02/07 21:36:17, epoger wrote: > yes, it's just over 80 chars, but I think wrapping it would be a cure worse than > the disease. :-) I dont think it would be terrible to move the value to the next line: 'imageAUrl': 'gradients_degenerate_2pt/10552995703607727960.png'
Thanks, Ravi. Please see patchset 2. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:193: expected_image_locator = _sanitize_locator(expected_image_locator) On 2014/02/10 17:34:54, rmistry wrote: > On 2014/02/07 21:36:17, epoger wrote: > > I needed to start removing slashes from the image locators, because in the new > > approach we will use the partial URLs as image locators... > > > > "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”, > > > > Now, difference images will be stored with names like this: > > > gradients_local_perspective_111111.png-vs-gradients_local_perspective_22222.png.png > > > > The extra ".png"s are kind of silly, but they won't hurt anyone. > > Yes it will not hurt, but out of curiosity what would it take to remove the > extra '.png's? > Is the 'image_suffix=DEFAULT_IMAGE_SUFFIX' causing complications with this? > without it we should be able to strip out the IMAGE_SUFFIX? Here's my thinking; maybe you can help me "thread the needle" on this one. As noted in the design doc ( https://goto.google.com/ChangingRbsJson ), I want the intermediate JSON to look like this: { “ImageSets”: [ { “description”: “expected image”, “imageUrlBase”: “http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5” }, ... ], ... "ImagePairs": [ { "imageAUrl”: “gradients_local_perspective/7894968237305173737.png”, "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”, ... }, The idea here is that the URL paths make no assumptions--just assemble imageUrlBase and imageAUrl, and the client has a link to the image in question. I could imagine situations when we want those URLs to point at something other than PNGs. A couple of thoughts that come to mind: 1. Add "fileSuffix" to ImageSet description ("to generate the image URL, combine imageUrlBase + imageAUrl + fileSuffix"). This would require that all images in a given ImageSet have the same suffix. 2. Leave the intermediate JSON as it is, but strip out known file extensions in _sanitize_locator(). ("If the locator ends in '.png', we know we can strip that out and the locators will still be unique.") If we do that, though, when the client comes back and asks for a diff image, it will ask for "gradients_local_perspective/7894968237305173737.png-vs-gradients_local_perspective/14463581150258917673.png", and we'll have to know to convert that to "gradients_local_perspective/7894968237305173737-vs-gradients_local_perspective/14463581150258917673" Maybe I should leave it as I have it right now, put in a TODO noting the discussion, and we can discuss it more live tomorrow? https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:351: return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator)) On 2014/02/10 17:34:54, rmistry wrote: > Isn't the locator always a string? Did it complain about unicode? It will be a string most of the time in production after this change, but in the past (see unittest data) we used numbers. That caused failures until I added the str(). https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb_test.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb_test.py:49: TODO(epoger): Instead of hitting Google Storage, we should read image On 2014/02/10 17:34:54, rmistry wrote: > On 2014/02/07 21:36:17, epoger wrote: > > If you think it's important, I can tackle this before committing; or we can > > leave it as a todo for later. > > > > It's already been running like this since it was first created. > > Does this hit the HTTP Google Storage or Google Storage with gsutil? > I think it hits the HTTP Google Storage in which case I do not think its that > important to fix. You're right, it hits Google Storage via HTTP. Not as bad as gsutil, for sure. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... File gm/rebaseline_server/imagepair.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair.py:66: asdict['expectationsData'] = self.expectations_dict On 2014/02/10 17:34:54, rmistry wrote: > Keep the names of the keys as top-level constants? Done. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... File gm/rebaseline_server/imagepair_test.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagepa... gm/rebaseline_server/imagepair_test.py:126: 'imageAUrl': 'gradients_degenerate_2pt/10552995703607727960.png', On 2014/02/10 17:34:54, rmistry wrote: > On 2014/02/07 21:36:17, epoger wrote: > > yes, it's just over 80 chars, but I think wrapping it would be a cure worse > than > > the disease. :-) > > I dont think it would be terrible to move the value to the next line: > 'imageAUrl': > 'gradients_degenerate_2pt/10552995703607727960.png' Oh, all right. :-)
LGTM https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:193: expected_image_locator = _sanitize_locator(expected_image_locator) On 2014/02/10 18:01:16, epoger wrote: > On 2014/02/10 17:34:54, rmistry wrote: > > On 2014/02/07 21:36:17, epoger wrote: > > > I needed to start removing slashes from the image locators, because in the > new > > > approach we will use the partial URLs as image locators... > > > > > > "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”, > > > > > > Now, difference images will be stored with names like this: > > > > > > gradients_local_perspective_111111.png-vs-gradients_local_perspective_22222.png.png > > > > > > The extra ".png"s are kind of silly, but they won't hurt anyone. > > > > Yes it will not hurt, but out of curiosity what would it take to remove the > > extra '.png's? > > Is the 'image_suffix=DEFAULT_IMAGE_SUFFIX' causing complications with this? > > without it we should be able to strip out the IMAGE_SUFFIX? > > Here's my thinking; maybe you can help me "thread the needle" on this one. > > As noted in the design doc ( https://goto.google.com/ChangingRbsJson ), I want > the intermediate JSON to look like this: > > { > “ImageSets”: [ > { > “description”: “expected image”, > “imageUrlBase”: > “http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5” > }, > ... > ], > ... > "ImagePairs": [ > { > "imageAUrl”: “gradients_local_perspective/7894968237305173737.png”, > "imageBUrl”: “gradients_local_perspective/14463581150258917673.png”, > ... > }, > > The idea here is that the URL paths make no assumptions--just assemble > imageUrlBase and imageAUrl, and the client has a link to the image in question. > > I could imagine situations when we want those URLs to point at something other > than PNGs. > > A couple of thoughts that come to mind: > > 1. Add "fileSuffix" to ImageSet description ("to generate the image URL, combine > imageUrlBase + imageAUrl + fileSuffix"). This would require that all images in > a given ImageSet have the same suffix. > > 2. Leave the intermediate JSON as it is, but strip out known file extensions in > _sanitize_locator(). ("If the locator ends in '.png', we know we can strip that > out and the locators will still be unique.") If we do that, though, when the > client comes back and asks for a diff image, it will ask for > "gradients_local_perspective/7894968237305173737.png-vs-gradients_local_perspective/14463581150258917673.png", > and we'll have to know to convert that to > "gradients_local_perspective/7894968237305173737-vs-gradients_local_perspective/14463581150258917673" > > > Maybe I should leave it as I have it right now, put in a TODO noting the > discussion, and we can discuss it more live tomorrow? SGTM. Lets discuss live tomorrow (if weather permits). https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:351: return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator)) On 2014/02/10 18:01:16, epoger wrote: > On 2014/02/10 17:34:54, rmistry wrote: > > Isn't the locator always a string? Did it complain about unicode? > > It will be a string most of the time in production after this change, but in the > past (see unittest data) we used numbers. That caused failures until I added > the str(). Got it. I was just curious. https://codereview.chromium.org/157593006/diff/100001/gm/rebaseline_server/im... File gm/rebaseline_server/imagepair.py (right): https://codereview.chromium.org/157593006/diff/100001/gm/rebaseline_server/im... gm/rebaseline_server/imagepair.py:20: KEY_IS_DIFFERENT = 'isDifferent' From the style guide in http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace : Don't use spaces to vertically align tokens on consecutive lines, since it becomes a maintenance burden (applies to :, #, =, etc.)
https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... File gm/rebaseline_server/imagediffdb.py (right): https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:193: expected_image_locator = _sanitize_locator(expected_image_locator) On 2014/02/10 18:09:29, rmistry wrote: > SGTM. Lets discuss live tomorrow (if weather permits). "If weather permits", indeed. I'll make a note to discuss it with you SOMETIME soon. https://codereview.chromium.org/157593006/diff/1/gm/rebaseline_server/imagedi... gm/rebaseline_server/imagediffdb.py:351: return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator)) On 2014/02/10 18:09:29, rmistry wrote: > On 2014/02/10 18:01:16, epoger wrote: > > On 2014/02/10 17:34:54, rmistry wrote: > > > Isn't the locator always a string? Did it complain about unicode? > > > > It will be a string most of the time in production after this change, but in > the > > past (see unittest data) we used numbers. That caused failures until I added > > the str(). > > Got it. I was just curious. No problem, thanks for checking into it. https://codereview.chromium.org/157593006/diff/100001/gm/rebaseline_server/im... File gm/rebaseline_server/imagepair.py (right): https://codereview.chromium.org/157593006/diff/100001/gm/rebaseline_server/im... gm/rebaseline_server/imagepair.py:20: KEY_IS_DIFFERENT = 'isDifferent' On 2014/02/10 18:09:29, rmistry wrote: > From the style guide in > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace : > Don't use spaces to vertically align tokens on consecutive lines, since it > becomes a maintenance burden (applies to :, #, =, etc.) Wow. Thanks for finding that... even though I don't agree with it. :-)
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/157593006/170001
Message was sent while issue was closed.
Change committed as 13385 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
