FIX: loadpkl failed when pklz file contained versioning info

[stilley2]

Summary

loadpkl called pickle.load at file position 0, which would fail if the first line of the file was json versioning info (as when using savepkl with versioning=True). This was fixed by only seeking to position 0 if no versioning information is found.

Acknowledgment

• (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

[effigies]

What about parameterizing this into a single test?

@pytest.mark.parametrize("save_ver", (True, False))
@pytest.mark.parametrize("load_ver", (True, False))
def test_pickle(tmp_path, save_ver, load_ver):
    testobj = 'iamateststr'
    pickle_fname = str(tmp_path / 'testpickle.pklz')
    utils.filemanip.savepkl(pickle_fname, testobj, versioning=save_ver)
    outobj = utils.filemanip.loadpkl(pickle_fname, versioning=load_ver)
    assert outobj == testobj

[stilley2]

Good idea.

[effigies]

This looks reasonable to me. I left a comment about the test. I think from a quick read that all four combinations of read/write ought to work.

[effigies]

@oesteban I think this issue was introduced in the previous version (#2985). I would recommend this to go into the 1.2.2 release, if it's ready in time.

[stilley2]

I actually just removed the versioning argument from loadpkl. All it was doing was altering the error reporting, and it seemed unnecessary (although maybe it was there for a reason?).

[effigies]

Looks like it was added in #2626, and what it seems to be doing is guarding against accessing the undefined pkl_metadata in the next couple lines. I would leave it for now, just because there doesn't seem to be a big gain in removing these lines, and there will be a semantic change.

We can open an issue to review that, though, if you think it will help with clarity.

[stilley2]

Ok I reverted it. However, I don't think it guards against that any more. I'll open up an issue.

[effigies]

LGTM. Locally, the tests fail without the fix, and pass with it, so this is a good regression test. A little surprised that nothing broke before...

👍 to merge when CI passes.