rossant / ipycache

Defines a %%cache cell magic in the IPython notebook to cache results of long-lasting computations in a persistent pickle file
BSD 3-Clause "New" or "Revised" License
138 stars 35 forks source link

Adding cloudpickle support #35

Closed fbnrst closed 9 years ago

fbnrst commented 9 years ago

Check if the cloudpickle module is installed and use it for dump because cloudpickle can dump things that pickle can't(e.g.lambda functions).

rossant commented 9 years ago

Thanks!

I would rather do the try import directly within save_vars

ihrke commented 9 years ago

Hi,

wouldn't this be enough? I don't think there is a need for foundcloudpickle variable...

try:
   import cloudpickle as pickle
except ImportError:
   import cPickle as pickle
fbnrst commented 9 years ago

Hi, @rossant I just prefer to keep all the imports together. But we could do that as well.

@ihrke I tried this first, but cloudpickle does not have a cloudpickle.load method. Maybe, one could overwrite the pickle.dump method like this:

try:
   import cloudpickle as pickle
   import cPickle as pickle
   pickle.dump = cloudpickle.dump
except ImportError:
   import cPickle as pickle

But I thought that's a bit ugly. Or we define our own dump function like that:

import cPickle as pickle
try:
   import cloudpickle
   dump = cloudpickle.dump
except ImportError:
   dump = pickle.dump

and then use our dump function in save_vars. I think I'd prefer this version.

btw: I went through all the hassle with the imp module because of this answer on Stackoverflow: http://stackoverflow.com/a/14050282/5142797

ihrke commented 9 years ago

I think using the imp module is only necessary if you want to check for the module's existence without actually wanting to import it. Since we always want to import cloudpickle if it exists, we do not have this problem and do not need imp. Also, if you want to use imp, we would need to implement it differently for python2 and python3 (see the same answer you linked to).

As to the problem with the missing load function, your second solution looks ok to me. Btw, can you check out why the travis builds failed? It's something in py3 with not finding your gobal variable so you need to make sure your patch works with py2 and py3...

fbnrst commented 9 years ago

OK, I updated the PR according to our discussion. I moved the cloudpickle import after the py2/py3 section because it relies on pickle already being imported. Before I had the cloudpickle import in the py2 section, I guess that's why the travis builds.

ihrke commented 9 years ago

Cool, thanks for your work! Would you mind putting the last commit in a clean branch that is based on our current master? That way, we avoid putting 7 unused commits in our history :-)

fbnrst commented 9 years ago

yes, no problem. I guess that means i have to create a new PR, right? I'll do so.