Expose CloudPickler as Pickler instead of pickle.Pickler

[twoertwein]

Hi,

I don't know whether there is a convention what <pickle_module>.Pickler is but it seems that some projects [1] assume that they can use it.

Unfortunately, Cloudpickle.Pickler is pickle's Pickler. It might be helpful if it points to cloudpickle.CloudPickler.

If the solution is to simply rename Pickle's Pickler to Pickler_ and to have an alias for CloudPickler, I'm happy to create a PR for that.

[1] pytorch/pytorch#38098

[ogrisel]

This was already discussed in #235. We would have to go through a FutureWarning phase for 2 releases (using module-level getattr).

Someone has to do it though. Would you be interested in submitting a PR?

[twoertwein]

oh, I didn't see that - thank you! I can make a PR this weekend.

So the idea is to simply add a FutureWarning for now when someones tries to use Cloudpickle.Pickler. And after two more releases, we can then merge #235 .

[twoertwein]

I closed my PR: master has changed quite a bit and I was still not able to figure out the python < 3.7 part.

It seems that there is now a way simpler solution for this problem since cloudpickle.Pickler is not defined anymore: we could simply define cloudpickle.Pickler = cloudpickle.CloudPickler in the init file. Does that sound like a good solution?

import cloudpickle
import torch

data = {"foo": lambda: True}
path = "foo.pickle"

assert not hasattr(cloudpickle, 'Pickler')
cloudpickle.Pickler = cloudpickle.CloudPickler

torch.save(data, path, pickle_module=cloudpickle)  # works

[ogrisel]

Indeed we broke the backward compat without realizing it :(

Therefore what you suggest seems fine. We should probably add some tests, make it explicit that this is the new correct way to use the library by updating the README and then later we will deprecate the CloudPickler class (but not before we drop Python 3.6 compat so that we can use module level __getattr__ to issue the deprecation warning).

[twoertwein]

I skimmed the tests, they don't seem to directly refer to cloudpickle.CloudPickler anywhere. Most calls are just cloudpickle.dump(). Which tests do you think will be needed?

[pierreglaser]

Something like assert hasattr(cloudpickle, "Pickler") and cloudpickle.Pickler is cloudpickle.CloudPickler is a good start. This should prevent our future selves from mistakenly removing these lines.

[twoertwein]

thanks @pierreglaser I added that test to #392.

The README does not mention cloudpickle.CloudPickler. For people following the README, there shouldn't be any change necessary.