Expose CloudPickler as Pickler instead of pickle.Pickler #366

Activity

1. ogrisel commented on May 20, 2020

   

   ogrisel

   on May 20, 2020

   Contributor

   More actions

   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?
2. twoertwein commented on May 20, 2020

   

   twoertwein

   on May 20, 2020

   ContributorAuthor

   More actions

   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 .
3. twoertwein commented on Jul 20, 2020

   

   twoertwein

   on Jul 20, 2020

   Last edited by twoertwein

   ContributorAuthor

   More actions

   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
4. ogrisel commented on Aug 11, 2020

   

   ogrisel

   on Aug 11, 2020

   Contributor

   More actions

   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).
5. twoertwein commented on Aug 11, 2020

   

   twoertwein

   on Aug 11, 2020

   ContributorAuthor

   More actions

   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?
6. pierreglaser commented on Aug 11, 2020

   

   pierreglaser

   on Aug 11, 2020

   Member

   More actions

   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.

   👍React with 👍1Reacted by Torsten Wörtwein
7. twoertwein commented on Aug 12, 2020

   

   twoertwein

   on Aug 12, 2020

   ContributorAuthor

   More actions

   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.
8. pierreglaser

   closed this as completedin #392on Aug 13, 2020
9. wip-sync

   added a commit that references this issue on Apr 4, 2022

   py-cloudpickle: updated to 2.0.0

   ...

   71089d6