Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641583002:
[Cuprit-finder] Add MetaDict class. (Closed)
|
Created:
3 years, 11 months ago by Sharu Jiang Modified:
3 years, 11 months ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, wrengr, inferno Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, ``MetaDictSerializer`` can serialize it to [e(1), e(2), e(3)] and de-serialize it back to {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}
BUG=679097
TBR=wrengr@chromium.org
Review-Url: https://codereview.chromium.org/2641583002
Committed: https://chromium.googlesource.com/infra/infra/+/0ad9be3160f568da7c068d4e569e2f999f7cad75
Patch Set 1 : . #
Total comments: 17
Patch Set 2 : Fix nits. #
Total comments: 6
Patch Set 3 : Fix nits. #
Total comments: 2
Patch Set 4 : Address comment. #
Total comments: 5
Patch Set 5 : Add module docs. #Patch Set 6 : Update doc. #
Dependent Patchsets: Messages
Total messages: 31 (13 generated)
Description was changed from
==========
[Predator] Add meta object.
BUG=679097
==========
to
==========
[Predator] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, and it can be
serialized to [e(1), e(2), e(3)].
BUG=679097
==========
katesonia@chromium.org changed reviewers: + mbarbella@google.com, stgao@chromium.org
PTAL :)
Description was changed from
==========
[Predator] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, and it can be
serialized to [e(1), e(2), e(3)].
BUG=679097
==========
to
==========
[Predator] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, and it can be
serialized to [e(1), e(2), e(3)].
BUG=679097
TBR=wrengr@chromium.org
==========
Description was changed from
==========
[Predator] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, and it can be
serialized to [e(1), e(2), e(3)].
BUG=679097
TBR=wrengr@chromium.org
==========
to
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, and it can be
serialized to [e(1), e(2), e(3)].
BUG=679097
TBR=wrengr@chromium.org
==========
Patchset #1 (id:1) has been deleted
katesonia@chromium.org changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org, mbarbella@chromium.org - mbarbella@google.com, stgao@chromium.org
PTAL.
At a glance this seems a bit more complicated than it has to be. Is there something I can look at for context on how this will be used?
https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:113: # Trucate the segment in the element list to construct Nit: 2 spaces between 'the' and 'segment' https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:113: # Trucate the segment in the element list to construct Nit:Truncate? https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:117: meta_objs[key] = serializer.FromList(segment, element_constructor) This means all elements in the dict must use the same element_constructor? https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:150: constructor (callable or None): Callable with dict as input and returns a constructor is not in argument list? https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... File appengine/findit/libs/meta_object.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:3: # found in the LICENSE file. A comment of this file in general: I think it'll be better if you call this file meta_dict rather than meta_object? https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:7: """Class that can be eight one element of a collection of elements.""" I don't understand the docstring... Could you explain? https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:31: def IsElement(self): How about change this to a parameter function? https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/t... File appengine/findit/libs/test/meta_object_test.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/t... appengine/findit/libs/test/meta_object_test.py:33: d['a'] = 9 Without line 33, what'll be the value of d['a']? It seems to me that d['a'] will be 9 still? If so, is this expected?
On 2017/01/18 20:08:03, Martin Barbella wrote: > At a glance this seems a bit more complicated than it has to be. Is there > something I can look at for context on how this will be used? There is a bug of the context of this - https://bugs.chromium.org/p/chromium/issues/detail?id=679097 The context is there are some feature that are can share code, for example ``TouchCrashedFile``, ``MinDistance``, ``TopFrameIndex`` they are all related to matching crashed file and changed file, so we can use a Meta feature to group this 3.
On 2017/01/18 20:08:03, Martin Barbella wrote: > At a glance this seems a bit more complicated than it has to be. Is there > something I can look at for context on how this will be used? I also pushed https://codereview.chromium.org/2625073003/, this cl includes more context. MetaWeight and MetaFeature can make the code cleaner.
https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:113: # Trucate the segment in the element list to construct On 2017/01/18 22:14:23, chanli wrote: > Nit:Truncate? Done. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:113: # Trucate the segment in the element list to construct On 2017/01/18 22:14:23, chanli wrote: > Nit: 2 spaces between 'the' and 'segment' Done. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:117: meta_objs[key] = serializer.FromList(segment, element_constructor) On 2017/01/18 22:14:23, chanli wrote: > This means all elements in the dict must use the same element_constructor? Yes. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:150: constructor (callable or None): Callable with dict as input and returns a On 2017/01/18 22:14:23, chanli wrote: > constructor is not in argument list? Oops, forgot to delete it. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... File appengine/findit/libs/meta_object.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:3: # found in the LICENSE file. On 2017/01/18 22:14:23, chanli wrote: > A comment of this file in general: I think it'll be better if you call this file > meta_dict rather than meta_object? The ``MetaDict`` and ``Element`` are all ``MetaObject``, so I think the meta_object should be better. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:7: """Class that can be eight one element of a collection of elements.""" On 2017/01/18 22:14:23, chanli wrote: > I don't understand the docstring... Could you explain? Sorry, 2 typos... Corrected. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:31: def IsElement(self): On 2017/01/18 22:14:24, chanli wrote: > How about change this to a parameter function? You mean property? Done. https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/t... File appengine/findit/libs/test/meta_object_test.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/t... appengine/findit/libs/test/meta_object_test.py:33: d['a'] = 9 On 2017/01/18 22:14:24, chanli wrote: > Without line 33, what'll be the value of d['a']? It seems to me that d['a'] will > be 9 still? If so, is this expected? This won't affect the test, since the test just want to check if the __setitem__ works as expected, but maybe using deepcopy would be cleaner.
https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/20001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:117: meta_objs[key] = serializer.FromList(segment, element_constructor) On 2017/01/19 00:00:52, Sharu Jiang wrote: > On 2017/01/18 22:14:23, chanli wrote: > > This means all elements in the dict must use the same element_constructor? > > Yes. So I assume this is expected behavior. https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:92: element_constructor=None): Nit: The argument list can be in 2 lines. https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... File appengine/findit/libs/test/meta_object_test.py (right): https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... appengine/findit/libs/test/meta_object_test.py:30: meta_dict = MetaDict(copy.deepcopy(d)) How about use deepcopy in MetaDict instead?
https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... File appengine/findit/libs/test/meta_dict_serializer_test.py (right): https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... appengine/findit/libs/test/meta_dict_serializer_test.py:59: if meta_object != other[key]: is key guaranteed to be in other? if not should this be other.get(key)?
https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:92: element_constructor=None): On 2017/01/19 02:10:58, chanli wrote: > Nit: The argument list can be in 2 lines. Done. https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... File appengine/findit/libs/test/meta_dict_serializer_test.py (right): https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... appengine/findit/libs/test/meta_dict_serializer_test.py:59: if meta_object != other[key]: On 2017/01/19 20:07:14, lijeffrey wrote: > is key guaranteed to be in other? if not should this be other.get(key)? Done. https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... File appengine/findit/libs/test/meta_object_test.py (right): https://codereview.chromium.org/2641583002/diff/40001/appengine/findit/libs/t... appengine/findit/libs/test/meta_object_test.py:30: meta_dict = MetaDict(copy.deepcopy(d)) On 2017/01/19 02:10:58, chanli wrote: > How about use deepcopy in MetaDict instead? Done.
Description was changed from
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}, and it can be
serialized to [e(1), e(2), e(3)].
BUG=679097
TBR=wrengr@chromium.org
==========
to
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}},
``MetaDictSerializer`` can be serialize it to [e(1), e(2), e(3)] and
de-serialize it back to {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}
BUG=679097
TBR=wrengr@chromium.org
==========
https://codereview.chromium.org/2641583002/diff/60001/appengine/findit/libs/m... File appengine/findit/libs/meta_object.py (right): https://codereview.chromium.org/2641583002/diff/60001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:30: self._value = value So what I meant in meta_object_test.py is that I don't know if you should use deepcopy here? Otherwise will the operation on a MetaDict object affect the value dict as well? Or is it expected?
https://codereview.chromium.org/2641583002/diff/60001/appengine/findit/libs/m... File appengine/findit/libs/meta_object.py (right): https://codereview.chromium.org/2641583002/diff/60001/appengine/findit/libs/m... appengine/findit/libs/meta_object.py:30: self._value = value On 2017/01/22 00:07:20, chanli wrote: > So what I meant in meta_object_test.py is that I don't know if you should use > deepcopy here? Otherwise will the operation on a MetaDict object affect the > value dict as well? Or is it expected? Good point, it is safer to make deepcopy here.
lgtm
https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:11: class ElementSerializer(Element): To leave some more specific feedback building on my last comment, it's really hard for me to take a quick look at this and conceptualize what an Element is, and why we need an ElementSerializer, what serialization will do. Basically, this seems to add a lot of complexity. I'd imagine there's a simpler, or at least clearer, way we can do what we're trying to do here. I'm having trouble picturing exactly what this should look like, but I don't think this is it. Happy to meet to discuss ideas if it would help. https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:49: assert len(element_list) == len(self), Exception( It'd be nice to see a less generic exception class, but if this is the practice throughout the code I wouldn't recommend changing it in this file only. Maybe a cleanup for another time.
Description was changed from
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}},
``MetaDictSerializer`` can be serialize it to [e(1), e(2), e(3)] and
de-serialize it back to {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}
BUG=679097
TBR=wrengr@chromium.org
==========
to
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}},
``MetaDictSerializer`` can serialize it to [e(1), e(2), e(3)] and de-serialize
it back to {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}
BUG=679097
TBR=wrengr@chromium.org
==========
lgtm https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:11: class ElementSerializer(Element): On 2017/01/23 22:21:16, Martin Barbella wrote: > To leave some more specific feedback building on my last comment, it's really > hard for me to take a quick look at this and conceptualize what an Element is, > and why we need an ElementSerializer, what serialization will do. Basically, > this seems to add a lot of complexity. > > I'd imagine there's a simpler, or at least clearer, way we can do what we're > trying to do here. I'm having trouble picturing exactly what this should look > like, but I don't think this is it. Happy to meet to discuss ideas if it would > help. Discussed offline. It's fine to land this CL pretty much as-is, though some comments for why the serializer is necessary and potentially some more example usage in tests will be helpful. It should be fine so long as the usage is easy to understand, and we can make additional changes if that seems not to be the case.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... File appengine/findit/libs/meta_dict_serializer.py (right): https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:11: class ElementSerializer(Element): On 2017/01/23 23:30:11, Martin Barbella wrote: > On 2017/01/23 22:21:16, Martin Barbella wrote: > > To leave some more specific feedback building on my last comment, it's really > > hard for me to take a quick look at this and conceptualize what an Element is, > > and why we need an ElementSerializer, what serialization will do. Basically, > > this seems to add a lot of complexity. > > > > I'd imagine there's a simpler, or at least clearer, way we can do what we're > > trying to do here. I'm having trouble picturing exactly what this should look > > like, but I don't think this is it. Happy to meet to discuss ideas if it would > > help. > > Discussed offline. It's fine to land this CL pretty much as-is, though some > comments for why the serializer is necessary and potentially some more example > usage in tests will be helpful. > > It should be fine so long as the usage is easy to understand, and we can make > additional changes if that seems not to be the case. Added some documents. https://codereview.chromium.org/2641583002/diff/80001/appengine/findit/libs/m... appengine/findit/libs/meta_dict_serializer.py:49: assert len(element_list) == len(self), Exception( On 2017/01/23 22:21:16, Martin Barbella wrote: > It'd be nice to see a less generic exception class, but if this is the practice > throughout the code I wouldn't recommend changing it in this file only. Maybe a > cleanup for another time. This is the practice throughout the code, I filed a bug about cleaning up this.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, mbarbella@chromium.org Link to the patchset: https://codereview.chromium.org/2641583002/#ps140001 (title: "Update doc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485220699726760,
"parent_rev": "6738d1adff43828df49de3c628048bf79316fbcb", "commit_rev":
"0ad9be3160f568da7c068d4e569e2f999f7cad75"}
Message was sent while issue was closed.
Description was changed from
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}},
``MetaDictSerializer`` can serialize it to [e(1), e(2), e(3)] and de-serialize
it back to {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}
BUG=679097
TBR=wrengr@chromium.org
==========
to
==========
[Cuprit-finder] Add MetaDict class.
MetaDict class is the base class that MetaWeight and MetaFeature will inherit.
A simple example is {'a': e(1), 'b': {'c': e(2), 'd': e(3)}},
``MetaDictSerializer`` can serialize it to [e(1), e(2), e(3)] and de-serialize
it back to {'a': e(1), 'b': {'c': e(2), 'd': e(3)}}
BUG=679097
TBR=wrengr@chromium.org
Review-Url: https://codereview.chromium.org/2641583002
Committed:
https://chromium.googlesource.com/infra/infra/+/0ad9be3160f568da7c068d4e569e2...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/infra/infra/+/0ad9be3160f568da7c068d4e569e2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
