Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle buffers in Widget._should_send_property #1595

Merged
merged 9 commits into from
Aug 9, 2017
Merged

Handle buffers in Widget._should_send_property #1595

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6445ea9
Fix change test in _should_send_property
vidartf Aug 6, 2017
afe6ae6
Add tests for Widget.set_state
vidartf Aug 6, 2017
40cd07c
Fix comment typos
vidartf Aug 6, 2017
349f556
Add test for when the serializer returns bytes.
jasongrout Aug 9, 2017
db04817
Fix the property lock when the deserializer returns bytes or buffer
jasongrout Aug 9, 2017
4d912e4
Fix test for python 2
jasongrout Aug 9, 2017
7d56205
Change the binary types for custom serializers to memoryview, bytearr…
jasongrout Aug 9, 2017
b9eb8c8
Factor out json and buffer list compare.
jasongrout Aug 9, 2017
0f0761c
Compare without copying if possible.
jasongrout Aug 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ipywidgets/widgets/tests/test_set_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,4 @@ def test_set_state_data_truncate():

# Sanity:
assert len(buffers) == 1
assert buffers[0].tobytes() == data[:20].tobytes()
assert buffers[0] == data[:20].tobytes()
15 changes: 9 additions & 6 deletions ipywidgets/widgets/widget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ def _compare(self, a, b):
else:
return a == b

def _compare_buffers(self, a, b, key):
def _buffer_list_equal(self, a, b):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have this as an extension point, keeping the key could be very informative as to which kind of comparison to perform. If anything, it's a problem with the current _compare that it does not have it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. for certain keys you might want to do a floating point comparison with a given precision, while for another you need exact equality. Without the key, you will not be able to determine which comparison to apply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on this being an extension point? I don't understand how you're thinking of this as an extension point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean extension point as in "something an inheriting class might want to override".

Copy link
Member

@jasongrout jasongrout Aug 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this comparison depends on the top-level put/remove buffer function implementations, so I factored out the comparison to also be a top-level function.

Copy link
Member

@jasongrout jasongrout Aug 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean extension point as in "something an inheriting class might want to override".

Ah. Since it depends on the implementation of the top-level functions (i.e., what exactly can be in the buffers list), I think it makes more sense to not override it, and instead put it at the top level too.

Copy link
Member Author

@vidartf vidartf Aug 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation works for me, but if someone uses a serializer/deserialize set that might introduce noise when data is passed through a loop (a != serializer(deserialize(a))), e.g. floating point inaccuracies, they will want to override this comparison.

It is still possible to do that by overriding the _should_send_property function, but it will be more brittle as it needs to take into account the internals of that method (e.g. _property_lock).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I might be over-engineering this one. Other fields in the state other than buffers might also have this issue, so there is no obvious reason to give buffers preferential treatment.

"""Compare two lists of buffers for equality.

Used to decide whether two sequences of buffers (memoryviews) differ,
Expand All @@ -501,7 +501,9 @@ def _compare_buffers(self, a, b, key):
# e.g. memoryview(np.frombuffer(ia, dtype='float32')) !=
# memoryview(np.frombuffer(b)), since the format info differs.
# However, since we only transfer bytes, we use `tobytes()`.
if ia.tobytes() != ib.tobytes():
iabytes = ia.tobytes() if isinstance(ia, memoryview) else ia
ibbytes = ib.tobytes() if isinstance(ib, memoryview) else ib
if ia != ib:
return False
return True

Expand Down Expand Up @@ -620,13 +622,14 @@ def _should_send_property(self, key, value):
# idiosyncracies of how python data structures map to json, for example
# tuples get converted to lists.
if key in self._property_lock:
# model_state, buffer_paths, buffers
split_value = _remove_buffers({ key: to_json(value, self)})
split_lock = _remove_buffers({ key: self._property_lock[key]})
# Compare state and buffer_paths
if jsondumps(split_value[:2]) == jsondumps(split_lock[:2]):
# Compare buffers
if self._compare_buffers(split_value[2], split_lock[2], key):
return False
if (jsonloads(jsondumps(split_value[0])) == jsonloads(jsondumps(split_lock[0]))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather pass sort_keys=True to dumps rather than pass it back through loads, but it is not critical and I have not profiled the two.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also factored out this comparison into a top-level function.

and split_value[1] == split_lock[1]
and self._buffer_list_equal(split_value[2], split_lock[2])):
return False
if self._holding_sync:
self._states_to_send.add(key)
return False
Expand Down