Sparkles now breaking on pickles

[jeanconn]

ska3-fido: python -m sparkles /home/jeanconn/sfe/MAR0220/prelim/MAR0220_prelim_2020_050_12_48_25_332.pkl.gz
Reading pickle file /home/jeanconn/sfe/MAR0220/prelim/MAR0220_prelim_2020_050_12_48_25_332.pkl.gz
Processing obsid 22641
Traceback (most recent call last):
  File "/proj/sot/ska3/flight/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/proj/sot/ska3/flight/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/sparkles/__main__.py", line 2, in <module>
    main()
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/sparkles/core.py", line 78, in main
    open_html=args.open_html)
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/sparkles/core.py", line 130, in run_aca_review
    open_html=open_html, context=context)
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/sparkles/core.py", line 177, in _run_aca_review
    aca.set_stars_and_mask()  # Not stored in pickle, need manual restoration
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/sparkles/core.py", line 536, in set_stars_and_mask
    self.set_stars(filter_near_fov=False)
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/proseco/core.py", line 684, in set_stars
    stars = StarsTable.from_agasc(self.att, date=self.date, logger=self.log)
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/proseco/core.py", line 1044, in from_agasc
    q_att = Quat(att)
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/Quaternion/Quaternion.py", line 184, in __init__
    q = attitude.q
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/Quaternion/Quaternion.py", line 275, in _get_q
    return self._q.reshape(self.shape+(4,))
  File "/proj/sot/ska3/flight/lib/python3.6/site-packages/Quaternion/Quaternion.py", line 240, in shape
    return self._shape
AttributeError: 'Quat' object has no attribute '_shape'

This failure also suggests to me that both sparkles and Quaternion need new tests.

[javierggt]

That is odd... _shape is set in the constructor, and at the end of the constructor there is even an assertion checking it (https://github.com/sot/Quaternion/blob/master/Quaternion/Quaternion.py#L226). Also, there is no return statement in the constructor, so how does it pass through the assertion?

[javierggt]

ah! I see. this is called before the init method finishes. scratch that... I still don't get it.

[taldcroft]

Pickle doesn't generally call the constructor. I suspect that if you pickle a "new" Quaternion that will work but the one coming from MATLAB fails.

[jeanconn]

Thanks @taldcroft! @javierggt figured that out too and I was immediately reassured that this is a tiny corner case then (and not really a test failure, but perhaps I should have thought of this special case of the MATLAB-made pkl files)

[jeanconn]

We might want to use the pkl files we already have for something without needing to use an environment with the old Quaternion module in it, though, so we probably still want a fix for this (if there is a way in Quaternion that isn't ridiculous).

[jeanconn]

And by "@javierggt figured that out too" I meant he immediately hit on the fact that the problem is caused by this Quaternion coming from the pickle. I confess I'm not completely clear on the way this breaks (We're trying to initialize a new Quat from a pickled-Quat but the pickled Quat is still calling methods/code in the new Quat code... but not enough of them to get _shape initialized...).

[javierggt]

I can confirm that this is because pickle assigns the wrong property when unpickling. I'm guessing pickle does not store information of properties assigned, as opposed to bound methods.

To explain this better, here is an example. This is how my current directory looks like:

(ska3) javierg test $ ls
example_1.py example_2.py test_1.py    test_2.py

The example_*.py scripts contain the same class, but changing the a property. The test_1.py creates an object, prints it, and pickles it. test_2.py unpickles it and prints it. The repr method is different in both versions, so one can see whether the unpickled object uses the corrrect repr method but the incorrect a property.

Then I do this:

(ska3) javierg test $ rm -fr __pycache__ example.py
(ska3) javierg test $ ln -s example_1.py example.py
(ska3) javierg test $ python test_1.py 
version 1: 1.0 = 1.0
(ska3) javierg test $ rm -fr __pycache__ example.py
(ska3) javierg test $ ln -s example_2.py example.py
(ska3) javierg test $ python test_2.py 
version2: 2.0 = 1.0

The contents of each file: example_1.py

class Example:
    def __init__(self, a):
        self.a = a
    def __repr__(self):
        return f'version 1: {self.a} = {self._a}'
    def _set_a(self, a):
        self._a = a
    def _get_a(self):
        return self._a
    a = property(_get_a, _set_a)

example_2.py

class Example:
    def __init__(self, a):
        self.a = a
    def __repr__(self):
        return f'version2: {self.a} = {self._a}'
    def _set_a(self, a):
        self._a = a/2
    def _get_a(self):
        return self._a*2
    a = property(_get_a, _set_a)

test_1.py

import pickle
from example import *
a = Example(1.)
print(a)
with open('example.pkl', 'wb') as f:
    pickle.dump(a, f)

test_2.py

import pickle
from example import *
with open('example.pkl', 'rb') as f:
    a = pickle.load(f)
print(a)

[javierggt]

actually, what I described above is the expected behavior, it uses all methods from the new definition.

[javierggt]

so what should one do with older versions of pickled classes?

In serialization libraries I have seen, the object can be updated to the current class version as it is being loaded, but I don't think that is the case with pickle. Can one define this somehow?

[taldcroft]

so what should one do with older versions of pickled classes?

Should be possible to make things work using the methods described here: https://docs.python.org/3/library/pickle.html#pickling-class-instances

[taldcroft]

Hopefully I'm not off track, but my assumption was just that the old pickled Quaternion doesn't have a _shape attribute that gets tossed into the object __dict__ by pickle. So __setstate__ can check if that is in state and if not put it there before calling super().__setstate__().

[javierggt]

That seems to be the way. However, I don't think one calls super().setstate(). None of the examples in that page do that, and when I do I get

AttributeError: 'super' object has no attribute 'setstate'

If we are going to start supporting versions of pickled objects, then perhaps we should start adding a version number to the class and so on.

[javierggt]

I added a quick fix for this. You can see it in commit fcaf49124519bbc1c6.

This might not be the best solution, because it tries to be smart and determine what to do based on the pickled dictionary. While this is ok for now, if things change it will get harder. In that case it is better to have a version number, and we might as well start doing that already.

[javierggt]

and by the way... it was not just adding the shape. I also had to reshape the data members. Also, the current test might not cover all cases (it depends on whether the quaternion was set with q, equatorial or transformation).

[javierggt]

after some checking... I concluded that actually one does not need to reshape the data members. In my mind I had this idea that the data members (in particular _q) should have one extra dimension when compared to the older version, but that does not seem to be true because every accessor does a reshape anyway.

I therefore decided to get rid of the reshaping I was doing in setstate.