Add ability to pickle dynamically create modules

[rodrigofarnhamsc]

The old logic treated all modules the same, which would fail when unpickling.
In save_module detect whether the module has been dynamically created by following the chain of imports. Noteworthy is that imp.find_module doesn't work with submodules (example sckit.tree), so we actually have to split the module name and iterate over each piece.

Dynamic modules are saved as dictionaries and reconstituted by dynamic_subimport function. While working on the test cases I discovered NotImplemented and Ellipsis also don't work properly (they are introduced into the test dynamic module by exec). I've also addressed that.

[rgbkrk]

What do you think @mrocklin? Seems like a good contribution.

[mrocklin]

Would it make sense to pull this logic out into a separate function?

[ygravrand]

I agree

[ogrisel]

LGTM but I think the commits could be all squashed together.

[ygravrand]

Great contribution, thanks !

[jakirkham]

Sounds like you are already fixing a case that I am interested in. :) Namely handling Ellipsis objects. ( #54 ) Feel free to close it once this is merged.

[rodrigofarnhamsc]

I've extracted out the _find_module function and squashed the commits together.

[jakirkham]

Would it be worth testing the _find_module function directly? Honestly, idk because it seems it is being covered indirectly and you have marked it as an internal detail, but something to think about.

[rodrigofarnhamsc]

I had forgotten to pass obj to save_reduce which meant dynamic modules were not treated as singletons. That actually raises an interesting question of how to handle unpickling dynamic modules in environment where there is naming collisions. For example:

my_mod = dyn_mod_gen()
m1, m2 = pickle_depickle([my_mod, my_mod])

# m1 is identical to m2, but they are different from my_mod

One possible way to address that issue is to peek at sys.modules and try to match module names with existing modules. But that seems very magical, and imp.new_module doesn't add created modules to sys.modules.

For my use case, Big Query API in python notebooks produces dynamically generated modules which I want to save for later.

[rodrigofarnhamsc]

@jakirkham I can add a test for _find_module. I just didn't want to crowd the test file with my so many tests for my feature, considering the other, arguably more important features, are not as densely tested.

[rodrigofarnhamsc]

obj=obj here is actually important to enable memoization, but it doesn't ensure that the produced dynamic modules will be singletons. Example:

dynmod.x = 1
m1, m2 = pickle.depickle([dynmod, dynmod])
m1.x = 2  # m1 and m2 are identical, m2.x is also 2
m3 = pickle.depickle(dynmod)
# m3 is a different module altogether

[jakirkham]

My feeling at least is that having good test coverage of the handling of dynamic modules will help keep things sane. Again they will probably be copy-and-paste versions of your existing tests that are trimmed down a bit. Still, if something goes wrong here, it is nice to be able to rule out some simpler cases and having these already written out will make it easier to construct variants. Again just a thought.

[rodrigofarnhamsc]

@jakirkham, I added tests for _find_module. I wish there was a way for me to verify that it correctly finds submodules. I could for example use scipy.io which is a submodule incorrectly handled by imp.find_module. But if scipy were refactored such that scipy.io was no longer a submodule the test would not longer work.

[rgbkrk]

if scipy were refactored such that scipy.io was no longer a submodule the test would not longer work.

we can always change that again later, and it would just be a test/dev dependency

[rodrigofarnhamsc]

@rgbkrk the test would have to be of the following form then:

with pytest.raises(ImportError):
  imp.find_module('scipy.io')

find_module('scipy.io')

To ensure that io remains a submodule of scipy.

[jakirkham]

What about doing something simple like testing cloudpickle.cloudpickle or would that not work?

Alternatively, I suppose you could create a dummy submodule in the tests directory for this purpose. A bit ugly perhaps, but not unreasonable. Something like foo/bar so it would not be picked up by the test runner.

[jakirkham]

Interesting fact, apparently Python 3.3+ fixed the pickling of NotImplemented and Ellipsis. Though this is not the case for Python 2.7.11. They also showed up in the exact same bug report. ( http://bugs.python.org/issue13842 ) So must just not be backported, but not sure why it hasn't happened yet.

[rgbkrk]

Thank you @rodrigofarnhamsc! I finally noticed the squashing of commits, sorry that took a little while to come back to.

[jakirkham]

Thanks. This is awesome. Any chance we might see this in a release any time soon?

[rgbkrk]

We were giving @mrocklin some time for #45 (see #46 (comment)), though we can push it out sooner.

[mrocklin]

I don't plan to have a solution to that in the near future. I think we should release soon.

[jakirkham]

No need to rush. I built a development copy of cloudpickle with these changes, which is serving me quite nicely. Just please let me know when it comes out so I can switch over to a proper release.

[rgbkrk]

@mrocklin sounds good!

[jakirkham]

Somehow it didn't show @mrocklin's comment while I was writing that. Either way. I'll let you guys decide.