Copying/pickling regression in 0.14

[bocklund]

In PyCalphad, we have some tests that test deepcopying/pickling and our tests have been showing a regression for SymEngine 0.14 on Windows only across Python versions.

I haven't been able to come up with a pure SymEngine reproducer yet, but I've made some notes in an issue in the PyCalphad repo: pycalphad/pycalphad#601 if anyone has ideas on changes that could have caused this or what I can do to make a pure SymEngine reproducible example.

[isuruf]

There were changes to the pickling in the C++ library. If you can make a pure SymEngine reproducible example I can have a look

[bocklund]

I've been trying some today, but haven't been able to create a pure SymEngine reproducer.

Between 0.13 and 0.14, commit 50310f008 in the C++ library changed symengine/serialize-cereal.h such that RCPBasicAwareOutputArchive.save_rcp_basic saves memory addresses and then loads objects by memory address in RCPBasicAwareInputArchive.load_rcp_basic.

Could it be that memory addresses as identifiers and the large expressions we have in PyCalphad ends up re-using or somehow corrupting memory address-based deserialization? That would explain some of the non-determinism we are seeing.

[bjodah]

@bocklund could it be that there's another .so object other than symengine.py that's loaded in your python process, and that also uses cereal (possibly another version thereof)? I was just remembering this comment from a while back. This is a far shot though, since I think we would be seeing issues both before and after the symengine commit you've identified if this was the culprit.

[bocklund]

Not that I know of, but it's not impossible. We've been pinned in PyCalphad to SymEngine==0.13 and it's been totally stable and several different people and machines can reproduce (with PyCalphad) on SymEngine >=0.14 (Windows only).

[bjodah]

When building SymEngine with gcc-14 I see some warnings that made me think of this issue again:

[15/78] Building CXX object symengine/tests/basic/CMakeFiles/test_serialize-cereal.dir/test_serialize-cereal.cpp.o
In file included from /usr/include/c++/14/bits/shared_ptr_atomic.h:33,
                 from /usr/include/c++/14/memory:81,
                 from /work/external/symengine/symengine/utilities/catch/catch.hpp:2948,
                 from /work/external/symengine/symengine/tests/basic/test_serialize-cereal.cpp:1:
In member function ‘std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::fetch_add(__int_type, std::memory_order) [with _ITp = unsigned int]’,
    inlined from ‘std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::operator++(int) [with _ITp = unsigned int]’ at /usr/include/c++/14/bits/atomic_base.h:382:25,
    inlined from ‘SymEngine::RCP<T>::RCP(T*) [with T = const SymEngine::Integer]’ at /work/external/symengine/symengine/symengine_rcp.h:117:26,
    inlined from ‘SymEngine::RCP<T> SymEngine::rcp_static_cast(const RCP<T1>&) [with T2 = const Integer; T1 = const Basic]’ at /work/external/symengine/symengine/symengine_rcp.h:225:25,
    inlined from ‘SymEngine::RCP<const T2> SymEngine::RCPBasicAwareInputArchive<Archive>::load_rcp_basic() [with T = SymEngine::Integer; Archive = cereal::BinaryInputArchive]’ at /work/external/symengine/symengine/type_codes.inc:1:1:
/usr/include/c++/14/bits/atomic_base.h:631:34: warning: ‘unsigned int __atomic_fetch_add_4(volatile void*, unsigned int, int)’ writing 4 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  631 |       { return __atomic_fetch_add(&_M_i, __i, int(__m)); }
      |                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
In member function ‘SymEngine::RCP<const T2> SymEngine::RCPBasicAwareInputArchive<Archive>::load_rcp_basic() [with T = SymEngine::Integer; Archive = cereal::BinaryInputArchive]’:
cc1plus: note: destination object is likely at address zero
In member function ‘std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::fetch_add(__int_type, std::memory_order) [with _ITp = unsigned int]’,
    inlined from ‘std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::operator++(int) [with _ITp = unsigned int]’ at /usr/include/c++/14/bits/atomic_base.h:382:25,
    inlined from ‘SymEngine::RCP<T>::RCP(T*) [with T = const SymEngine::Integer]’ at /work/external/symengine/symengine/symengine_rcp.h:117:26,
    inlined from ‘SymEngine::RCP<T> SymEngine::rcp_static_cast(const RCP<T1>&) [with T2 = const Integer; T1 = const Basic]’ at /work/external/symengine/symengine/symengine_rcp.h:225:25,
    inlined from ‘SymEngine::RCP<const T2> SymEngine::RCPBasicAwareInputArchive<Archive>::load_rcp_basic() [with T = SymEngine::Integer; Archive = cereal::BinaryInputArchive]’ at /work/external/symengine/symengine/type_codes.inc:1:1:
/usr/include/c++/14/bits/atomic_base.h:631:34: warning: ‘unsigned int __atomic_fetch_add_4(volatile void*, unsigned int, int)’ writing 4 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  631 |       { return __atomic_fetch_add(&_M_i, __i, int(__m)); }
      |                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
In member function ‘SymEngine::RCP<const T2> SymEngine::RCPBasicAwareInputArchive<Archive>::load_rcp_basic() [with T = SymEngine::Integer; Archive = cereal::BinaryInputArchive]’:
cc1plus: note: destination object is likely at address zero

I haven't figured out if this warning is spurious or not.

[bocklund]

I've tried debugging this a little bit with an LLM and think I've been able to narrow in on the root cause. I don't claim to fully understand it, but it seems credible and fixes the issue we're having. Prose here is written by me and reflects my understanding, code snippets in the SymEngine diffs are generated by an LLM, as marked.

I'd like to try to move this issue forward because we're currently blocked on upgrading SymEngine in PyCalphad beyond 0.13. That is preventing us from supporting Python 3.14 because SymEngine 0.13.X wheels aren't built for that version and installs from source don't seem to work well out-of-the-box for our users.

Behavior

I still haven't been able to produce an isolated, pure SymEngine reproducer. In PyCalphad, the bad behavior shows up sporadically with the following test:

diff --git a/pycalphad/tests/test_model.py b/pycalphad/tests/test_model.py
index cb1eac6a..9fb08f7f 100644
--- a/pycalphad/tests/test_model.py
+++ b/pycalphad/tests/test_model.py
@@ -261,3 +261,43 @@ def test_error_raised_for_higher_order_reciprocal_parameter():
     # Check that a model with a higher order reciprocal parameter > 2 throws an error
     with pytest.raises(ValueError):
         mod = Model(dbf, ["MO", "NB", "C", "VA"], "PHASE_HIGH_ORDER")
+
+
+@select_database("alnipt.tdb")
+def test_model_pickle_expression_integrity(load_database):
+    """Test that Model pickle preserves SymEngine expression structure.
+
+    The tests a spurious, Windows-specific bug caused by SymEngine 0.14 and
+    the test is known to be flaky and may need to be run many times to fail.
+    With pytest-repeat, running this isolated test 100 times usually has a
+    failure rate around 10%.
+    """
+    dbf = load_database()
+    test_model = Model(dbf, ['NI', 'PT', 'VA'], 'FCC_L12')
+    new_model = pickle.loads(pickle.dumps(test_model))
+
+    # Check 1: Basic equality (same as test_model_pickle)
+    assert test_model == new_model, "Model equality failed after pickle round-trip"
+
+    # Check 2: Expression structure preservation
+    # Compare string representations of all model contributions
+    for attr in ['GM', 'HM', 'SM', 'CPM']:
+        orig_expr = getattr(test_model, attr, None)
+        new_expr = getattr(new_model, attr, None)
+        if orig_expr is not None and new_expr is not None:
+            assert str(orig_expr) == str(new_expr), \
+                f"Expression string mismatch for {attr} after pickle round-trip"
+
+    # Check 3: Database parameter expressions survive round-trip
+    orig_params = test_model._dbe._parameters.all()
+    new_params = new_model._dbe._parameters.all()
+    assert len(orig_params) == len(new_params), \
+        "Parameter count mismatch after pickle round-trip"
+
+    for orig_p, new_p in zip(orig_params, new_params):
+        if 'parameter' in orig_p and 'parameter' in new_p:
+            orig_val = orig_p['parameter']
+            new_val = new_p['parameter']
+            assert str(orig_val) == str(new_val), \
+                f"Parameter expression mismatch: {orig_val} != {new_val}"
+

To reproduce:

1. git clone https://github.com/pycalphad/pycalphad.git
2. apply the patch above
3. update symengine dependency in pyproject.toml to symengine>=0.14
4. uv add pytest-repeat
5. uv run pytest --count 100 .\pycalphad\tests\test_model.py::test_model_pickle_expression_integrity - 100 runs is about 10 seconds on my machine and gives about 10% failure rate and always fails on the first check.

My guess is that this happens with "large" expressions and that is what has made it hard to make a pure reproducer.

Root cause

This is where I've gotten help from the LLM.

The model's claim is that commit 50310f008 added RCPBasicAwareArchive wrappers that violate the One Definition Rule through incompatible save_basic/load_basic overloads for Symbol: one coming from the serialize-ceral.h and the other coming from the pywarapper.cpp bindings.

In serialize-cerial.h:

template <class Archive>
inline void save_basic(Archive &ar, const Symbol &b)
{
    ar(b.__str__());
}

In the Python bindings (pywrapper.cpp):

void save_basic(RCPBasicAwareOutputArchive<cereal::PortableBinaryOutputArchive> &ar, const Symbol &b)
{
    bool is_pysymbol = is_a_sub<PySymbol>(b);
    ar(is_pysymbol);
    ar(b.__str__());
    if (is_pysymbol) {
        RCP<const PySymbol> p = rcp_static_cast<const PySymbol>(b.rcp_from_this());
        PyObject *obj = p->get_py_object();
        std::string pickle_str = pickle_dumps(obj);
        ar(pickle_str);
        ar(p->store_pickle);
        Py_XDECREF(obj);
    }
}

Each implementation is internally consistent (that is, the Python bindings pick up the extra is_pysmbol prefix in both the save and load). It seems like the issue is that our Symbol objects might be getting serialized with the C++ core and others serialized with the Python bindings because of undefined behavior in argument-dependent lookups when save_basic is called by save_rcp_basic. If we save/load from mismatched implementations (C++ vs. Python wrapper) the streams get corrupted because the layout is different.

Apparently this is a Windows-specific thing because Windows statically links everything, rather than using shared libraries that are self-consistent and don't hit function lookup issues.

Potential solution

The LLM proposed a workaround that fixes the issue: prepend a dummy value in the C++ core implementation so that the SymEngine data is consitent regardless of which implementation is resolved.
I'm sure there are proper ways to do this and this particular solution might cause issues if the serialization is handled differently for other non-Python wrappers, but the fact that this fixes the issue, to me, is evidence that the root cause is correctly identified.
Another problem with this particular solution is that detailed PySymbol information is lost if the C++ core is chosen for deserialization, but for PyCalphad that is fine.

C++ core patch (LLM generated):

diff --git a/symengine/serialize-cereal.h b/symengine/serialize-cereal.h
index 0e10816f2..9d93084bf 100644
--- a/symengine/serialize-cereal.h
+++ b/symengine/serialize-cereal.h
@@ -155,6 +155,12 @@ inline void save_basic(Archive &ar, const Basic &b)
 template <class Archive>
 inline void save_basic(Archive &ar, const Symbol &b)
 {
+    // Write is_pysymbol=false for format compatibility with pywrapper.cpp's
+    // save_basic, which writes is_pysymbol + name + optional pickle data.
+    // This ensures the cereal stream format is consistent regardless of which
+    // instantiation the linker selects (fixes pycalphad#601 ODR violation).
+    bool is_pysymbol = false;
+    ar(is_pysymbol);
     ar(b.__str__());
 }
 template <class Archive>
@@ -429,8 +435,21 @@ RCP<const Basic> load_basic(Archive &ar, RCP<const NaN> &)
 template <class Archive>
 RCP<const Basic> load_basic(Archive &ar, RCP<const Symbol> &)
 {
+    // Read is_pysymbol flag for format compatibility with pywrapper.cpp's
+    // load_basic, which writes is_pysymbol + name + optional pickle data.
+    // If is_pysymbol is true, read and discard the pickle data to keep the
+    // stream aligned. The core library cannot create PySymbol objects, so
+    // a plain Symbol is returned in that case (fixes pycalphad#601).
+    bool is_pysymbol;
     std::string name;
+    ar(is_pysymbol);
     ar(name);
+    if (is_pysymbol) {
+        std::string pickle_str;
+        bool store_pickle;
+        ar(pickle_str);
+        ar(store_pickle);
+    }
     return symbol(name);
 }
 template <class Archive>
diff --git a/symengine/tests/basic/test_serialize-cereal.cpp b/symengine/tests/basic/test_serialize-cereal.cpp
index ec6599884..c6d1979ca 100644
--- a/symengine/tests/basic/test_serialize-cereal.cpp
+++ b/symengine/tests/basic/test_serialize-cereal.cpp
@@ -4,6 +4,7 @@
 #include <symengine/parser.h>
 #include <symengine/serialize-cereal.h>
 #include <cereal/archives/binary.hpp>
+#include <cereal/archives/portable_binary.hpp>

 using std::string;

@@ -14,10 +15,12 @@ using SymEngine::cos;
 using SymEngine::dummy;
 using SymEngine::Integer;
 using SymEngine::is_a;
+using SymEngine::load_basic;
 using SymEngine::Number;
 using SymEngine::RCP;
 using SymEngine::RCPBasicAwareInputArchive;
 using SymEngine::RCPBasicAwareOutputArchive;
+using SymEngine::save_basic;
 using SymEngine::sin;
 using SymEngine::Symbol;
 #ifdef HAVE_SYMENGINE_MPFR
@@ -111,6 +114,7 @@ TEST_CASE("Test serialization exception", "[serialize-cereal]")
         // or throw an error
         try {
             Basic::loads(data);
+        } catch (std::bad_alloc &e) {
         } catch (se::SerializationError &e) {
         }
     }
@@ -125,3 +129,103 @@ TEST_CASE("Test serialization shared pointer", "[serialize-cereal]")
     REQUIRE(new_expr->get_args()[0]->get_args()[0].get()
             == new_expr->get_args()[1]->get_args()[0].get());
 }
+
+TEST_CASE("Symbol serialization format compatible with PySymbol protocol",
+          "[serialize-cereal]")
+{
+    // The pywrapper.cpp save_basic for Symbol writes:
+    //   [is_pysymbol:bool] [name:string] [optional pickle data]
+    // The template save_basic in serialize-cereal.h must produce
+    // the same format prefix: [is_pysymbol:bool] [name:string]
+    //
+    // If the formats differ, the linker may pick mismatched versions
+    // causing cereal stream corruption (pycalphad#601).
+
+    auto x = se::symbol("test_sym");
+
+    // Serialize the Symbol using the template save_basic
+    // (which is the only version visible in this translation unit)
+    std::ostringstream oss;
+    {
+        RCPBasicAwareOutputArchive<cereal::PortableBinaryOutputArchive> ar(oss);
+        save_basic(ar, *x);
+    }
+
+    // Deserialize using the PySymbol-compatible format:
+    // read is_pysymbol flag first, then name
+    std::istringstream iss(oss.str());
+    {
+        RCPBasicAwareInputArchive<cereal::PortableBinaryInputArchive> ar(iss);
+        bool is_pysymbol;
+        std::string name;
+        ar(is_pysymbol);
+        ar(name);
+        REQUIRE(is_pysymbol == false);
+        REQUIRE(name == "test_sym");
+    }
+}
+
+TEST_CASE("Symbol load_basic format compatible with PySymbol protocol",
+          "[serialize-cereal]")
+{
+    // Construct a byte stream in PySymbol-compatible format:
+    // [is_pysymbol=false] [name="load_test"]
+    // The template load_basic must be able to read this format.
+
+    std::ostringstream oss;
+    {
+        RCPBasicAwareOutputArchive<cereal::PortableBinaryOutputArchive> ar(oss);
+        bool is_pysymbol = false;
+        std::string name = "load_test";
+        ar(is_pysymbol);
+        ar(name);
+    }
+
+    std::istringstream iss(oss.str());
+    {
+        RCPBasicAwareInputArchive<cereal::PortableBinaryInputArchive> ar(iss);
+        RCP<const Symbol> dummy_ptr;
+        RCP<const Basic> result = load_basic(ar, dummy_ptr);
+        REQUIRE(is_a<Symbol>(*result));
+        REQUIRE(result->__str__() == "load_test");
+    }
+}
+
+TEST_CASE("Symbol load_basic handles PySymbol data in stream",
+          "[serialize-cereal]")
+{
+    // Construct a byte stream as if pywrapper's save_basic wrote a PySymbol:
+    // [is_pysymbol=true] [name="pysym"] [pickle_str="fake"] [store_pickle=true]
+    // The template load_basic must read and skip the PySymbol data
+    // to keep the stream aligned, even though it can't create a PySymbol.
+
+    std::ostringstream oss;
+    {
+        RCPBasicAwareOutputArchive<cereal::PortableBinaryOutputArchive> ar(oss);
+        bool is_pysymbol = true;
+        std::string name = "pysym";
+        std::string pickle_str = "fake_pickle_data";
+        bool store_pickle = true;
+        ar(is_pysymbol);
+        ar(name);
+        ar(pickle_str);
+        ar(store_pickle);
+        // Write a sentinel value after to verify stream alignment
+        std::string sentinel = "SENTINEL";
+        ar(sentinel);
+    }
+
+    std::istringstream iss(oss.str());
+    {
+        RCPBasicAwareInputArchive<cereal::PortableBinaryInputArchive> ar(iss);
+        RCP<const Symbol> dummy_ptr;
+        RCP<const Basic> result = load_basic(ar, dummy_ptr);
+        // Should get a plain Symbol (PySymbol data is discarded in core library)
+        REQUIRE(is_a<Symbol>(*result));
+        REQUIRE(result->__str__() == "pysym");
+        // Verify stream is still aligned by reading the sentinel
+        std::string sentinel;
+        ar(sentinel);
+        REQUIRE(sentinel == "SENTINEL");
+    }
+}

Python bindings test that matches the PyCalphad usage pattern (LLM generated):

diff --git a/symengine/tests/test_pickling.py b/symengine/tests/test_pickling.py
index a51a325..a8f7e01 100644
--- a/symengine/tests/test_pickling.py
+++ b/symengine/tests/test_pickling.py
@@ -73,3 +73,53 @@ def test_pickling_roundtrip():
     _check_pickling_roundtrip(x+y)
     _check_pickling_roundtrip(Dummy('d'))
     _check_pickling_roundtrip(Dummy('d') - z)
+
+
+class TaggedSymbol(Symbol):
+    """A Symbol subclass that becomes a PySymbol at C++ level."""
+    def __init__(self, name):
+        super().__init__(name=name)
+        self._tag = "tagged"
+
+    def __reduce__(self):
+        return (self.__class__, (self.name,))
+
+    def __eq__(self, other):
+        if isinstance(other, TaggedSymbol):
+            return self.name == other.name
+        return NotImplemented
+
+    def __hash__(self):
+        return super().__hash__()
+
+
+def test_pysymbol_in_expression_roundtrip():
+    """Test that PySymbol identity survives pickle when embedded in expressions.
+
+    Regression test for symengine.py#514: ODR violation between
+    serialize-cereal.h and pywrapper.cpp can cause PySymbol data to be
+    lost or the cereal stream to become misaligned during serialization.
+    """
+    x = TaggedSymbol("x")
+    y = Symbol("y")
+
+    # Expression tree containing both PySymbol and plain Symbol
+    expr = x**2 + y + 1
+
+    try:
+        data = pickle.dumps(expr)
+        restored = pickle.loads(data)
+
+        # The expression must survive intact
+        assert str(restored) == str(expr), \
+            f"Expression corrupted: expected '{expr}', got '{restored}'"
+
+        # Find the TaggedSymbol in the restored expression's free symbols
+        orig_tagged = [s for s in expr.free_symbols if isinstance(s, TaggedSymbol)]
+        rest_tagged = [s for s in restored.free_symbols if isinstance(s, TaggedSymbol)]
+
+        assert len(orig_tagged) == len(rest_tagged), \
+            f"TaggedSymbol count changed: {len(orig_tagged)} -> {len(rest_tagged)}. " \
+            f"Types in restored: {[type(s).__name__ for s in restored.free_symbols]}"
+    finally:
+        x._unsafe_reset()