-
Notifications
You must be signed in to change notification settings - Fork 167
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
Drop annotation of dynamic classes when pickling for Python < 3.7 #348
Drop annotation of dynamic classes when pickling for Python < 3.7 #348
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by the relationship between the new test and the code change:
''' | ||
cmd = [sys.executable, '-c', code.format(protocol=self.protocol)] | ||
proc = subprocess.Popen( | ||
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE | |
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE |
depickled_f = pickle_depickle(f, protocol=self.protocol) | ||
assert depickled_f.__annotations__ == {} | ||
|
||
@unittest.skipIf(sys.version_info >= (3, 7) or sys.version_info < (3, 6), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is really hard to parse. It should be equivalent to:
@unittest.skipIf(sys.version_info[:2] != (3, 6))
but then contratdicts the message "pickling annotations is supported starting Python 3.7" because the test is not skipped and passes on Python 3.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests actually test that annotations are properly dropped on Python 3.6.
@@ -581,6 +581,11 @@ def save_dynamic_class(self, obj): | |||
if isinstance(__dict__, property): | |||
type_kwargs['__dict__'] = __dict__ | |||
|
|||
if sys.version_info < (3, 7): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new tests introduced in this PR cover the case where sys.version_info >= (3, 7)
. We should introduce a new test precisely for Python 3.6 no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the new tests cover precisely the case where Python < 3.7
:)
Also the original issue was about Python 3.6 and non of the new tests cover this case. |
However I tried the fix in this PR on Python 3.6 and it does actually fix the original reproducer: import typing
import cloudpickle
X = typing.TypeVar("X")
class Input(typing.Generic[X]):
pass
class Lero:
a: Input[int]
obj = Lero()
cloudpickle.dumps(obj) |
Some remarks:
|
Sorry I was confused with double negations... |
No worries, I hate double negations too :) |
Drop annotation of dynamic classes when pickling for Python < 3.7 (cloudpipe#348)
fixes #347
cc @ogrisel