Remove non-standard __transient__ support

[Peque]

The __transient__ dunder attribute is not the standard way to prevent attributes from being pickled. Instead, the standard approach is to use the __getstate__ and __setstate__ magic methods.

Considering that:

• This is an old fix that was implemented maybe for unsupported Python versions (i.e.: Python 2.6).
• Nobody knows what it is doing there exactly or why it was added.
• Having this special non-standard case implemented makes the code more complex and may result in unexpected behavior (see The __transient__ attribute. What is it? Could it be thrown away? #108).
• This code was not even covered by tests, so removing it should increase code coverage and make the module more robust.

I propose to remove any support for the non-standard approach.

As mentioned in #108, some other projects might be using this attribute. But when looking at those projects:

• Most simply have copies of the cloudpickle.py file (hence the match when searching for __transient__).
• Others simply seem to be using __transient__ but without depending on cloudpickle as an external module dependency.
• Most seem to be not very relevant (i.e.: fewer than 2 stars).

I think even though this change may break other's code it is an unlikely scenario. Anyway, if that was the case, I think they should be fixing their code rather than making cloudpickle carry ugly fixes. Also, they can always choose to use an older cloudpickle version from PyPi.

Fixes #108.

[codecov-io]

Codecov Report

Merging #110 into master will increase coverage by 1.77%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   81.34%   83.12%   +1.77%     
==========================================
  Files           2        2              
  Lines         563      551      -12     
  Branches      113      107       -6     
==========================================
  Hits          458      458              
+ Misses         75       65      -10     
+ Partials       30       28       -2

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	83.02% <ø> (+1.77%)	⬆️

---

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 d986894...bb3708c. Read the comment docs.

[rgbkrk]

I think this is good reasoning and we'll want to make a major version release for this change if others think this is a good path to go.

/cc @ogrisel and @mrocklin

[irmen]

awesome analysis btw @Peque

[mrocklin]

I have no objection to removing support for this non-standard protocol

[holdenk]

Anyone mind if I give this a quick spin in Spark to see if we run into any issues first? We don't directly use the library but I'm eternally hopefully I'll find a way to migrate us to using the common cloud pickle.

[rgbkrk]

Anyone mind if I give this a quick spin in Spark to see if we run into any issues first?

Happy to wait to see.

[irmen]

@holdenk unrelated to this particular pull request but I'm curious: do you know if this is in any way related to the use of regular Python pickle + my pyrolite library (that provides a java implementation of the pickle protocol), in Spark?

[holdenk]

@irmen I'm not sure I'm resolving the "this" correctly in do you know if this is in any way related to the use of regular Python pickle correctly - what are you referencing?

[irmen]

@holdenk not really sure myself tbh... I'm not familiar with [py]Spark myself. I guess I'm looking for the reason cloudpickle is used in Spark when pickle (+pyrolite for java connectivity) is used as well. Maybe I'm confusing pyspark and spark itself

[holdenk]

So from memory the CloudPickle serializer is used for "commands" or functions (as well as things included in their scope like broadcast variables). I think its just for better handling of serialized functions+dependencies whereas for the contents of data we need to be able to parse the keys (which are encoded as long values) in Java - we don't have to understand any part of the function in Java.

Of course this is based off of fuzzy memories of long ago discussions, so don't hold me to be 100% accurate and before you make any decissions on this reach out and we can sort out the real reasons (or a close enough approximation for whatever you need).

[rgbkrk]

I got a little curious if it would be helpful if PySpark and Spark (in Scala) used pickles for the interchange format using pyrolite on the JVM side.

[holdenk]

So at the end of the day I think we're going to start seeing different formats being used for interchange in Spark (like Arrow) - so I think the best path forward is seeing what happens there. I'm going to also perhaps take this opportunity to resurrects @rgbkrk's work on getting Spark using a current version of cloudpickle since I need to do some of that to test this PR w/Spark anyways.

[irmen]

@rgbkrk as far as I know, Spark already uses pickle + pyrolite.

[rgbkrk]

Oh silly me.

[holdenk]

So I'm seeing some failures with this in Spark, but my build was in a bit of a bad state so I'm going to kick off a fully clean build and see where this takes us. Sorry for my slowness at looking at this on the Spark side I've been running around a lot the past few days.

[irmen]

no worries

[holdenk]

LGTM, there are some outstanding issues but they aren't impacted but this PR,

[rgbkrk]

Thanks for checking!