Fix #895: Bug pickling model instances

[burnpanck]

No description provided.

[twiecki]

I don't understand why that fixes things but great that it does. And I assume you confirmed that the test breaks without it but works with it?

[ColCarroll]

Really cool! Taught me a new thing: __getnewargs__.

I'd be happier if name wasn't so heavily weighed down (in that it can be a string or _Unpickled), but can't see how to change that: it looks like python3.4+ has __newargs_ex__ so we could pass a keyword flag, but only for protocol 4+.

Any chance you could document the _Unpickling class, perhaps pointing to the docs, and explaining that it is used as a flag to unpickle distributions?

[burnpanck]

Yes, the test breaks without the fix, at least locally on my machine, at protocol 2 (and probably everything above), under python 3.5. If I'm right, protocol 2 introduced __getnewargs__. That one was there before the fix, but it used None as a sentinel. However, the test for the sentinel was after the check for the model context, which it failed under pickling circumstances.
I changed the sentinel to make it more clear that it is only for pickling purposes.

Instead of having a sentinel, we could also enter special unpickle context and detect that one. I discarded that because that will be less efficient. A third option would be to rely on __reduce__.

In my eyes, the real problem here is that the constructor wants to enforce a model context. To me, the most important aspect for a library like this is a well designed data structure/API that allows for easy programmatic introspection and manipulation. Given that, one can implement event the most fancy user magic on top of it. The other way around is difficult: If the data structure is convolved with magic, all programmatic manipulation will need to work around that magic. In the case at hand here, the pythonic API to create instances is to call the constructor, which is not what this constructor does, so all programmatic manipulation of Distribution will need to be special-cased.

[twiecki]

Thanks @burnpanck! Your thoughts on design are intriguing.

[burnpanck]

I have second thoughts: Changing the sentinel from None to something different will break pickles generated without the patch, when loaded with the patch. While these pickles would anyway fail during loading under normal circumstances, it was possible to work around by initiating a dummy context stack. This fails now. While there are plenty of warnings in the pickle documentation that one shouldn't rely on pickle for long-term storage, there are many people who do (including me). I assume storing model instances is done rarely, but still. We could add a check for None and emit a warning. What do you think?

[twiecki]

I think that's fine. This will be part of RC3 and 3.0. If it came in later I would agree with you.