Restore compatibility with functions pickled with 0.4.0

[nikhaldi]

Release 0.4.1 introduced a change to arguments of _fill_function
which meant that functions pickled before that couldn't be
unpickled anymore.

Addresses #126

[codecov-io]

Codecov Report

Merging #128 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   83.94%   84.09%   +0.15%     
==========================================
  Files           2        2              
  Lines         517      522       +5     
  Branches       90       91       +1     
==========================================
+ Hits          434      439       +5     
  Misses         62       62              
  Partials       21       21

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	84% <100%> (+0.15%)	⬆️

---

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 c7ea597...2b8809c. Read the comment docs.

[nikhaldi]

I'm not very happy I had to skip the tests for Python 3. My pickle/Python 3 knowledge is not deep, gladly taking suggestions for handling this better. Worst case, I could add separate set of tests for Python 3.

[ogrisel]

LGTM as it is. In the long run we should probably setup a folder of sample pickle files to check version compat for the latest 2 consecutive versions but I would rather not delay 0.4.2 for this.

[ogrisel]

Actually, this does not address @pitrou's comment in #126 (comment): something pickled in 0.4.2 won't be loadable in 0.4.0.

But I don't see how it can be fixed.

[nikhaldi]

Agreed, I don't think there's any way to restore forward compatibility now. Like I said elsewhere, ultimately I think you need to carry around a protocol version marker somewhere. Then you can at least give an explicit error if you get a pickle of a version that you can't support.

[nikhaldi]

Actually, I take that back - there are ways to restore forward compatibility but they may be quite hacky. Basically, the module argument could be attached to one of the existing arguments to _fill_function in some way. I'll propose something on a different branch because I'm not sure it's really a good idea.

[pitrou]

@nikhaldi or we bite the bullet and transform fill_function to take most of its arguments as a state dictionary, which makes it trivial to handle missing or unexpected data. I suspect more data may need to be added in the future... For example __qualname__ isn't currently replicated.

[nikhaldi]

@pitrou yeah, obviously at some point you have to redesign fill_function.

Ok, here's a fix which actually makes pickles compatible with 0.4.0 again: https://github.com/nikhaldi/cloudpickle/compare/feat/function_pickle_compat...nikhaldi:feat/function_pickle_forward_compat?expand=1

It works but it's a hack. I could be convinced to include this or not, I don't have strong feelings either way.

[pitrou]

@pitrou yeah, obviously at some point you have to redesign fill_function.

Then let's redesign it now. Since we just broke compatibility, we might as well break it soon again than wait for next year.

Ok, here's a fix which actually makes pickles compatible with 0.4.0 again

I'd rather avoid this :-)

[Peque]

You don't even need the else here:

if len(args) == 5:
    # [...]
    return _fill_function_internal(*updated_args)
return _fill_function_internal(*args)

😊

[rgbkrk]

I currently agree with merging this as is, with the caveat that we should remove the workaround at some point in the future.

[rgbkrk]

Whoops, accidentally closed and reopened, retriggering Travis.

[ogrisel]

Ok merging this as it to release 0.4.2.

[ogrisel]

Hum I had not read @pitrou's comment in #128 (comment). I will try to address it now.

[Peque]

@ogrisel Maybe an issue should be opened to revert this fix in future versions?

[HyukjinKwon]

Thank you guys for fixing this ..