REG: sympy 1.9 breaks unpickling for symbols stored with sympy 1.8

[neutrinoceros]

The new version of sympy breaks yt's CI (https://tests.yt-project.org/job/yt_py38_latest_deps/386/)
Here's a minimal example to reproduce the issue we're facing.
I first ran this script using sympy 1.8, then upgraded to 1.9 and ran it again

import pickle

import sympy as sp
from sympy.abc import x
from packaging.version import Version

SP_VERSION = Version(sp.__version__)
PFILE = "/tmp/x.pickle"

assert isinstance(x, sp.Symbol)

if SP_VERSION < Version("1.9"):
    with open(PFILE, "wb") as fh:
        pickle.dump(x, fh)

with open(PFILE, "rb") as fh:
    x_ = pickle.load(fh)    

output:

Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t_pickle_sympy.py", line 17, in <module>
    x_ = pickle.load(fh)
AttributeError: 'Symbol' object has no attribute '__dict__'

I see in sympy 1.9's release notes that there have been some changes related to pickling, however I see no mention of this kind of breaking change so I'm assuming that it wasn't intended. I'll try to bisect to the exact commit/PR that broke this.

[neutrinoceros]

the exact breaking commit was 6a1c295 (#21260)

[oscarbenjamin]

To be clear what you mean is that a pickle created with sympy 1.8 can not be unpickled with sympy 1.9. Is that correct?

[neutrinoceros]

Yes, exactly.

[oscarbenjamin]

I'm not sure how easy this is to fix. There are a few related problems. One is that Symbol uses __slots__ so you can make the error go away like this:

diff --git a/sympy/core/symbol.py b/sympy/core/symbol.py
index fa43635..be57d45 100644
--- a/sympy/core/symbol.py
+++ b/sympy/core/symbol.py
@@ -201,7 +201,7 @@ class Symbol(AtomicExpr, Boolean):
 
     is_comparable = False
 
-    __slots__ = ('name',)
 
     is_Symbol = True
     is_symbol = True

Does that fix all the problems for yt? It's probably not the right fix for sympy because it allows arbitrary attribute assignment on symbols which I think is deliberately disallowed.

[neutrinoceros]

Pinging @Xarthisius who has admin rights on the server where we're seeing this error.
I don't know how easily we could regenerate our pickled objects without taking a risk of letting another bug slip in. Maybe your patch is a workable solution is to transition, but I don't have authority here.

[oscarbenjamin]

I didn't consider cross-version pickling compatibility when making #21260. It's not clear (to me) what should be expected for portability with pickle.

The pickling protocols are quite complicated to implement. It isn't clear to me how __slots__ is used but it seems to be involved somehow.

Here any pickle made with SymPy 1.8 was pickled using the __getstate__/__setstate__ protocol to set the _assumptions dict for Symbol. However in SymPy 1.9 Symbol pickles using __getnewargs_ex__ to pass the assumptions to the constructor so __setstate__ is not needed.

The original issue #21121 (comment) was I think due to the fact that __setstate__ violates the immutability of Basic which then causes problems with the object hash and with the cache and some other things.

Maybe it's enough to implement __setstate__ but not __getstate__ for Symbol in SymPy 1.9:

diff --git a/sympy/core/symbol.py b/sympy/core/symbol.py
index fa43635..60783be 100644
--- a/sympy/core/symbol.py
+++ b/sympy/core/symbol.py
@@ -303,6 +303,10 @@ def __new_stage2__(cls, name, **assumptions):
     def __getnewargs_ex__(self):
         return ((self.name,), self.assumptions0)
 
+    def __setstate__(self, state):
+        for name, value in state.items():
+            setattr(self, name, value)
+
     def _hashable_content(self):
         # Note: user-specified assumptions not hashed, just derived ones
         return (self.name,) + tuple(sorted(self.assumptions0.items()))

I think that maybe means that SymPy 1.9 can unpickle both the old and the new pickles but the new pickles will still use the __getnewargs_ex__ protocol. It seems to fix it for the example code in the OP.

[oscarbenjamin]

I've opened #22245 which I think fixes this. It makes it possible to unpickle pickles created by either SymPy 1.8 or 1.9. I've also tested that the new pickles created in SymPy 1.9 can be unpickled by SymPy 1.8. If we were to release that fix in 1.9.1 then with the exception of 1.9 the pickles from all versions of SymPy would be compatible with each other.

Note that old pickles would still be subject to the bug in #21260

I've only tested this in simple cases so it would be good if someone could check that this actually fixes the problem for yt.

[neutrinoceros]

Unfortunately we will not be able to test your patch thoroughly any time soon, but I believe your simple cases testing should be sufficient. I'm hoping it can be considered for a bug fix release

[oscarbenjamin]

I think it can be considered for a bugfix release. I was expecting more bug reports but so far this is the only reported regression for 1.9.

[neutrinoceros]

Excellent, thanks for your transparency and responsiveness. I appreciate it.

[oscarbenjamin]

I haven't seen any other regression reports so I'm preparing a release to fix this. I'm not sure when I will find time to actually put it out though.

[neutrinoceros]

Great, no rush on our side. Thank you !

[yurivict]

The 4.0.2 release of Yt disables sympy-1.9 because of this issue, which makes Yt unbuildable on systems with sympy-1.9

Any chance to resolve this?

[neutrinoceros]

This is an unfortunate side effect that we didn't anticipate on the yt side, I'm sorry about that !
I do not know if we can resolve this in yt itself now, but I note that we specify sympy != 1.9 (not sympy < 1.9), so hopefully any new sympy release (1.9.1 or 1.10) could help making this case much rarer.

If there's a way we can resolve this from the yt side that we haven't thought of I'll be happy to help with it.

[oscarbenjamin]

A SymPy 1.10 release is imminent.

[neutrinoceros]

@yurivict we ended up patching this limitation away in the conda-forge version of yt 4.0.2, you could just do the same thing for FreeBSD. For full context the issue we had with sympy 1.9 was specifically encountered in the context of our CI, where previously validated results were stored as pickle files. Yt users were not actually affected.

see conda-forge/yt-feedstock@b988c65

[neutrinoceros]

I think that another similar problem slipped in sympy 1.10, and yt's CI is once again broken with:

AttributeError: 'One' object has no attribute '__dict__'

[oscarbenjamin]

I think the fix is the same as before it's just that apparently it should be added to Basic rather than Symbol:

diff --git a/sympy/core/basic.py b/sympy/core/basic.py
index fc6b6fe..f156959 100644
--- a/sympy/core/basic.py
+++ b/sympy/core/basic.py
@@ -132,6 +132,10 @@ def copy(self):
     def __getnewargs__(self):
         return self.args
 
+    def __setstate__(self, state):
+        for name, value in state.items():
+            setattr(self, name, value)
+
     def __getstate__(self):
         return None
 

I would like to know if that can be tested to see that it does make everything work in yt though. In the case of Symbol there is an attribute _assumptions that should be set when unpickling which is why I localised the fix there. Here __setstate__ is just being called with an empty dict and doesn't do anything but the protocol apparently requires either __setstate__ or __dict__. The issue is not just with S.One it would also happen if you pickled S(2) e.g.:

import pickle

import sympy as sp
from sympy import S
from packaging.version import Version

SP_VERSION = Version(sp.__version__)
PFILE = "/tmp/x.pickle"

if SP_VERSION < Version("1.9"):
    with open(PFILE, "wb") as fh:
        pickle.dump(S(2), fh)

with open(PFILE, "rb") as fh:
    x_ = pickle.load(fh) 

[neutrinoceros]

Thank you for your promptly response. I can't speak for the team but right now I'm not sure anyone with the necessary admin permissions has time to try it extensively.
That said I believe that the whole problem we're having comes down to unpicking old instances of unyt.unyt_array, and
I think this can be reproduced by

import pickle
from unyt import unyt_array
import sympy as sp
from sympy import S
from packaging.version import Version

SP_VERSION = Version(sp.__version__)
PFILE = "/tmp/x.pickle"

if SP_VERSION < Version("1.9"):
    with open(PFILE, "wb") as fh:
        pickle.dump(unyt_array([1, 2], "cm"), fh)

with open(PFILE, "rb") as fh:
    x_ = pickle.load(fh)

Also, I can confirm that your proposed patch fixes it !

[oscarbenjamin]

I opened a PR against master which seems to pass the existing tests. If another reason arises to make 1.10.1 release then I'll put that in. Otherwise I want unyt to test this because my previous attempt wasn't fully tested and now turns out not to have fixed everything.

[oscarbenjamin]

The PR: #23217

[neutrinoceros]

Thanks a lot. I'll propose a test in unyt itself, which should be run against the dev version of sympy to alleviate your maintenance burden.

[neutrinoceros]

I also confirm that #23217 fixes my test

[oscarbenjamin]

I've just released SymPy 1.10.1. Can you confirm if that resolves this issue?

[neutrinoceros]

All good ! I've checked that both unyt and yt CI were compatible. Thank you so much, I'll close !

• unyt https://github.com/neutrinoceros/unyt/runs/5617468514?check_suite_focus=true
• yt MNT: check sympy 1.10.1 yt-project/yt#3850

[oscarbenjamin]

Thanks for testing!