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

minimal working python 3 support #3

Merged
merged 3 commits into from
Nov 22, 2015

Conversation

glindstedt
Copy link
Contributor

I wanted python 3 support for this library and started porting myself before seeing #2. However after seeing the pull request I incorporated some of the solutions in to my own, since @F483 had solved them in a better way.

However considering that #2 contains many other changes not strictly related to python 3 support I can understand why it has not been merged yet. This is an effort to separate out only the changes necessary for python 3 support, which might be easier to justify for a merge.

If you only want to consider the #2 pull request for merging, and feel that this is redundant, then feel free to close this pull request.

@glindstedt
Copy link
Contributor Author

Update: Don't consider merging this yet. I discovered a bug that was not triggered by example.py but was triggered by my self-ported python3 version of the kademlia module. Not sure yet if the bug is inherent to this branch of rpcudp or to my ported version of kademlia.

@bmuller
Copy link
Owner

bmuller commented Oct 26, 2015

Thanks @glindste - I've been traveling (and will be for the next week or so) - but when I get back I'll take a look. Thanks for the help!

@glindstedt
Copy link
Contributor Author

@bmuller Ok, happy travelling! I've found the bug to be due to umsgpack treating strings differently in python 2 and python 3 when packing. I'll have pushed a commit dealing with it by the time you get back.

Import str from builtins provided by the future package. Wrap function
name in str() when packing to ensure that it is packed as a unicode
string by umsgpack. If not a python3 node might receive a bytes object
when unpacking the function name and fail to match it.
@glindstedt
Copy link
Contributor Author

For the latest commit, see https://github.com/vsergeev/u-msgpack-python#behavior-notes for a description how u-msgpack packs strings in python 2 and 3. By making sure to use a unicode encoded string type in python 2, we can be sure that the method name will be packed the same in python 2 and 3. Unfortunately this brings in future as an additional dependency, however I think it's better to solve this in a standard way instead of adding complexity by solving it with code.

@bmuller
Copy link
Owner

bmuller commented Oct 29, 2015

Thanks @glindste! Will those changes mean that new versions of this lib won't be able to communicate with old versions (given that msgpack behavior)?

@glindstedt
Copy link
Contributor Author

@bmuller that's a good question, I hadn't considered it before. I'm confident that an instance of the new version on python 3 would face problems communicating with an old instance running python 2. In that case 'funcname' in line 59 will be a 'bytes' object and fail to match on the python 3 instance, which was what happened in the first place and led me to make that change.

However I'm not completely sure of the case where both the new and the old instance are running on python 2. In the case of old->new, the new instance would receive an old-style python 2 string which would probably still be evaluated correctly. Presumably new->old would also work since the old version would receive a 'unicode' object, which should also work. I could try to test this if you want.

I assume that this kind of behaviour is unacceptable for a minor version bump. An alternative would be to keep the old functionality for now and for python 3 instances manually detect and cast 'funcname' from 'bytes' to 'str', and save the change in behaviour for a major version bump.

@bmuller
Copy link
Owner

bmuller commented Nov 5, 2015

OK - sounds like the safest thing is a major version bump. If you want to make that a part of this PR (along with a notice in the README that v1 won't work w/ v2), then I think it's ready to merge. Thanks @glindste!

@glindstedt
Copy link
Contributor Author

@bmuller Sorry I haven't replied, I've been a bit busy lately. Sounds good! I'll bump the version and write about the incompatibility in the README. You sure there's nothing else that breaks compatibility that you'd like to change?

@bmuller
Copy link
Owner

bmuller commented Nov 18, 2015

@glindste there's some final testing I'll probably do before pushing to pypi after you update the PR - but I think that should be it. Thanks!

@glindstedt
Copy link
Contributor Author

@bmuller Alright! Pushed now, let me know if something needs changing.

@F483
Copy link

F483 commented Nov 18, 2015

Hi I've been busy and its been a month since I started a python 3 port (see #2). Is this complete and do python 2 and python 2 clients interoperate properly (not the case in my pull request).

In short should I drop my pull request and take a look at this?

@glindstedt
Copy link
Contributor Author

@F483 From what I've tested Python 2 and Python 3 clients can interoperate. The main issue I found for interoperability was that u-msgpack-python packs Python 2 strings and Python 3 strings differently, which is the reason for making sure that the method names are always packed as Unicode strings.

However, any client application using rpcudp with both Python 2 and Python 3 clients still needs to make sure that all arguments that are sent are explicitly typed so that they will pack and unpack in the same way for both versions.

Any feedback is appreciated if you have the time!

@F483
Copy link

F483 commented Nov 19, 2015

Yes python 2 vs python 3 strings have been a pain. Relying on applications using it is ok but will very likely be forgotten ...

It may be worth having the data sanitized by default so that all strings are unicode json.loads(json.dumps(data))

@glindstedt
Copy link
Contributor Author

@F483 that's not a bad idea actually. Looking at json decoding in Python 2 and Python 3 the only ambiguity left when decoding is for large integers. However a client application that is designed to handle large numbers and run on Python 2 and Python 3 should already have decided to use either the new backported int type or long from past.builtins. Any thoughts @bmuller?

@bmuller
Copy link
Owner

bmuller commented Nov 22, 2015

I think that's fine. If everything looks good to you @glindste and @F483 - I'll do some final testing (esp between a clients running Python 2 and another on Python 3) and then push this version. Sound good?

@bmuller
Copy link
Owner

bmuller commented Nov 22, 2015

Testing was super smooth - everything looks good to me. I'm going to merge and do a release. Thanks!

bmuller added a commit that referenced this pull request Nov 22, 2015
minimal working python 3 support, bumped version to 2.0
@bmuller bmuller merged commit 1d6e52f into bmuller:master Nov 22, 2015
@glindstedt glindstedt deleted the python3-support branch November 23, 2015 10:31
@F483
Copy link

F483 commented Dec 4, 2015

@glindste using json.loads(json.dumps(data)) is actually not a good idea. I explicitly pass bytes to ensure small udp packets, which umsgpack supports. Filtering through json would break this and require me to encode, which would lead to larger packages.

@glindstedt
Copy link
Contributor Author

@F483 yes you're right, it would just create new problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants