Pickling

[louispotok]

Took a stab at implementing pickling for EvalFactor. This is mainly to show the approach.
I think the verbose error message might be overkill
but it just shows the flexibility of this pattern.

[louispotok]

This is a followup to this pull request: #67

I couldn't figure out how to submit a pull request from my fork of @chrish42 's fork -- seems like I didn't have permission. Anyway, this outlines an approach to pickling, unpickling, and testing.

A few questions for @njsmith and @chrish42 :

1. Location of pickling tests -- do you prefer to have them all in one place, or spread them around?
   I see both conventions elsewhere in this library. It looks like simple tests are inline with class definitions, and high level tests are all in their own module. I think it makes sense to keep these by themselves, but just wanted to check.
2. I switched away from using tuples to represent state and am using a dict instead. IMO this will allow better inter-version compatability and is overall safer.
3. I thought about versioning a bit. As I understand it, there are two pieces to think about:
   • At runtime: object gets pickled in one version and unpickled in a different version:
     • As long as all the expected data is available, this should work fine.
     • Otherwise, this can be handled by the unpickler on a case-by-case basis.
     • For example, in my implementation this will be extremely verbose, and give either an error or a warning depending on what's missing.
     • And each version gets to decide for itself what data is required vs optional in order to unpickle.
     • I don't think it actually needs to be this verbose for EvalFactor, just wanted to show the way I was thinking about it.
   • At test time: how will we know what whether code changes make it impossible to unpickle older versions, or whether an old version unpickles to something different than we expect?
     • I solved this by just storing a version-by-version history of what each object looks like pickled.
     • This ensures that we'll catch when new versions stop being able to unpickle something that was pickled by an older version.
     • And if we decide to stop supporting an older version, we can just delete it from the history (and add a descriptive message to the unpickling function).
     • There may be a better place to store this data than just inline in the file, but I couldn't think of a good way to do it. Suggestions welcome.
     • Similarly, I'm just generating it now by running it locally and copying it into the file. Again, there may be a better way to do it.
     • This may get cumbersome to maintain, so I'm open to talking about it more.
4. Git history -- I want to acknowledge the work that chrish put in and I did end up using some of his code. So are you okay merging those partial, work in progress commits? Or would you prefer I clean up that history?

I think that covers most of the concerns. Does this approach work for you? Any other ideas for how to do this?

[louispotok]

Hm, don't know why the build failed -- looks unrelated to the pull request, though.

[louispotok]

@njsmith , does this look like the right approach to you, or at least close enough to keep moving? Don't want to be pushy but I'd like to either make some progress or talk more about the best way to do this.

[chrish42]

Hi @louispotok. Thanks for taking a stab at this. This is a case where a lot of the discussion was between njs and me in person, so the bug report and pull request don't contain all the design talk that happened.

The challenging part for this is less saving a pickle and loading it back in a new version, and more the test infrastructure around it all, that makes sure that we don't accidentally break compatibility, and that if we do, we catch it before the release. And this needs to be very easy to use by the person doing the releases, and by people running tests. So a prototype for this part, that exhibits the qualities described previously, would probably be a better conversation starter for pull request. Once that part is agreed upon and implemented, writing the __setstate__ methods is the easy bit, at least in my opinion.

I'll dig in the coming days and find my notes for my discussions with njs about this, and post more about it all here. But I wanted to at least give a first reply to you here. (I assume njs is busy or otherwise away.)

[louispotok]

Ah! Okay, got it, thanks for the explanation. I'll wait for you to find the
notes.

In the meanwhile, is there a library you know of that has solved this
problem in a respectable way?
On May 8, 2016 15:07, "Christian Hudon" notifications@github.com wrote:

Hi @louispotok https://github.com/louispotok. Thanks for taking a stab
at this. This is a case where a lot of the discussion was between njs and
me in person, so the bug report and pull request don't contain all the
design talk that happened.

The challenging part for this is less saving a pickle and loading it back
in a new version, and more the test infrastructure around it all, that
makes sure that we don't accidentally break compatibility, and that if we
do, we catch it before the release. And this needs to be very easy to use
by the person doing the releases, and by people running tests. So a
prototype for this part, that exhibits the qualities described previously,
would probably be a better conversation starter for pull request. Once that
part is agreed upon and implemented, writing the setstate methods is
the easy bit, at least in my opinion.

I'll dig in the coming days and find my notes for my discussions with njs
about this, and post more about it all here. But I wanted to at least give
a first reply to you here. (I assume njs is busy or otherwise away.)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#86 (comment)

[njsmith]

Hi @louispotok -- thanks for looking at this, and sorry for being slow to respond!

The build failures look real, though some of those logs are a bit overwhelming -- I'm not sure what's going on with that. Probably some tweaks to .travis.yml are needed... But the two issues that I noticed skimming though are (1) raise Class, arg is invalid syntax in py3; instad write raise Class(arg), which works on all versions, (2) the tests make deprecation warnings into errors (to force us to notice when we're using stuff that's going to go away), and then the code is accessing the DesignInfo.builder attribute, which is deprecated, which makes the test fail.

I'll also hope that @chrish42 manages to find his notes, because I don't remember all those discussions off the top of my head either :-). But definitely we will want some tests that have saved versions of the pickles from older versions to check that they still load, and it would be very nice if these tests were written in a way that allowed us to hit a button and dump a new batch of test pickles into the test directory, so that we don't have to manually add new test cases every time we adjust the pickle format -- does that make sense?

[louispotok]

Closing this, since #104 is implementing the same functionality and I don't intend to pick this back up.