Handle dynamic modules globals modification #201

pierreglaser · 2018-09-11T12:51:40Z

I created this PR to have us take a stand about the behavior with regards to dynamic modules caching.

cloudpickle bevaves inconstently when a function defined in a dynamic module that mutates globals variables of that module gets pickled and then unpickled several times: new globals are created each time and erase the previous values.

As as example, let mod be a dynamic module:

import imp
import textwrap
import cloudpickle
import pickle

mod = imp.new_module('mod')
code = '''
x = 1
def func_that_relies_on_dynamic_module():
    global x
    return x
'''
exec(textwrap.dedent(code), mod.__dict__)

this will run fine

first_f = pickle.loads(
    cloudpickle.dumps(mod.func_that_relies_on_dynamic_module)
)
mod.x = 2
new_f = pickle.loads(cloudpickle.dumps(mod.func_that_relies_on_dynamic_module))
assert first_f() == 1
assert new_f() == 2

The question is: is this what we want, or would we rather cache the globals of the dynamic module?
The potential risk is if the globals are large objects, and that we keep a reference to it, the size of the cache my increase, leading to a potential memory leak.

pinging @ogrisel @tomMoral on this.

codecov-io · 2018-09-12T14:40:52Z

Codecov Report

Merging #201 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #201     +/-   ##
=========================================
+ Coverage   84.95%   85.06%   +0.1%     
=========================================
  Files           1        1             
  Lines         565      569      +4     
  Branches      109      110      +1     
=========================================
+ Hits          480      484      +4     
  Misses         62       62             
  Partials       23       23

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	`85.06% <100%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81a7a8c...ae1079b. Read the comment docs.

ogrisel · 2018-09-13T11:23:37Z

@pierreglaser could you please rebase on top of the current master now that #198 has been merged?

pierreglaser · 2018-09-13T12:17:20Z

@ogrisel done.

ogrisel · 2018-09-13T12:39:22Z

Thanks. The current code is problematic however: it can cause a memory leak in applications that generate a large number of transient dynamic modules with different names.

Instead of storing the globals vars in the _dynamic_modules_globals dict, it should be possible to refactor your code to use a weakref.WeakValueDictionary instance named _dynamic_modules to keep track of the dynamic module themselves without causing a memory leak when there are no longer used. See below:

>>> import weakref
>>> import imp
>>> import textwrap
>>> import cloudpickle
>>> mod = imp.new_module('mod')
>>> code = '''
... x = 1
... def func_that_relies_on_dynamic_module():
...     global x
...     return x
... '''
...
>>> exec(textwrap.dedent(code), mod.__dict__)
>>> _dynamic_modules = weakref.WeakValueDictionary()
>>> _dynamic_modules[mod.__name__] = mod
>>> _dynamic_modules
<WeakValueDictionary at 0x7f73d3f84320>
>>> list(_dynamic_modules.keys())
['mod']
>>> del mod
>>> list(_dynamic_modules.keys())
[]

pierreglaser · 2018-09-14T11:03:49Z

Sounds good. Should there be a test that effectively verifies if _ dynamic_modules does not increase too much in size when several modules are created and deleted?

ogrisel · 2018-09-14T14:01:24Z

You can check that if you unpickle the dynamic module function in a local function run a subprocess, list(_dynamic_modules.keys()) is back to empty list after the function call.

ogrisel · 2018-09-14T17:56:33Z

cloudpickle/cloudpickle.py

+    """class used to instanciate base_globals, in order to be weakly refrenced
+    into _dynamic_modules_globals
+    """
+    pass


Why don't you keep a reference to the module object itself as I suggested in #201 (comment) ?

That would make the code much more homogeneous, no?

In this particular case, a module was never properly created in the previous code, simply a dict object was created. But we can change this by using imp.new_module instead.

No my point would be to register the dynamic modules themselves in _dynamic_modules when we unpickle them (in dynamic_subimport I think) and then use their name as value for base_globals exactly as we do for regular modules. Falling back to empty dict might still be useful for weird modules that are neither found in sys.modules nor registered in _dynamic_modules.

In the case where we pickle only a function from a dynamic module, and not the dynamic module itself, it is not possible, because only mod.f (a function) is unpickled, so a dynamic module is never unpickled.
Creating a new module as I said does not work though, because it will only be referenced in _dynamic_modules_globals, however, since it is a WeakValueDictionnary , the module will be garbage collected as soon as we get out of the function where the module is created.

By using a dictionary instead of a module, things worked because f.__globals__ is a reference to a value in _dynamic_modules_globals, so the item in this dict was not garbage collected as long as the function was not deleted, which is not the case anymore if we use a module, as f does not have a reference to the module itself.

So in this particular use case, it seems hard to keep a reference to the dynamic module where an unpickled function lived.

Alright, that's a good point.

ogrisel · 2018-09-14T17:57:40Z

tests/cloudpickle_test.py

@@ -440,6 +442,48 @@ def method(self, x):
        mod1, mod2 = pickle_depickle([mod, mod])
        self.assertEqual(id(mod1), id(mod2))

+    def test_dynamic_modules_cache(self):


This is not a cache. A cache has specific semantics which are not the semantics of this hold. It's more like a list of reference to the known active dynamic modules.

ogrisel · 2018-09-14T17:59:20Z

tests/cloudpickle_test.py

+        code = '''
+        x = 1
+        def f():
+            return


We also need the function to reference and mutate the global variable as we did in #188 and #198. Otherwise we cannot make assertions on the correct handling of the global variables.

+1 on this one

Or maybe test_function_from_dynamic_module_with_globals_modifications is already testing this behavior?

It is. I can merge the two test, but unsure if it is better.

for now only a function of a dynamic module is unpickled, to show a case where creating a dynamic module is hard.

pierreglaser · 2018-09-17T10:00:10Z

I changed the test to show the case where creating a dynamic module is challenging. Also, shouldn't the _dynamic_modules_globals.keys() be done in the subprocess itself, since it is here where _dynamic_modules_globals gets populated? (I also re-worked this in the new commit)

tomMoral · 2018-09-17T10:01:25Z

tests/cloudpickle_test.py

            return
        '''
        exec(textwrap.dedent(code), mod.__dict__)

        pickled_module_path = 'mod.pkl'
+        print(mod.func.__module__)


spurious print

ogrisel

Here is a first batch of comments, I still need to review the second test.

ogrisel · 2018-09-18T08:19:08Z

cloudpickle/cloudpickle.py

+
+class DynamicDict(dict):
+    """class used to instanciate base_globals, in order to be weakly refrenced
+    into _dynamic_modules_globals


nitpick: please follow PEP 257 for docstring: oneliner short summary, then optional blank line + paragraph with more details that could not fit in the summary. Following PEP 257 is helpful to make code editors documentation tooltips work better (as well as sphinx API doc for public function / classes).

Also I think we should make the this class private explicitly with a leading underscore.

class _DynamicModuleFuncGlobals(dict): """Global variables referenced by a function defined in a dynamic module To avoid leaking references we store such context in a WeakValueDictionary instance. However instances of python builtin types such as dict cannot be used directly as values in such a construct, hence the need for a derived class. """ pass

ogrisel · 2018-09-18T08:21:29Z

cloudpickle/cloudpickle.py

+            base_globals = _dynamic_modules_globals.get(
+                    base_globals, DynamicDict())
+            # base_globals is not a string anymore, using base_globals_name
+            # instead


This comment is unnecessary in my opinion. The base_globals_name variable name is explicit enough.

ogrisel · 2018-09-18T08:47:54Z

cloudpickle/cloudpickle.py

            # lived in
            base_globals = vars(sys.modules[base_globals])
        else:
-            base_globals = {}
+            base_globals = _dynamic_modules_globals.get(
+                    base_globals, DynamicDict())


Small optim:

base_globals = _dynamic_modules_globals.get(base_globals_name, None) if base_globals is None: base_globals = _DynamicModuleFuncGlobals()

to spare the creation of _DynamicModuleFuncGlobals when not necessary.

ogrisel · 2018-09-18T08:49:07Z

tests/cloudpickle_test.py

@@ -42,7 +43,8 @@
    tornado = None

 import cloudpickle
-from cloudpickle.cloudpickle import _find_module, _make_empty_cell, cell_set
+from cloudpickle.cloudpickle import _find_module, _make_empty_cell, cell_set,\
+        _dynamic_modules_globals


Please use a new from .... import line instead of multiline imports: I find it more readable and it makes it easier to avoid merge conflict.

ogrisel · 2018-09-18T08:50:54Z

tests/cloudpickle_test.py

@@ -440,6 +442,58 @@ def method(self, x):
        mod1, mod2 = pickle_depickle([mod, mod])
        self.assertEqual(id(mod1), id(mod2))

+    def test_dynamic_modules_globals(self):
+        # _dynamic_modules_globals is a WeakValueDictionary, so if this dynamic
+        # module has no other reference than in this # dict (whether on the


# leftover

ogrisel · 2018-09-18T08:51:40Z

tests/cloudpickle_test.py

+        # _dynamic_modules_globals is a WeakValueDictionary, so if this dynamic
+        # module has no other reference than in this # dict (whether on the
+        # child process or the parent process), this module will be garbage
+        # collected.


In the context of this test, _dynamic_modules_globals is only used in the child process.

Also it's not the module that gets garbage collected by the function globals dict.

ogrisel · 2018-09-18T08:52:28Z

tests/cloudpickle_test.py

+
+        # A dictionnary storing the globals of the newly unpickled function
+        # should have been created
+        assert list(_dynamic_modules_globals.keys())==['mod']


style: whitespaces around ==.

ogrisel · 2018-09-18T08:52:46Z

tests/cloudpickle_test.py

+        # There is no reference to the globals of func since func has been
+        # deleted and _dynamic_modules_globals is a WeakValueDictionary,
+        # so _dynamic_modules_globals should now be empty
+        assert list(_dynamic_modules_globals.keys())==[]


ogrisel · 2018-09-18T17:27:50Z

For some reason I could not directly push to your branch so I took over in #205.

pierreglaser mentioned this pull request Sep 11, 2018

FIX More func base globals fixes #198

Merged

added dynamic module test where expected behavior is ambiguous

f022c41

pierreglaser force-pushed the handle-dynamic-modules-globals-modification branch from ae1079b to f022c41 Compare September 13, 2018 12:15

changed _dynamic_modules_globals to a WeakValueDictionary

2a0c3cb

ogrisel reviewed Sep 14, 2018

View reviewed changes

changed the dynamic_modules_globals test

780dee7

for now only a function of a dynamic module is unpickled, to show a case where creating a dynamic module is hard.

tomMoral reviewed Sep 17, 2018

View reviewed changes

pierreglaser added 4 commits September 17, 2018 12:02

removed unused comment

7561346

removed print statement

faab1f5

changed pickled function file path

c82008b

added pickled files removal

21f05db

ogrisel reviewed Sep 18, 2018

View reviewed changes

various style fixes

f1f4c31

ogrisel mentioned this pull request Sep 18, 2018

Global variables handling in dynamically defined functions. #205

Merged

ogrisel closed this Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle dynamic modules globals modification #201

Handle dynamic modules globals modification #201

pierreglaser commented Sep 11, 2018 •

edited by ogrisel

Loading

codecov-io commented Sep 12, 2018 •

edited

Loading

ogrisel commented Sep 13, 2018

pierreglaser commented Sep 13, 2018

ogrisel commented Sep 13, 2018 •

edited

Loading

pierreglaser commented Sep 14, 2018

ogrisel commented Sep 14, 2018

ogrisel Sep 14, 2018

pierreglaser Sep 17, 2018

ogrisel Sep 17, 2018

pierreglaser Sep 17, 2018 •

edited

Loading

ogrisel Sep 18, 2018

ogrisel Sep 14, 2018

ogrisel Sep 14, 2018

tomMoral Sep 17, 2018

tomMoral Sep 17, 2018

pierreglaser Sep 17, 2018

pierreglaser commented Sep 17, 2018

tomMoral Sep 17, 2018

ogrisel left a comment

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel Sep 18, 2018

ogrisel commented Sep 18, 2018

Handle dynamic modules globals modification #201

Handle dynamic modules globals modification #201

Conversation

pierreglaser commented Sep 11, 2018 • edited by ogrisel Loading

codecov-io commented Sep 12, 2018 • edited Loading

Codecov Report

ogrisel commented Sep 13, 2018

pierreglaser commented Sep 13, 2018

ogrisel commented Sep 13, 2018 • edited Loading

pierreglaser commented Sep 14, 2018

ogrisel commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreglaser Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreglaser commented Sep 17, 2018

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel commented Sep 18, 2018

pierreglaser commented Sep 11, 2018 •

edited by ogrisel

Loading

codecov-io commented Sep 12, 2018 •

edited

Loading

ogrisel commented Sep 13, 2018 •

edited

Loading

pierreglaser Sep 17, 2018 •

edited

Loading