Skip to content

ENH support subclasses of list, set, dict, object for circular pointers - #554

Merged
adrinjalali merged 3 commits into
skops-dev:mainfrom
adrinjalali:gh553-container-subclasses
Oct 2, 2026
Merged

adrinjalali merged 3 commits into
skops-dev:mainfrom
adrinjalali:gh553-container-subclasses

Conversation

@adrinjalali

Copy link
Copy Markdown
Member

Follow up from #549 and fixes #553

This was flagged while working on #549 but kept it separate for an easier review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Empty custom state, slotted subclasses, and circular defaultdict instances remain incorrectly handled.

Review effort: Balanced
Findings: 3 Medium severity · 1 Low severity

Open (4)
What changed in this PR

Adds circular-reference and attribute persistence for built-in container subclasses while retaining protocol 0–2 compatibility.

Changes:

  • Constructs container subclasses with __new__, fills them in place, and restores state.
  • Adds legacy container-node loaders and compatibility tests.
  • Expands round-trip and circular-reference coverage.
File Description
skops/​io/​_general.py Implements container state persistence and reconstruction.
skops/​io/​_utils.py Expands circular-reference support detection.
skops/​io/​old/​_general_v2.py Adds legacy container loaders.
skops/​io/​tests/​test_persist.py Tests subclass state and circular references.
skops/​io/​tests/​test_persist_old.py Tests legacy protocol behavior.
docs/​changes.rst Documents the persistence changes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread skops/io/_general.py
Comment thread skops/io/_general.py
Comment thread skops/io/_utils.py Outdated
if type(value) in (list, set):
return True
return state["__loader__"] in ("DictNode", "ObjectNode")
return state["__loader__"] in ("DictNode", "ListNode", "SetNode", "ObjectNode")
Comment thread docs/changes.rst Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Defaultdict subclasses remain downcast, and overridden set mutators can break loading.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (3)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Preserve defaultdict subclasses and instance state during loading

skops/​io/​_general.py:208

singledispatch routes subclasses of collections.defaultdict to defaultdict_get_state, but this loader always creates the built-in defaultdict. Such subclasses are therefore silently downcast and their instance attributes are discarded, despite this PR's support for dict subclasses. Preserve the serialized class and state here (with an old-protocol reader for the prior schema), or explicitly exclude these subclasses from the stated support.

Medium severity Use the built-in set mutator when loading set items

skops/​io/​_general.py:304

Calling the virtual instance.update can execute an override on an object whose __init__ was deliberately skipped. A valid set subclass whose overridden update depends on initialized state—or simply rejects direct calls—will fail to load, whereas pickle bypasses that override for set items. Invoke the built-in mutator directly so filling cannot depend on subclass initialization.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation is coherent, backward-compatible, and thoroughly covered by targeted tests.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

@adrinjalali
adrinjalali merged commit 639b9a1 into skops-dev:main Oct 2, 2026
30 checks passed
@adrinjalali
adrinjalali deleted the gh553-container-subclasses branch October 2, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make container subclasses saved and loaded like pickle

2 participants