Lift closure cell update to earliest function

[anivegesana]

This is a partial bug fix for #458. It covers most reasonable cases that were not covered in #443. See #458 for more details.

[mmckerns]

Tested python-2.7, 3.7-3.11; dill test suite works as expected.
Tested pypy-2.7, 3.7-3.9; dill test suite works as expected, except for pypy-3.9:

$ pypy-3.9 test_recursive.py 
Traceback (most recent call last):
  File "test_recursive.py", line 179, in <module>
    test_collection_function_recursion()
  File "test_recursive.py", line 166, in test_collection_function_recursion
    g = copy(collection_function_recursion())
  File "test_recursive.py", line 28, in copy
    return dill.copy(obj, byref=byref, recurse=recurse)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 312, in copy
    return loads(dumps(obj, *args, **kwds), ignore=ignore)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 352, in dumps
    dump(obj, file, protocol, byref, fmode, recurse, **kwds)#, strictio)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 324, in dump
    Pickler(file, protocol, **_kwds).dump(obj)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 567, in dump
    StockPickler.dump(self, obj)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 500, in dump
    self.save(obj)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 1752, in save_function
    _save_with_postproc(pickler, (_create_function, (
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 1054, in _save_with_postproc
    pickler.save_reduce(*reduction, obj=obj)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 705, in save_reduce
    save(args)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 915, in save_tuple
    save(element)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 900, in save_tuple
    save(element)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 1438, in save_cell
    pickler.save_reduce(_create_cell, (f,), obj=obj)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 705, in save_reduce
    save(args)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 900, in save_tuple
    save(element)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 1164, in save_module_dict
    StockPickler.save_dict(pickler, obj)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 985, in save_dict
    self._batch_setitems(obj.items())
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 1016, in _batch_setitems
    save(v)
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/pickle.py", line 573, in save
    f(self, obj)  # Call unbound method with explicit self
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 1752, in save_function
    _save_with_postproc(pickler, (_create_function, (
  File "/Users/mmckerns/src/pypy3.9-v7.3.8-osx64/lib/pypy3.9/site-packages/dill-0.3.5.dev0-py3.9.egg/dill/_dill.py", line 1048, in _save_with_postproc
    warnings.warn('Cannot pickle %r: %shas recursive self-references that trigger a RecursionError.' % (obj, name), PicklingWarning)
dill._dill.PicklingWarning: Cannot pickle <function collection_function_recursion.<locals>.g at 0x000000010a680020>: __main__.collection_function_recursion.<locals>.g has recursive self-references that trigger a RecursionError.

[mmckerns]

All versions fail here: uqfoundation/mystic@f6f71f5 (as expected?)

[anivegesana]

Sorry for the delay. I was unable to reproduce the error message on pypy3.9-v7.3.9-osx64. Perhaps, it was a bug within pypy that was fixed between 7.3.8 and 7.3.9. Can you try it again with the new version of pypy?

As for mystic, I am still looking into it. I am getting this error message. Is this what you are seeing or are there additional errors?

tests/__main__.py
Warning: Invalid termination condition (nPop < 2)
Warning: Invalid termination condition (nPop < 2)
Warning: Invalid termination condition (nPop < 2)
----------------------------------------------------------------------
Ran 32 tests in 3.695s

OK
----------------------------------------------------------------------
Ran 16 tests in 0.045s

OK
----------------------------------------------------------------------
Ran 16 tests in 0.072s

OK
----------------------------------------------------------------------
Ran 16 tests in 0.098s

OK
----------------------------------------------------------------------
Ran 16 tests in 3.403s

OK
Traceback (most recent call last):
  File "/Users/ani/GitHub/mystic/tests/test_boundsconstrained.py", line 52, in <module>
    test_constrained(NelderMeadSimplexSolver, tight=True, clip=None) #ftol=1e-8?
  File "/Users/ani/GitHub/mystic/tests/test_boundsconstrained.py", line 48, in test_constrained
    assert eval(_eqn) # constraint is satisfied
AssertionError
............................

I don't think I made a change to either repo, and now 939d7aee and f6f71f5 work for me. I did make a couple of force pushes before ab3e8e3, so it is possible that you got an old version. I will still try to reproduce find out where that previous error came from. Can you please share the error message for mystic?

[mmckerns]

You may be right. I just downloaded pypy v7.3.9, and will upgrade and run the dill test suite again.

With regard to mystic, you are seeing an AssertionError that is unrelated, and these can happen from time to time (a rerun should pass). I'm seeing the pickling errors from mystic with regard to mystic.cache, and some of the super new code that has not yet migrated into the mystic codebase, but is in examples3. It happens whenever a cache of function evaluations is built. I don't have tests for it yet, but I do have the example code. You'd need to undo this change: uqfoundation/mystic@f6f71f5, and then run this example: examples3/test_lub_expected_error1.py -- anything that calls sample on a ouq_model or uses mystic.cache. Basically, this should also trigger the error:

    from ouq_models import NoisyModel
    from toys import function5 as toy; nx = 5; ny = None
    true = dict(mu=.01, sigma=0., zmu=-.01, zsigma=0.)
    truth = NoisyModel('truth', model=toy, nx=nx, ny=ny, **true)
    data = truth.sample([(0,1),(1,10)]+[(0,10)]*(nx-2), pts=-16)

The current mystic test suite should all pass.

[mmckerns]

There is an alternate to making updates to recover the previous behavior. We could, revert the previous behavior in dill, and then add an additional code path (i.e. a flag/setting) that would enable the new behavior. Then code on the alternate path can be advanced to the point that it doesn't have feature loss... and the old code and the switch would be removed. It's not an optimal choice, but it's an alternative to rolling back. I've rescheduled the release to the end of the month, to give some time to sort this out.

[mmckerns]

I'll also work on getting you the error message I'm seeing with mystic.cache.

[anivegesana]

@mmckerns FYI. Using the call log.info("Me: %s" % obj) creates unnecessary calls to __repr__ for obj, even if logging is disabled, which could be arbitrarily complex. The correct call is log.info("Me: %s", obj), which only calls the formatting if it is necessary. Should be an easy find and replace.

[anivegesana]

I believe the mystic code is an example of something that #448 tries to fix, not this PR. As of right now, I am still having trouble getting it to work, so there may be something deeper or some very small corner case that I missed. Will let you know.

[mmckerns]

Using the call log.info("Me: %s" % obj) creates unnecessary calls to __repr__ for obj, even if logging is disabled, which could be arbitrarily complex. The correct call is log.info("Me: %s", obj), which only calls the formatting if it is necessary. Should be an easy find and replace.

I'm assuming this is likely a historical artifact, but yes you are correct.

[anivegesana]

I believe the mystic code is an example of something that #448 tries to fix, not this PR. As of right now, I am still having trouble getting it to work, so there may be something deeper or some very small corner case that I missed. Will let you know.

I found the bug. It was that an if _postproc: statement was missing, so the NumpyPickler was treated as a dill.Pickler which caused a NameError when I tried to access _postproc. For some reason, the error message got obscured and it became an EOFError, which made it harder to track down. The bug that you pointed out is indeed fixed if both #448 and #461 are pulled. You can checkout my branch https://github.com/anivegesana/dill/tree/448-461 to test the merged version.

[mmckerns]

LGTM.