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.

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

Conversation

@vidartf
Copy link
Member

commented Aug 6, 2017

Previous check in _should_send_property() did not handle buffers. After this change, the comparison is performed in the serialized state which also prevents creation of new widgets/instances during comparison.

Fixes #1592.

vidartf added some commits Aug 6, 2017

Fix change test in _should_send_property
Previous test did not handle buffers.. After this change, the comparison
is performed in the serialized state which also prevents creation of new
widgets/instances during comparison.
Add tests for Widget.set_state
Ensure that Widget.set_state sends the expected update messages

@vidartf vidartf force-pushed the vidartf:fix-should-send branch from f830665 to 40cd07c Aug 6, 2017

@jasongrout jasongrout modified the milestone: 7.0 Aug 7, 2017

@jasongrout jasongrout force-pushed the vidartf:fix-should-send branch from be6df90 to 349f556 Aug 9, 2017

Fix the property lock when the deserializer returns bytes or buffer
Also, fix the case when the state is serialized with keys in a different order, by parsing the json we generated with jsondumps.

@jasongrout jasongrout force-pushed the vidartf:fix-should-send branch from 0dbb6b7 to db04817 Aug 9, 2017

jasongrout added some commits Aug 9, 2017

Fix test for python 2
Also add copyright, change tests to use nose assert for better error messages.
Change the binary types for custom serializers to memoryview, bytearr…
…ay, or python 3 bytes.

We disregard the python 2 buffer type - it is obsolete, even on python 2.

See #1194
@jasongrout

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

@maartenbreddels - the last commit (7d56205) changes the definition of what is a binary object in python for the custom serializers - basically it adds bytearray and gets rid of the python 2 buffer (which is superseded by memoryview, even on python 2).

What do you think?

@jasongrout

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

@vidartf - what do you think of my changes? I feel like this is ready to merge now, so if you and @maartenbreddels approve it, let's merge.

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

LGTM 👍

split_value = _remove_buffers({ key: to_json(value, self)})
split_lock = _remove_buffers({ key: self._property_lock[key]})
# Compare state and buffer_paths
if (jsonloads(jsondumps(split_value[0])) == jsonloads(jsondumps(split_lock[0]))

This comment has been minimized.

Copy link
@vidartf

vidartf Aug 9, 2017

Author Member

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.

This comment has been minimized.

Copy link
@jasongrout

jasongrout Aug 9, 2017

Member

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

@@ -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):

This comment has been minimized.

Copy link
@vidartf

vidartf Aug 9, 2017

Author Member

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.

This comment has been minimized.

Copy link
@vidartf

vidartf Aug 9, 2017

Author Member

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.

This comment has been minimized.

Copy link
@jasongrout

jasongrout Aug 9, 2017

Member

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

This comment has been minimized.

Copy link
@vidartf

vidartf Aug 9, 2017

Author Member

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

This comment has been minimized.

Copy link
@jasongrout

jasongrout Aug 9, 2017

Member

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.

This comment has been minimized.

Copy link
@jasongrout

jasongrout Aug 9, 2017

Member

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.

This comment has been minimized.

Copy link
@vidartf

vidartf Aug 9, 2017

Author Member

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).

This comment has been minimized.

Copy link
@vidartf

vidartf Aug 9, 2017

Author Member

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.

@vidartf

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

Went through it, and only found some minor points commented above. Neither are sticking points for me, so feel free to make the final decision on them as you see fit and merge.

Factor out json and buffer list compare.
Compare deterministic json strings instead of rehydrating them into objects.
@jasongrout

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

One more iteration to try to avoid copying if possible (in python 3, or python 2 with byte memoryviews). @maartenbreddels, @vidartf - does 0f0761c look good to you?

Edit: actually, 0f0761c

@jasongrout jasongrout force-pushed the vidartf:fix-should-send branch from 1c4d8a6 to 0f0761c Aug 9, 2017

@jasongrout

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

@vidartf said on gitter:

@jasongrout Does memoryview.tobytes() actually copy the buffer? I think your changes are ok either way, but just FMI

@jasongrout jasongrout merged commit ee00a61 into jupyter-widgets:master Aug 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vidartf vidartf deleted the vidartf:fix-should-send branch Aug 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.