Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
1.3.0
=====

- Fix a bug affecting dynamic modules occuring with modified builtins
([issue #316](https://github.com/cloudpipe/cloudpickle/issues/316))

1.2.3
=====

Expand Down
4 changes: 4 additions & 0 deletions cloudpickle/cloudpickle.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
from cStringIO import StringIO
except ImportError:
from StringIO import StringIO
import __builtin__ as builtins
string_types = (basestring,) # noqa
PY3 = False
PY2 = True
Expand All @@ -101,6 +102,7 @@
PY3 = True
PY2 = False
from importlib._bootstrap import _find_spec
import builtins

_extract_code_globals_cache = weakref.WeakKeyDictionary()

Expand Down Expand Up @@ -510,6 +512,7 @@ def save_module(self, obj):
Save a module as an import
"""
if _is_dynamic(obj):
obj.__dict__.pop('__builtins__', None)
self.save_reduce(dynamic_subimport, (obj.__name__, vars(obj)),
obj=obj)
else:
Expand Down Expand Up @@ -1149,6 +1152,7 @@ def subimport(name):
def dynamic_subimport(name, vars):
mod = types.ModuleType(name)
mod.__dict__.update(vars)
mod.__dict__['__builtins__'] = builtins.__dict__
Copy link
Member Author

@pierreglaser pierreglaser Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the Python docs: "most modules have the name __builtins__ made available as part of their globals. The value of __builtins__ is normally either this module or the value of this module’s __dict__ attribute"

We may want to set the '__builtins__' entry to the type it was at unpickling time, although I have no idea whether taking this additional precaution this has an impact on runtime (either performance or simply errors)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with always using a dict for now. If someone complains we can always try to improve that later.

return mod


Expand Down
1 change: 1 addition & 0 deletions cloudpickle/cloudpickle_fast.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ def _memoryview_reduce(obj):

def _module_reduce(obj):
if _is_dynamic(obj):
obj.__dict__.pop('__builtins__', None)
return dynamic_subimport, (obj.__name__, vars(obj))
else:
return subimport, (obj.__name__,)
Expand Down
40 changes: 40 additions & 0 deletions tests/cloudpickle_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,46 @@ def test_module_locals_behavior(self):
finally:
os.unlink(pickled_func_path)

def test_dynamic_module_with_unpicklable_builtin(self):
# Reproducer of https://github.com/cloudpipe/cloudpickle/issues/316
# Some modules such as scipy inject some unpicklable objects into the
# __builtins__ module, which appears in every module's __dict__ under
# the '__builtins__' key. In such cases, cloudpickle used to fail
# when pickling dynamic modules.
class UnpickleableObject(object):
def __reduce__(self):
raise ValueError('Unpicklable object')

mod = types.ModuleType("mod")

exec('f = lambda x: abs(x)', mod.__dict__)
assert mod.f(-1) == 1
assert '__builtins__' in mod.__dict__

unpicklable_obj = UnpickleableObject()
with pytest.raises(ValueError):
cloudpickle.dumps(unpicklable_obj)

# Emulate the behavior of scipy by injecting an unpickleable object
# into mod's builtins.
# The __builtins__ entry of mod's __dict__ can either be the
# __builtins__ module, or the __builtins__ module's __dict__. #316
# happens only in the latter case.
if isinstance(mod.__dict__['__builtins__'], dict):
mod.__dict__['__builtins__']['unpickleable_obj'] = unpicklable_obj
elif isinstance(mod.__dict__['__builtins__'], types.ModuleType):
mod.__dict__['__builtins__'].unpickleable_obj = unpicklable_obj

depickled_mod = pickle_depickle(mod, protocol=self.protocol)
assert '__builtins__' in depickled_mod.__dict__

if isinstance(depickled_mod.__dict__['__builtins__'], dict):
assert "abs" in depickled_mod.__builtins__
elif isinstance(
depickled_mod.__dict__['__builtins__'], types.ModuleType):
assert hasattr(depickled_mod.__builtins__, "abs")
assert depickled_mod.f(-1) == 1

def test_load_dynamic_module_in_grandchild_process(self):
# Make sure that when loaded, a dynamic module preserves its dynamic
# property. Otherwise, this will lead to an ImportError if pickled in
Expand Down