From 64eba59f3dee3eb29b3cb2d9b1b170b00fbefd6e Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Mon, 13 Nov 2017 19:09:39 +0100 Subject: [PATCH 01/11] More robust non-regression test --- tests/testutils.py | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/tests/testutils.py b/tests/testutils.py index 110e2f78d..4b78f84c8 100644 --- a/tests/testutils.py +++ b/tests/testutils.py @@ -1,7 +1,8 @@ import sys import os -from subprocess import Popen -from subprocess import PIPE +import os.path as op +import tempfile +from subprocess import Popen, check_output, PIPE, STDOUT, CalledProcessError from cloudpickle import dumps from pickle import loads @@ -68,5 +69,43 @@ def pickle_echo(stream_in=None, stream_out=None): stream_out.close() +def assert_run_python_script(source_code, timeout=5): + """Utility to help check pickleability of objects defined in __main__ + + The script provided in the source code should return 0 and not print + anything on stderr or stdout. + """ + fd, source_file = tempfile.mkstemp(suffix='_src_test_cloudpickle.py') + os.close(fd) + try: + with open(source_file, 'wb') as f: + f.write(source_code.encode('utf-8')) + cloudpickle_repo_folder = op.normpath( + op.join(op.dirname(__file__), '..')) + + cmd = [sys.executable, source_file] + pythonpath = "{src}/tests:{src}".format(src=cloudpickle_repo_folder) + kwargs = { + 'cwd': cloudpickle_repo_folder, + 'stderr': STDOUT, + 'env': {'PYTHONPATH': pythonpath}, + } + if timeout_supported: + kwargs['timeout'] = timeout + try: + try: + out = check_output(cmd, **kwargs) + except CalledProcessError as e: + raise RuntimeError(u"script errored with output:\n%s" + % e.output.decode('utf-8')) + if out != b"": + raise AssertionError(out.decode('utf-8')) + except TimeoutExpired as e: + raise RuntimeError(u"script timeout, output so far:\n%s" + % e.output.decode('utf-8')) + finally: + os.unlink(source_file) + + if __name__ == '__main__': pickle_echo() From fdb46ce61a6d0fd79645bc021791322f5a24a002 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Mon, 13 Nov 2017 20:57:42 +0100 Subject: [PATCH 02/11] Use __file__ as reference instead of os.getcwd() --- tests/testutils.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/testutils.py b/tests/testutils.py index 4b78f84c8..ba2cab01c 100644 --- a/tests/testutils.py +++ b/tests/testutils.py @@ -28,10 +28,15 @@ def subprocess_pickle_echo(input_data): [1, 'a', None] """ - pickled_input_data = dumps(input_data) - cmd = [sys.executable, __file__] - cwd = os.getcwd() - proc = Popen(cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE, cwd=cwd) + pickled_input_data = dumps(input_data, protocol=protocol) + cmd = [sys.executable, __file__] # run then pickle_echo() in __main__ + # cwd = os.getcwd() + cloudpickle_repo_folder = op.normpath( + op.join(op.dirname(__file__), '..')) + cwd = cloudpickle_repo_folder + pythonpath = "{src}/tests:{src}".format(src=cloudpickle_repo_folder) + env = {'PYTHONPATH': pythonpath} + proc = Popen(cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE, cwd=cwd, env=env) try: comm_kwargs = {} if timeout_supported: @@ -80,10 +85,9 @@ def assert_run_python_script(source_code, timeout=5): try: with open(source_file, 'wb') as f: f.write(source_code.encode('utf-8')) + cmd = [sys.executable, source_file] cloudpickle_repo_folder = op.normpath( op.join(op.dirname(__file__), '..')) - - cmd = [sys.executable, source_file] pythonpath = "{src}/tests:{src}".format(src=cloudpickle_repo_folder) kwargs = { 'cwd': cloudpickle_repo_folder, From 0522851be15efb413e4021971b7f4a779b6af171 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Mon, 13 Nov 2017 21:56:11 +0100 Subject: [PATCH 03/11] Nicer indentation handling for inline code snippet --- tests/cloudpickle_test.py | 44 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index 5b1bbcb19..66b22dd2e 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -736,6 +736,50 @@ def test_builtin_type__new__(self): for t in list, tuple, set, frozenset, dict, object: self.assertTrue(pickle_depickle(t.__new__) is t.__new__) + def test_interactively_defined_function(self): + # Check that callables defined in the __main__ module of a Python + # script (or jupyter kernel) can be pickled / unpickled / executed. + code = """\ + from testutils import subprocess_pickle_echo + + CONSTANT = 42 + + class Foo(object): + + def method(self, x): + return x + + + def f1(): + return Foo + + + def f2(x): + return Foo().method(x) + + + def f3(): + return Foo().method(CONSTANT) + + + cloned = subprocess_pickle_echo(lambda x: x**2, protocol={protocol}) + assert cloned(3) == 9 + + cloned = subprocess_pickle_echo(Foo, protocol={protocol}) + assert cloned().method(2) == Foo().method(2) + + cloned = subprocess_pickle_echo(f1, protocol={protocol}) + assert cloned()().method('a') == f1()().method('a') + + cloned = subprocess_pickle_echo(f2, protocol={protocol}) + assert cloned(2) == f2(2) + + cloned = subprocess_pickle_echo(f3, protocol={protocol}) + assert cloned() == f3() + """.format(protocol=self.protocol) + code = "\n".join(line[8:] for line in code.splitlines()) + assert_run_python_script(code) + @pytest.mark.skipif(sys.version_info >= (3, 0), reason="hardcoded pickle bytes for 2.7") def test_function_pickle_compat_0_4_0(self): From 97c946f540f4435b2518e6cedecdb8df6512b685 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Mon, 13 Nov 2017 22:05:32 +0100 Subject: [PATCH 04/11] cleanup --- tests/testutils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/testutils.py b/tests/testutils.py index ba2cab01c..11cffa77a 100644 --- a/tests/testutils.py +++ b/tests/testutils.py @@ -30,7 +30,6 @@ def subprocess_pickle_echo(input_data): """ pickled_input_data = dumps(input_data, protocol=protocol) cmd = [sys.executable, __file__] # run then pickle_echo() in __main__ - # cwd = os.getcwd() cloudpickle_repo_folder = op.normpath( op.join(op.dirname(__file__), '..')) cwd = cloudpickle_repo_folder From 7a87c8dc05aa988fccca80c677f6613e797a3621 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Mon, 13 Nov 2017 23:10:11 +0100 Subject: [PATCH 05/11] More comprehensive interactive function tests --- tests/cloudpickle_test.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index 66b22dd2e..eeb8f68ac 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -749,25 +749,30 @@ class Foo(object): def method(self, x): return x + def f0(x): + return x ** 2 def f1(): return Foo - def f2(x): return Foo().method(x) - def f3(): return Foo().method(CONSTANT) - cloned = subprocess_pickle_echo(lambda x: x**2, protocol={protocol}) assert cloned(3) == 9 + cloned = subprocess_pickle_echo(f0, protocol={protocol}) + assert cloned(3) == 9 + cloned = subprocess_pickle_echo(Foo, protocol={protocol}) assert cloned().method(2) == Foo().method(2) + cloned = subprocess_pickle_echo(Foo(), protocol={protocol}) + assert cloned.method(2) == Foo().method(2) + cloned = subprocess_pickle_echo(f1, protocol={protocol}) assert cloned()().method('a') == f1()().method('a') @@ -777,8 +782,7 @@ def f3(): cloned = subprocess_pickle_echo(f3, protocol={protocol}) assert cloned() == f3() """.format(protocol=self.protocol) - code = "\n".join(line[8:] for line in code.splitlines()) - assert_run_python_script(code) + assert_run_python_script(textwrap.dedent(code)) @pytest.mark.skipif(sys.version_info >= (3, 0), reason="hardcoded pickle bytes for 2.7") From dc35b47bb1cf98fc7184bd786cda179172947887 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Mon, 13 Nov 2017 23:11:00 +0100 Subject: [PATCH 06/11] Fix interactive function pickling --- cloudpickle/cloudpickle.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cloudpickle/cloudpickle.py b/cloudpickle/cloudpickle.py index 45cffe5ba..0083d6855 100644 --- a/cloudpickle/cloudpickle.py +++ b/cloudpickle/cloudpickle.py @@ -621,11 +621,15 @@ def save_global(self, obj, name=None, pack=struct.pack): dispatched here. """ try: - return Pickler.save_global(self, obj, name=name) + if obj.__module__ == "__main__": + raise ValueError("%r is defined in __main__" % obj) + else: + return Pickler.save_global(self, obj, name=name) except Exception: if obj.__module__ == "__builtin__" or obj.__module__ == "builtins": if obj in _BUILTIN_TYPE_NAMES: - return self.save_reduce(_builtin_type, (_BUILTIN_TYPE_NAMES[obj],), obj=obj) + return self.save_reduce( + _builtin_type, (_BUILTIN_TYPE_NAMES[obj],), obj=obj) typ = type(obj) if typ is not obj and isinstance(obj, (type, types.ClassType)): From 93dfd39c60e6b470a897d80bbf03417cab390a67 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Tue, 14 Nov 2017 08:38:26 +0100 Subject: [PATCH 07/11] Simpler fix --- cloudpickle/cloudpickle.py | 8 ++++---- tests/cloudpickle_test.py | 8 ++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/cloudpickle/cloudpickle.py b/cloudpickle/cloudpickle.py index 0083d6855..f429a58d1 100644 --- a/cloudpickle/cloudpickle.py +++ b/cloudpickle/cloudpickle.py @@ -620,11 +620,11 @@ def save_global(self, obj, name=None, pack=struct.pack): The name of this method is somewhat misleading: all types get dispatched here. """ + if obj.__module__ == "__main__": + return self.save_dynamic_class(obj) + try: - if obj.__module__ == "__main__": - raise ValueError("%r is defined in __main__" % obj) - else: - return Pickler.save_global(self, obj, name=name) + return Pickler.save_global(self, obj, name=name) except Exception: if obj.__module__ == "__builtin__" or obj.__module__ == "builtins": if obj in _BUILTIN_TYPE_NAMES: diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index eeb8f68ac..12b54abcf 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -749,6 +749,8 @@ class Foo(object): def method(self, x): return x + foo = Foo() + def f0(x): return x ** 2 @@ -761,6 +763,9 @@ def f2(x): def f3(): return Foo().method(CONSTANT) + def f4(x): + return foo.method(x) + cloned = subprocess_pickle_echo(lambda x: x**2, protocol={protocol}) assert cloned(3) == 9 @@ -781,6 +786,9 @@ def f3(): cloned = subprocess_pickle_echo(f3, protocol={protocol}) assert cloned() == f3() + + cloned = subprocess_pickle_echo(f4, protocol={protocol}) + assert cloned(2) == f4(2) """.format(protocol=self.protocol) assert_run_python_script(textwrap.dedent(code)) From 1ce55f1cfcde8a909e240f7a14779ecd43b4a468 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Tue, 14 Nov 2017 19:32:13 +0100 Subject: [PATCH 08/11] Further simplification of save_global --- cloudpickle/cloudpickle.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cloudpickle/cloudpickle.py b/cloudpickle/cloudpickle.py index f429a58d1..ded2bd227 100644 --- a/cloudpickle/cloudpickle.py +++ b/cloudpickle/cloudpickle.py @@ -631,11 +631,7 @@ def save_global(self, obj, name=None, pack=struct.pack): return self.save_reduce( _builtin_type, (_BUILTIN_TYPE_NAMES[obj],), obj=obj) - typ = type(obj) - if typ is not obj and isinstance(obj, (type, types.ClassType)): - return self.save_dynamic_class(obj) - - raise + return self.save_dynamic_class(obj) dispatch[type] = save_global dispatch[types.ClassType] = save_global From b146e506a82e06023c25bdb007848a156d4e0160 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Wed, 15 Nov 2017 16:59:27 +0100 Subject: [PATCH 09/11] Add entry to CHANGES.md --- CHANGES.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index abbcd0dfe..1b5d66ee0 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,10 @@ +0.4.3 +===== + +- Fixed a regression: `AttributeError` when loading pickles that hold a + reference to a dynamically defined class from the `__main__` module. + ([issue #131]( https://github.com/cloudpipe/cloudpickle/issues/131)). + 0.4.2 ===== From 98c1455349c00b1049093f9528e21b48de8ca360 Mon Sep 17 00:00:00 2001 From: Olivier Grisel Date: Wed, 15 Nov 2017 17:37:42 +0100 Subject: [PATCH 10/11] Revert "Further simplification of save_global" This reverts commit d9e02fbdfcd52da1e1020f73e8e1a10b82b60f04. --- cloudpickle/cloudpickle.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cloudpickle/cloudpickle.py b/cloudpickle/cloudpickle.py index ded2bd227..f429a58d1 100644 --- a/cloudpickle/cloudpickle.py +++ b/cloudpickle/cloudpickle.py @@ -631,7 +631,11 @@ def save_global(self, obj, name=None, pack=struct.pack): return self.save_reduce( _builtin_type, (_BUILTIN_TYPE_NAMES[obj],), obj=obj) - return self.save_dynamic_class(obj) + typ = type(obj) + if typ is not obj and isinstance(obj, (type, types.ClassType)): + return self.save_dynamic_class(obj) + + raise dispatch[type] = save_global dispatch[types.ClassType] = save_global From 3cfc30396ba05555ff5bfb8a337837a7a6030dd3 Mon Sep 17 00:00:00 2001 From: hyukjinkwon Date: Mon, 5 Feb 2018 12:51:29 +0900 Subject: [PATCH 11/11] Fix few missed corrections --- tests/cloudpickle_test.py | 19 ++++++++++--------- tests/testutils.py | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/cloudpickle_test.py b/tests/cloudpickle_test.py index 12b54abcf..2a73786c4 100644 --- a/tests/cloudpickle_test.py +++ b/tests/cloudpickle_test.py @@ -44,6 +44,7 @@ from cloudpickle.cloudpickle import _find_module, _make_empty_cell, cell_set from .testutils import subprocess_pickle_echo +from .testutils import assert_run_python_script HAVE_WEAKSET = hasattr(weakref, 'WeakSet') @@ -766,30 +767,30 @@ def f3(): def f4(x): return foo.method(x) - cloned = subprocess_pickle_echo(lambda x: x**2, protocol={protocol}) + cloned = subprocess_pickle_echo(lambda x: x**2) assert cloned(3) == 9 - cloned = subprocess_pickle_echo(f0, protocol={protocol}) + cloned = subprocess_pickle_echo(f0) assert cloned(3) == 9 - cloned = subprocess_pickle_echo(Foo, protocol={protocol}) + cloned = subprocess_pickle_echo(Foo) assert cloned().method(2) == Foo().method(2) - cloned = subprocess_pickle_echo(Foo(), protocol={protocol}) + cloned = subprocess_pickle_echo(Foo()) assert cloned.method(2) == Foo().method(2) - cloned = subprocess_pickle_echo(f1, protocol={protocol}) + cloned = subprocess_pickle_echo(f1) assert cloned()().method('a') == f1()().method('a') - cloned = subprocess_pickle_echo(f2, protocol={protocol}) + cloned = subprocess_pickle_echo(f2) assert cloned(2) == f2(2) - cloned = subprocess_pickle_echo(f3, protocol={protocol}) + cloned = subprocess_pickle_echo(f3) assert cloned() == f3() - cloned = subprocess_pickle_echo(f4, protocol={protocol}) + cloned = subprocess_pickle_echo(f4) assert cloned(2) == f4(2) - """.format(protocol=self.protocol) + """ assert_run_python_script(textwrap.dedent(code)) @pytest.mark.skipif(sys.version_info >= (3, 0), diff --git a/tests/testutils.py b/tests/testutils.py index 11cffa77a..6a50f9732 100644 --- a/tests/testutils.py +++ b/tests/testutils.py @@ -28,7 +28,7 @@ def subprocess_pickle_echo(input_data): [1, 'a', None] """ - pickled_input_data = dumps(input_data, protocol=protocol) + pickled_input_data = dumps(input_data) cmd = [sys.executable, __file__] # run then pickle_echo() in __main__ cloudpickle_repo_folder = op.normpath( op.join(op.dirname(__file__), '..'))