Issue 270413002: rebaseline_server JSON: pass category values as values, not keys

Issue 270413002: rebaseline_server JSON: pass category values as values, not keys (Closed)

Created:
6 years, 7 months ago by epoger

Modified:
6 years, 7 months ago

Reviewers:
borenet

CC:
skia-review_googlegroups.com

Base URL:
https://skia.googlesource.com/skia.git@master

Visibility:
Public.

More Reviews

Description

rebaseline_server JSON: pass category values as values, not keys BUG=skia:1758 NOTRY=True Committed: http://code.google.com/p/skia/source/detail?r=14658

Patch Set 2 : use list of tuples rather than two separate lists #

Patch Set 3 : rebase #

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+263 lines, -108 lines)			Patch
M	gm/rebaseline_server/column.py	View	1	3 chunks	+14 lines, -11 lines	0 comments	Download
M	gm/rebaseline_server/compare_to_expectations.py	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	gm/rebaseline_server/imagepairset.py	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	gm/rebaseline_server/imagepairset_test.py	View	1	1 chunk	+1 line, -3 lines	0 comments	Download
M	gm/rebaseline_server/results.py	View		1 chunk	+1 line, -1 line	0 comments	Download
M	gm/rebaseline_server/static/constants.js	View	1	3 chunks	+7 lines, -7 lines	0 comments	Download
M	gm/rebaseline_server/static/loader.js	View	1	2 chunks	+28 lines, -4 lines	0 comments	Download
M	gm/rebaseline_server/static/view.html	View	1 2	2 chunks	+10 lines, -10 lines	0 comments	Download
M	gm/rebaseline_server/testdata/outputs/expected/compare_configs_test.CompareConfigsTest.test_gm/gm.json	View	1	1 chunk	+69 lines, -24 lines	0 comments	Download
M	gm/rebaseline_server/testdata/outputs/expected/compare_rendered_pictures_test.CompareRenderedPicturesTest.test_endToEnd/compare_rendered_pictures.json	View	1 2	1 chunk	+45 lines, -18 lines	0 comments	Download
M	gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json	View	1	1 chunk	+85 lines, -28 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

epoger

PTAL at patchset 1. https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column.py File gm/rebaseline_server/column.py (right): https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column.py#newcode2 gm/rebaseline_server/column.py:2: Reason for the CL: the ...

6 years, 7 months ago (2014-05-07 21:26:16 UTC) #1

borenet

I don't really like having two separate lists. https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column.py File gm/rebaseline_server/column.py (right): https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column.py#newcode2 gm/rebaseline_server/column.py:2: On ...

6 years, 7 months ago (2014-05-08 12:40:17 UTC) #2

I don't really like having two separate lists.

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

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:2: 
On 2014/05/07 21:26:17, epoger wrote:
> Reason for the CL: the fact that the rebaseline_server JSON included arbitrary
> field values as dictionary keys limited which fields we could search and sort
> on.  See http://skbug.com/1758 for more details.
> 
> This is a different approach than I had originally envisioned in the bug , but
> as I started on the implementation I decided this approach was better, and
> should resolve the same root problem.

Top-level comment about parsing JSON: I really like the way it's done in Go. 
You provide a struct which gets filled in by the decoder:

myData := struct {
  MyField:      string
  MyOtherField: int
}{}
json.Unmarshal(jsonData, &myData)

You can even specify how to convert the names of the fields from JSON to Go:

myData := struct {
  MyField:      string `json:"my_field"`
  MyOtherField: int    `json:"another_field"`
}{}

There's no guarantee that all of the fields of the struct get filled in, or that
everything in the JSON data was put into a field.

I'm just wondering if there might be a way to do something similar in Python and
avoid having to use all these constants.

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepairset_test.py (right):

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset_test.py:95: 'values': ['MyBuilder'],
My concern with this format is that it's possible for the two lists to be out of
sync.  What if we used tuples or lists to create pairs:

'valuesAndCounts': [
  ('MyBuilder', 3),
]

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/static/...
File gm/rebaseline_server/static/constants.js (right):

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/static/...
gm/rebaseline_server/static/constants.js:45: // NOTE: Keep these in sync with
../results.py
Yikes.

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

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/utils.p...
gm/rebaseline_server/utils.py:7: found in the LICENSE file.
I've been copy/pasting the same header, which reads:

# Copyright (c) 2013 The Chromium Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/utils.p...
gm/rebaseline_server/utils.py:14: within the lists can be used to connect key
and value.
It seems like this should be a built-in function.  I guess there's no guarantee
that the following code would give you correct orderings?

return dict.keys(), dict.values()

If the results don't need to be sorted, this would be faster, since there's no
copy:
for k, v in input_dict.iteritems():
  keys.append(k)
  values.append(v)

epoger

Thanks, Eric, will try a new tack... https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column.py File gm/rebaseline_server/column.py (right): https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column.py#newcode2 gm/rebaseline_server/column.py:2: On 2014/05/08 ...

6 years, 7 months ago (2014-05-08 15:04:35 UTC) #3

Thanks, Eric, will try a new tack...

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

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/column....
gm/rebaseline_server/column.py:2: 
On 2014/05/08 12:40:18, borenet wrote:
> On 2014/05/07 21:26:17, epoger wrote:
> > Reason for the CL: the fact that the rebaseline_server JSON included
arbitrary
> > field values as dictionary keys limited which fields we could search and
sort
> > on.  See http://skbug.com/1758 for more details.
> > 
> > This is a different approach than I had originally envisioned in the bug ,
but
> > as I started on the implementation I decided this approach was better, and
> > should resolve the same root problem.
> 
> Top-level comment about parsing JSON: I really like the way it's done in Go. 
> You provide a struct which gets filled in by the decoder:
> 
> myData := struct {
>   MyField:      string
>   MyOtherField: int
> }{}
> json.Unmarshal(jsonData, &myData)
> 
> You can even specify how to convert the names of the fields from JSON to Go:
> 
> myData := struct {
>   MyField:      string `json:"my_field"`
>   MyOtherField: int    `json:"another_field"`
> }{}
> 
> There's no guarantee that all of the fields of the struct get filled in, or
that
> everything in the JSON data was put into a field.
> 
> I'm just wondering if there might be a way to do something similar in Python
and
> avoid having to use all these constants.

Thanks for pointing out this other way of living.  I agree that it would be
better to access the fields using attribute names (given that the field names
are supposed to be predictable!) rather than arbitrary string keys into a dict.

Python's built-in JSON library is mainly geared towards using dicts with
arbitrary string keys, but there are some solutions that fit object attributes
on top of that.  Best reference I could find so far:
http://stackoverflow.com/questions/6578986/how-to-convert-json-data-into-a-py...

I think that may be worthwhile in the long term; I don't want to tackle it now.

Note: I think we'll need to define the string constants in both Python and
Javascript anyway, since the client and server need to agree on them.

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepairset_test.py (right):

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepairset_test.py:95: 'values': ['MyBuilder'],
On 2014/05/08 12:40:18, borenet wrote:
> My concern with this format is that it's possible for the two lists to be out
of
> sync.  What if we used tuples or lists to create pairs:
> 
> 'valuesAndCounts': [
>   ('MyBuilder', 3),
> ]

JSON doesn't have a concept of tuples, but I could use a list of 2-item lists. 
I'll try that and see how it goes.  As you note, that will inherently guard
against out-of-sync lists.

Thanks for the suggestion.  It could make things a bit more complicated on the
client side, but we'll see.

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/static/...
File gm/rebaseline_server/static/constants.js (right):

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/static/...
gm/rebaseline_server/static/constants.js:45: // NOTE: Keep these in sync with
../results.py
On 2014/05/08 12:40:18, borenet wrote:
> Yikes.

The good news is that the comments pointing both ways make the sync process
reasonably easy.

It would be cool to automate it, but I don't anticipate getting to that soon...

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

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/utils.p...
gm/rebaseline_server/utils.py:7: found in the LICENSE file.
On 2014/05/08 12:40:18, borenet wrote:
> I've been copy/pasting the same header, which reads:
> 
> # Copyright (c) 2013 The Chromium Authors. All rights reserved.
> # Use of this source code is governed by a BSD-style license that can be
> # found in the LICENSE file.

The most recent directive I have on what the copyright headers should look like
is https://b.corp.google.com/issue?id=5072851 ('write/run script to
automatically update license terms in all Skia source files'), which is in turn
based on a 2011 email from Mike.

Is there a more recent directive I should be following?

https://codereview.chromium.org/270413002/diff/1/gm/rebaseline_server/utils.p...
gm/rebaseline_server/utils.py:14: within the lists can be used to connect key
and value.
On 2014/05/08 12:40:18, borenet wrote:
> It seems like this should be a built-in function.  I guess there's no
guarantee
> that the following code would give you correct orderings?
> 
> return dict.keys(), dict.values()

Right, no guarantee of consistent ordering.  Also vulnerable to multithreading
errors (if dict changes between those two calls).

Actually, what I have already written is already vulnerable to multithreading
errors, in case a key disappears from input_dict while we're in the loop.

> 
> If the results don't need to be sorted, this would be faster, since there's no
> copy:
> for k, v in input_dict.iteritems():
>   keys.append(k)
>   values.append(v)

At some point before the results are presented to the user they should be
sorted.  This seemed as good a place as any (why wait until the client side?). 
I'll see how I can do that in the list-of-tuples model.

epoger

Eric, PTAL at patchset 2. Thanks for the idea; I think this is better.

6 years, 7 months ago (2014-05-08 18:40:30 UTC) #4

commit-bot: I haz the power

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

6 years, 7 months ago (2014-05-08 21:15:04 UTC) #7

Message was sent while issue was closed.

Change committed as 14658

Expand Messages | Collapse Messages