ENH some steps to make cloudpickle dynamic function/classes more deterministic

ci/install_coverage_subprocess_pth.py

@@ -9,8 +9,8 @@
 import coverage; coverage.process_startup()
 """
 
-filename = op.join(get_path('purelib'), 'coverage_subprocess.pth')
-with open(filename, 'wb') as f:
-    f.write(FILE_CONTENT.encode('ascii'))
+filename = op.join(get_path("purelib"), "coverage_subprocess.pth")
+with open(filename, "wb") as f:
+    f.write(FILE_CONTENT.encode("ascii"))
 
-print('Installed subprocess coverage support: %s' % filename)
+print("Installed subprocess coverage support: %s" % filename)

cloudpickle/cloudpickle.py

@@ -126,7 +126,7 @@ def _lookup_class_or_track(class_tracker_id, class_def):
 
 
 def register_pickle_by_value(module):
-    """Register a module to make it functions and classes picklable by value.
+    """Register a module to make its functions and classes picklable by value.
 
     By default, functions and classes that are attributes of an importable
     module are to be pickled by reference, that is relying on re-importing
@@ -369,7 +369,7 @@ def func():
                 # sys.modules.
                 if name is not None and name.startswith(prefix):
                     # check whether the function can address the sub-module
-                    tokens = set(name[len(prefix) :].split("."))
+                    tokens = set(name[len(prefix):].split("."))
                     if not tokens - set(code.co_names):
                         subimports.append(sys.modules[name])
     return subimports
@@ -409,7 +409,9 @@ def _walk_global_ops(code):
 
 def _extract_class_dict(cls):
     """Retrieve a copy of the dict of a class without the inherited method."""
-    clsdict = dict(cls.__dict__)  # copy dict proxy to a dict
+    # copy dict proxy to a dict. Sort the keys to make the pickle deterministic
+    clsdict = {k: cls.__dict__[k] for k in sorted(cls.__dict__)}
+
     if len(cls.__bases__) == 1:
         inherited_dict = cls.__bases__[0].__dict__
     else:
@@ -533,9 +535,15 @@ class id will also reuse this class definition.
     The "extra" variable is meant to be a dict (or None) that can be used for
     forward compatibility shall the need arise.
     """
+    # We need to intern the keys of the type_kwargs dict to avoid having
+    # different pickles for the same dynamic class depending on whether it was
+    # dynamically created or reconstructed from a pickled stream.
+    type_kwargs = {sys.intern(k): v for k, v in type_kwargs.items()}
+
     skeleton_class = types.new_class(
         name, bases, {"metaclass": type_constructor}, lambda ns: ns.update(type_kwargs)
     )
+
     return _lookup_class_or_track(class_tracker_id, skeleton_class)
 
 
@@ -694,7 +702,9 @@ def _function_getstate(func):
     #   unpickling time by iterating over slotstate and calling setattr(func,
     #   slotname, slotvalue)
     slotstate = {
-        "__name__": func.__name__,
+        # Intern the function names to be consistent with the method names that are
+        # automatically interned with `setattr`. This is only the case for cpython.
+        "__name__": sys.intern(func.__name__) if not PYPY else func.__name__,
         "__qualname__": func.__qualname__,
         "__annotations__": func.__annotations__,
         "__kwdefaults__": func.__kwdefaults__,
@@ -802,6 +812,10 @@ def _code_reduce(obj):
     # of the specific type from types, for example:
     # >>> from types import CodeType
     # >>> help(CodeType)
+
+    # We need to intern the function names to be consistent with the method name,
+    # which are interned automatically with `setattr`. This is only the case for cpython.
+    co_name = sys.intern(obj.co_name) if not PYPY else obj.co_name
     if hasattr(obj, "co_exceptiontable"):
         # Python 3.11 and later: there are some new attributes
         # related to the enhanced exceptions.
@@ -817,7 +831,7 @@ def _code_reduce(obj):
             obj.co_names,
             obj.co_varnames,
             obj.co_filename,
-            obj.co_name,
+            co_name,
             obj.co_qualname,
             obj.co_firstlineno,
             obj.co_linetable,
@@ -840,7 +854,7 @@ def _code_reduce(obj):
             obj.co_names,
             obj.co_varnames,
             obj.co_filename,
-            obj.co_name,
+            co_name,
             obj.co_firstlineno,
             obj.co_linetable,
             obj.co_freevars,
@@ -861,7 +875,7 @@ def _code_reduce(obj):
             obj.co_consts,
             obj.co_varnames,
             obj.co_filename,
-            obj.co_name,
+            co_name,
             obj.co_firstlineno,
             obj.co_lnotab,
             obj.co_exc_handlers,
@@ -885,7 +899,7 @@ def _code_reduce(obj):
             obj.co_names,
             obj.co_varnames,
             obj.co_filename,
-            obj.co_name,
+            co_name,
             obj.co_firstlineno,
             obj.co_lnotab,
             obj.co_freevars,
@@ -1127,6 +1141,10 @@ def _class_setstate(obj, state):
         if attrname == "_abc_impl":
             registry = attr
         else:
+            # Note: attribute names are automatically interned in cpython. This means that to get
+            # determinist pickling in subprocess, we need to make sure that the dynamic function names
+            # are also interned.
+            # https://github.com/python/cpython/blob/main/Objects/object.c#L1060
             setattr(obj, attrname, attr)
     if registry is not None:
         for subclass in registry:

cloudpickle/cloudpickle_fast.py

@@ -6,6 +6,7 @@
 
 See: tests/test_backward_compat.py
 """
+
 from . import cloudpickle
 
 

tests/__init__.py

@@ -0,0 +1,3 @@
+import pytest
+
+pytest.register_assert_rewrite("tests.testutils")

tests/cloudpickle_test.py

@@ -48,10 +48,11 @@
 from cloudpickle.cloudpickle import _extract_class_dict, _whichmodule
 from cloudpickle.cloudpickle import _lookup_module_and_qualname
 
+from .testutils import subprocess_worker
 from .testutils import subprocess_pickle_echo
 from .testutils import subprocess_pickle_string
 from .testutils import assert_run_python_script
-from .testutils import subprocess_worker
+from .testutils import check_determinist_pickle
 
 
 _TEST_GLOBAL_VARIABLE = "default_value"
@@ -108,7 +109,7 @@ def method_c(self):
             return "c"
 
     clsdict = _extract_class_dict(C)
-    assert sorted(clsdict.keys()) == ["C_CONSTANT", "__doc__", "method_c"]
+    assert list(clsdict.keys()) == ["C_CONSTANT", "__doc__", "method_c"]
     assert clsdict["C_CONSTANT"] == 43
     assert clsdict["__doc__"] is None
     assert clsdict["method_c"](C()) == C().method_c()
@@ -1951,7 +1952,6 @@ def lookup(obj_id):
 
             class A:
                 '''Updated class definition'''
-                pass
 
             assert not w.run(lambda obj_id: isinstance(lookup(obj_id), A), id1)
             retrieved1 = w.run(lookup, id1)
@@ -1983,6 +1983,113 @@ class A:
         """.format(protocol=self.protocol)
         assert_run_python_script(code)
 
+    def test_dynamic_func_determinist(self):
+        # Check that the pickle serialization for a dynamic func is the same
+        # in two processes.
+
+        def get_dynamic_func_pickle():
+            def test_method(arg_1, arg_2):
+                pass
+            return cloudpickle.dumps(test_method)
+
+        with subprocess_worker(protocol=self.protocol) as w:
+
+            A_dump = w.run(get_dynamic_func_pickle)
+            check_determinist_pickle(A_dump, get_dynamic_func_pickle())
+
+    def test_dynamic_class_determinist(self):
+        # Check that the pickle serialization for a dynamic class is the same
+        # in two processes.
+        pytest.xfail("This test fails due to different tracker_id.")
+
+        def get_dynamic_class_pickle():
+            class A:
+                """Class with potential string interning issues."""
+
+                arg_1 = "class_value"
+
+                def join(self):
+                    pass
+
+                def test_method(self, arg_1, join):
+                    pass
+            return cloudpickle.dumps(A)
+
+        with subprocess_worker(protocol=self.protocol) as w:
+
+            A_dump = w.run(get_dynamic_class_pickle)
+            check_determinist_pickle(A_dump, get_dynamic_class_pickle())
+
+    def test_dynamic_class_determinist_subworker_order(self):
+        # Check that the pickle produced by the unpickled instance is the same.
+        # This checks that the order of the class attributes is deterministic.
+
+        with subprocess_worker(protocol=self.protocol) as w:
+
+            class A:
+                """Simple class definition"""
+
+                pass
+
+            A_dump = w.run(cloudpickle.dumps, A)
+            check_determinist_pickle(A_dump, cloudpickle.dumps(A))
+
+            # If the doc is defined after some class variables, this can cause ordering changes
+            # due to the way we reconstruct the class with _make_class_skeleton, which create
+            # the class and thus `__doc__` before populating it.
+            class A:
+                name = "A"
+                __doc__ = "Updated class definition"
+
+            A_dump = w.run(cloudpickle.dumps, A)
+            check_determinist_pickle(A_dump, cloudpickle.dumps(A))
+
+            # If the doc is defined in `__init__`, this can cause ordering changes due to the way
+            # we reconstruct the class with _make_class_skeleton. Make sure the order is deterministic
+            class A:
+                def __init__(self):
+                    """Class definition with explicit __init__"""
+                    pass
+
+            A_dump = w.run(cloudpickle.dumps, A)
+            check_determinist_pickle(A_dump, cloudpickle.dumps(A))
+
+    def test_dynamic_class_determinist_subworker_str_interning(self):
+        # Check that the pickle produced by the unpickled instance is the same.
+        # This checks that there is no issue due to string interning.
+
+        with subprocess_worker(protocol=self.protocol) as w:
+            # Due to interning of class attributes, check that this does not create issues
+            # with dynamic function definition.
+            class A:
+                """Class with potential string interning issues."""
+
+                arg_1 = "class_value"
+
+                def join(self):
+                    pass
+
+                def test_method(self, arg_1, join):
+                    pass
+
+            A_dump = w.run(cloudpickle.dumps, A)
+            check_determinist_pickle(A_dump, cloudpickle.dumps(A))
+
+            # XXX - this does not seem to work, and I am not sure there is an easy fix.
+            class A:
+                """Class with potential string interning issues."""
+
+                arg_1 = "join"
+
+                def join(self, arg_1):
+                    pass
+
+            A_dump = w.run(cloudpickle.dumps, A)
+            pytest.xfail(
+                "This test is expected to fail due to string interning errors."
+            )
+            check_determinist_pickle(A_dump, cloudpickle.dumps(A))
+
     @pytest.mark.skipif(
         platform.python_implementation() == "PyPy",
         reason="Skip PyPy because memory grows too much",

tests/cloudpickle_testpkg/_cloudpickle_testpkg/__init__.py

@@ -27,19 +27,22 @@ def relative_imports_factory():
     Relative import of functions living both inside modules and packages are
     tested.
     """
+
     def f():
         # module_function belongs to _cloudpickle_testpkg.mod, which is a
         # module
         from .mod import module_function
+
         return module_function()
 
     def g():
         # package_function belongs to _cloudpickle_testpkg, which is a package
         from . import package_function
+
         return package_function()
 
     return f, g
 
 
 some_singleton = _SingletonClass()
-T = typing.TypeVar('T')
+T = typing.TypeVar("T")

tests/cloudpickle_testpkg/_cloudpickle_testpkg/mod.py

@@ -20,7 +20,7 @@
 # module.  The following lines emulate such a behavior without being a compiled
 # extension module.
 
-submodule_name = '_cloudpickle_testpkg.mod.dynamic_submodule'
+submodule_name = "_cloudpickle_testpkg.mod.dynamic_submodule"
 dynamic_submodule = types.ModuleType(submodule_name)
 
 # This line allows the dynamic_module to be imported using either one of:
@@ -32,7 +32,7 @@
 # so this dynamic module could be binded to another name. This behavior is
 # demonstrated with `dynamic_submodule_two`
 
-submodule_name_two = '_cloudpickle_testpkg.mod.dynamic_submodule_two'
+submodule_name_two = "_cloudpickle_testpkg.mod.dynamic_submodule_two"
 # Notice the inconsistent name binding, breaking attribute lookup-based import
 # attempts.
 another_submodule = types.ModuleType(submodule_name_two)
@@ -41,9 +41,7 @@
 
 # In this third case, the module is not added to sys.modules, and can only be
 # imported using attribute lookup-based imports.
-submodule_three = types.ModuleType(
-    '_cloudpickle_testpkg.mod.dynamic_submodule_three'
-)
+submodule_three = types.ModuleType("_cloudpickle_testpkg.mod.dynamic_submodule_three")
 code = """
 def f(x):
     return x
@@ -53,9 +51,7 @@ def f(x):
 
 # What about a dynamic submodule inside a dynamic submodule inside an
 # importable module?
-subsubmodule_name = (
-    '_cloudpickle_testpkg.mod.dynamic_submodule.dynamic_subsubmodule'
-)
+subsubmodule_name = "_cloudpickle_testpkg.mod.dynamic_submodule.dynamic_subsubmodule"
 dynamic_subsubmodule = types.ModuleType(subsubmodule_name)
 dynamic_submodule.dynamic_subsubmodule = dynamic_subsubmodule
 sys.modules[subsubmodule_name] = dynamic_subsubmodule

tests/cloudpickle_testpkg/setup.py

@@ -5,12 +5,12 @@
 
 
 setup(
-    name='cloudpickle_testpkg',
-    version='0.0.0',
-    description='Package used only for cloudpickle testing purposes',
-    author='Cloudpipe',
-    author_email='cloudpipe@googlegroups.com',
-    license='BSD 3-Clause License',
-    packages=['_cloudpickle_testpkg'],
-    python_requires='>=3.8',
+    name="cloudpickle_testpkg",
+    version="0.0.0",
+    description="Package used only for cloudpickle testing purposes",
+    author="Cloudpipe",
+    author_email="cloudpipe@googlegroups.com",
+    license="BSD 3-Clause License",
+    packages=["_cloudpickle_testpkg"],
+    python_requires=">=3.8",
 )

tests/generate_old_pickles.py

@@ -8,6 +8,7 @@
 active cloudpickle branch to make sure that cloudpickle is able to depickle old
 cloudpickle files.
 """
+
 import sys
 
 from pathlib import Path

tests/mock_local_folder/mod.py

@@ -5,6 +5,7 @@
 reference should instead flagged to cloudpickle for pickling by value: this is
 done using the register_pickle_by_value api exposed by cloudpickle.
 """
+
 import typing
 
 

tests/test_backward_compat.py

@@ -9,6 +9,7 @@
 few canonical use cases. Cloudpicke backward-compatitibility support remains a
 best-effort initiative.
 """
+
 import pickle
 
 import pytest