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

Teleport functions' keyword-only argument defaults #422

Merged
merged 3 commits into from
Dec 25, 2020

Conversation

Jongy
Copy link
Contributor

@Jongy Jongy commented Dec 19, 2020

Adds support for teleportation of function defaults of the type def f(*, b=5) (keyword-only argument defaults)

Note that the new code I included is not Python 2 compatible. I thought it's okay to do saw because I noticed you have dropped Python 2 support (7afaec2).

Also - this breaks teleportation compatibility across different RPyC versions. Is this a feature we try to maintain? If so, I can see how to add such support.

@Jongy
Copy link
Contributor Author

Jongy commented Dec 19, 2020

Ran teleportation tests on Python 3.6, 3.8

@comrumino comrumino self-requested a review December 24, 2020 00:11
@comrumino comrumino self-assigned this Dec 25, 2020
@comrumino
Copy link
Collaborator

Everything looks pretty good. I'll probably make some minor changes like reducing some accidental complexity I found---funcobj.__kwdefaults__ = {t[0]: t[1] for t in kwdefaults} could be written more cleanly as funcobj.__kwdefaults__ = dict(kwdefaults).

this breaks teleportation compatibility across different RPyC versions
If you are referring to #387, I think it is out of scope. Your changes do fix the unit tests you added:

$ docker exec -it rpyc-3.7 /opt/rpyc/bin/rpyc_classic.py --host 0.0.0.0
$ docker exec -it rpyc-3.6 python -c "exec('def kwdefaults(pos=5, *, a=42, b=\'bye\', c=(12.4, )):\n    return pos, a, b, c\n');import socket;import rpyc;addr = socket.gethostbyname('rpyc-3.7');conn = rpyc.classic.connect(addr, port=18812); remote_kwdefaults = rpyc.utils.classic.teleport_function(conn, kwdefaults); print(remote_kwdefaults(pos=1))"

Thank you for the contribution!

@comrumino comrumino merged commit 28be845 into tomerfiliba-org:master Dec 25, 2020
@Jongy
Copy link
Contributor Author

Jongy commented Dec 25, 2020

Everything looks pretty good. I'll probably make some minor changes like reducing some accidental complexity I found---funcobj.__kwdefaults__ = {t[0]: t[1] for t in kwdefaults} could be written more cleanly as funcobj.__kwdefaults__ = dict(kwdefaults).

Hmm you're right, I forgot dict() can be used this way. Anyway it's not the hot area of RPyC, I guess, so less important to make efficient. For readability's sake, tho...

If you are referring to #387, I think it is out of scope. Your changes do fix the unit tests you added:

$ docker exec -it rpyc-3.7 /opt/rpyc/bin/rpyc_classic.py --host 0.0.0.0
$ docker exec -it rpyc-3.6 python -c "exec('def kwdefaults(pos=5, *, a=42, b=\'bye\', c=(12.4, )):\n    return pos, a, b, c\n');import socket;import rpyc;addr = socket.gethostbyname('rpyc-3.7');conn = rpyc.classic.connect(addr, port=18812); remote_kwdefaults = rpyc.utils.classic.teleport_function(conn, kwdefaults); print(remote_kwdefaults(pos=1))"

I was referring to cross RPyC versions, not cross CPython; Because now export_function & import_function work with tuples of 5 items, but these used to be 4 items, so if I try to teleport a function from RPyC master to the previous tag, I will get an error in import_function for trying to unpack a 5-tuple into 4 names.

I guess it can be easily solved by expecting this ValueError, catching it and calling import_function again without kwdefaults, but like I said, I don't know if cross-version is a goal of RPyC, so I haven't added it in this PR.

Thank you for the contribution!

Thanks for the quick review and merge :)

@Jongy Jongy deleted the teleport-function-kwdefaults branch December 25, 2020 22:02
@comrumino
Copy link
Collaborator

comrumino commented Dec 26, 2020

I released RPyC 5.0.0 today. Per semantic versioning users should expect breaks in backwards compatibility. Of course, I'll add a note in the release description regarding teleport function not being backwards compatibility. Thank you for your work and explanation!

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.

2 participants