From d99845439d1bfa3bc8b89632c7298bde0e070313 Mon Sep 17 00:00:00 2001 From: adrinjalali Date: Sat, 26 Sep 2026 17:45:24 +0100 Subject: [PATCH 1/3] FIX a raising reduce shouldn't make dump fail --- docs/changes.rst | 11 ++++++++++ skops/io/_general.py | 12 +++++++++-- skops/io/tests/test_persist.py | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index 5ff4a358..162dde6e 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -9,6 +9,17 @@ skops Changelog :depth: 1 :local: +v0.17 +----- +- Fix a regression since v0.12.0 where saving an object whose ``__reduce__`` + raises failed at dump time. ``__reduce__`` is called on every object to + detect a plain constructor call, but Cython extension types with a + ``__cinit__`` and no ``__reduce__`` raise instead of returning one; pandas' + ``BlockValuesRefs`` is such a type and sits inside every ``Series``, + ``DataFrame`` and ``Index``, so any object holding one could not be saved. + Such objects are now saved through ``__getstate__``/``__dict__`` again, as + before v0.12.0. :pr:`XXX` by `Adrin Jalali`_. + v0.16 ----- - Fix loading of time-zone-aware ``datetime.datetime`` and ``datetime.time`` diff --git a/skops/io/_general.py b/skops/io/_general.py index ef557ec6..097538da 100644 --- a/skops/io/_general.py +++ b/skops/io/_general.py @@ -410,8 +410,16 @@ def object_get_state(obj: Any, save_context: SaveContext) -> dict[str, Any]: # ``datetime.timezone`` for instance returns ``(timezone, (offset,), None)``. # If the constructor is the same as the object's type, then we consider it # safe to call it with the specified arguments. - - reduce_output = obj.__reduce__() + # + # The call is only a probe for that shape. Objects that cannot be pickled + # raise from ``__reduce__``, e.g. Cython extension types with a + # ``__cinit__`` such as ``pandas._libs.internals.BlockValuesRefs``, and for + # those we fall through to the ``__getstate__``/``__dict__`` path below, as + # we did before this probe existed. + try: + reduce_output = obj.__reduce__() + except Exception: + reduce_output = () if ( len(reduce_output) >= 2 and reduce_output[0] is type(obj) diff --git a/skops/io/tests/test_persist.py b/skops/io/tests/test_persist.py index 387a1b1f..f725edf1 100644 --- a/skops/io/tests/test_persist.py +++ b/skops/io/tests/test_persist.py @@ -1520,6 +1520,44 @@ def test_custom_reduce(): assert obj.value == loaded_obj.value +# This class is here as opposed to inside the test because it needs to be importable. +# It mimics Cython extension types with a ``__cinit__`` and no ``__reduce__``, +# such as ``pandas._libs.internals.BlockValuesRefs``, whose ``__reduce__`` +# raises instead of returning a value. +class RaisingReduce: + def __init__(self): + self.x = 3 + + def __reduce__(self): + raise TypeError("no default __reduce__ due to non-trivial __cinit__") + + +def test_reduce_raises_falls_back_to_dict(): + # ``__reduce__`` is only called to probe for a constructor call; objects + # whose ``__reduce__`` raises must still be persisted through ``__dict__``, + # as they were before the probe was added, see gh-450. + dumped = dumps(RaisingReduce()) + loaded_obj = loads(dumped, trusted=[RaisingReduce]) + assert type(loaded_obj) is RaisingReduce + assert loaded_obj.x == 3 + + +def test_object_holding_pandas_can_be_dumped(): + # pandas Series, DataFrame and Index objects hold a ``BlockValuesRefs``, + # whose ``__reduce__`` raises, so any object containing one failed to dump, + # see gh-450. Loading pandas objects is not supported, so only dumping and + # auditing are checked here. The default ``RangeIndex`` does not hold such + # a reference, hence the explicit index. + pd = pytest.importorskip("pandas") + + class Holder: + def __init__(self): + self.series = pd.Series([1, 2, 3], index=["a", "b", "c"]) + + dumped = dumps(Holder()) + assert "pandas.Series" in get_untrusted_types(data=dumped) + + def test_loss_get_state_unsupported_reduce(): # loss_get_state understands the two shapes of __reduce__ output produced by # scikit-learn's loss classes, and refuses anything else. From 73154309c650b5bacaa29c4f676497e6d92aa816 Mon Sep 17 00:00:00 2001 From: adrinjalali Date: Sun, 27 Sep 2026 07:22:18 +0100 Subject: [PATCH 2/3] review / fix --- docs/changes.rst | 11 ++++++----- skops/io/tests/test_persist.py | 4 +++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index 162dde6e..f151e207 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -14,11 +14,12 @@ v0.17 - Fix a regression since v0.12.0 where saving an object whose ``__reduce__`` raises failed at dump time. ``__reduce__`` is called on every object to detect a plain constructor call, but Cython extension types with a - ``__cinit__`` and no ``__reduce__`` raise instead of returning one; pandas' - ``BlockValuesRefs`` is such a type and sits inside every ``Series``, - ``DataFrame`` and ``Index``, so any object holding one could not be saved. - Such objects are now saved through ``__getstate__``/``__dict__`` again, as - before v0.12.0. :pr:`XXX` by `Adrin Jalali`_. + ``__cinit__`` and no ``__reduce__`` raise instead of returning one. pandas' + ``BlockValuesRefs`` is such a type and every pandas ``Index`` except + ``RangeIndex`` holds one, so objects containing such an index, or a + ``Series`` or ``DataFrame`` using one, could not be saved. Such objects are + now saved through ``__getstate__``/``__dict__`` again, as before v0.12.0. + :pr:`550` by `Adrin Jalali`_. v0.16 ----- diff --git a/skops/io/tests/test_persist.py b/skops/io/tests/test_persist.py index f725edf1..47aeae3b 100644 --- a/skops/io/tests/test_persist.py +++ b/skops/io/tests/test_persist.py @@ -1555,7 +1555,9 @@ def __init__(self): self.series = pd.Series([1, 2, 3], index=["a", "b", "c"]) dumped = dumps(Holder()) - assert "pandas.Series" in get_untrusted_types(data=dumped) + # Depending on the pandas version, the class is reported as + # ``pandas.Series`` or ``pandas.core.series.Series``. + assert any(t.endswith(".Series") for t in get_untrusted_types(data=dumped)) def test_loss_get_state_unsupported_reduce(): From 855989a850e34f79a34e10076008081c331bbd72 Mon Sep 17 00:00:00 2001 From: adrinjalali Date: Sun, 27 Sep 2026 08:03:27 +0100 Subject: [PATCH 3/3] unneeded test --- skops/io/tests/test_persist.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/skops/io/tests/test_persist.py b/skops/io/tests/test_persist.py index 47aeae3b..30adf0e8 100644 --- a/skops/io/tests/test_persist.py +++ b/skops/io/tests/test_persist.py @@ -1542,24 +1542,6 @@ def test_reduce_raises_falls_back_to_dict(): assert loaded_obj.x == 3 -def test_object_holding_pandas_can_be_dumped(): - # pandas Series, DataFrame and Index objects hold a ``BlockValuesRefs``, - # whose ``__reduce__`` raises, so any object containing one failed to dump, - # see gh-450. Loading pandas objects is not supported, so only dumping and - # auditing are checked here. The default ``RangeIndex`` does not hold such - # a reference, hence the explicit index. - pd = pytest.importorskip("pandas") - - class Holder: - def __init__(self): - self.series = pd.Series([1, 2, 3], index=["a", "b", "c"]) - - dumped = dumps(Holder()) - # Depending on the pandas version, the class is reported as - # ``pandas.Series`` or ``pandas.core.series.Series``. - assert any(t.endswith(".Series") for t in get_untrusted_types(data=dumped)) - - def test_loss_get_state_unsupported_reduce(): # loss_get_state understands the two shapes of __reduce__ output produced by # scikit-learn's loss classes, and refuses anything else.