Pickling fails for immutable objects that occur in recursive structures

[anivegesana]

Minimal failure case:

def f():
  d = {}
  def g():
    return d
  d['g'] = g
  return g

import dill
dill.dumps(f())

g.__closure__ contains g, but not directly. Currently, save_cell only breaks cycles that directly contain the function, not collections or objects that contain the function.

CloudPickle's solution of breaking the cycles within functions as opposed to within cells (like dill does) may be undesirable in some situations:

import cloudpickle

class C:
  def __init__(self, fx):
    self.fx = fx
    print(self.fx.d)
  def __reduce__(self):
    return (C, (self.fx,))

def f():
  t = ((lambda: t),)
  t[0].d = 0
  t[0].c = C(t[0])
  return t

pkl = cloudpickle.dumps(f())
cloudpickle.loads(pkl)

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in __init__
AttributeError: 'function' object has no attribute 'd'

Therefore, a more dynamic cycle breaking mechanism would be preferred, but tradeoffs of each method should be deliberated.

Another example:

uqfoundation/mystic@f6f71f5

[mmckerns]

@anivegesana: I'm really hesitant to put out a new release that removes functionality, however we are at that point after your PR #443. What are your plans for this issue? I may have to roll back #443, or try to fix this issue, before I put out the next release. I've been delaying it in hopes this gets resolved. Essentially, it's my fault that I didn't catch you'd removed functionality in #443. However, let me know what your plans are with addressing this, or if I need to do it.

[anivegesana]

I was unable to find any significant cases where cloudpickle's approach (break cycles in the functions) fails. The only two cases that their approach fails are ones where the __closure__ member of a function is used in its own definition and ones that depend on the insertion order of __dict__ (like the one I mentioned above.) Neither case would appear in normal Python code.

I guess I only need to know if it is fine to exclude these two rarer edge cases so that we can adopt this simpler solution. This, a complete solution, or not making any new changes will not remove functionality since this feature didn't work before #443. The only changes of a more complete solution would be reimplementations of save_list, save_dict, save_tuple, etc. that can gracefully handle RecursionErrors, so adding a more complete solution later wouldn't break backwards compatibility since none of the Unpickler functions would have to be removed or added.

The implementation of the simpler solution is at #461.

[mmckerns]

sounds like a good plan, thanks for the quick follow-up

[mmckerns]

@anivegesana: What do you want to do with this issue in light of #461 and #448?

[anivegesana]

I think we can close it. Although there are some objects that cannot be handled by the PR, a custom __reduce__ function can be written to manually break the circularity. Another issue can be opened if this is not the case.