Skip to content

only evaluate items of builtin collections not subclasses - #1612

Merged
rcap107 merged 2 commits into
skrub-data:mainfrom
jeromedockes:no-builtin-collections-subclasses
Sep 16, 2025
Merged

rcap107 merged 2 commits into
skrub-data:mainfrom
jeromedockes:no-builtin-collections-subclasses

Conversation

@jeromedockes

Copy link
Copy Markdown
Member

When evaluating a DataOp, we evaluate built-in collections such as lists by evaluating each item and then reconstructing the list. This allows things like choose_from([choose_from(...), choose_from(...)]) (because we recurse into the list) and many more useful cases. This is similar to what scikit-learn clone does.

However, we should not do it for instances of subclasses of those builtin-types, because their initializers may work differently or they may have other invariants we do not know about so we may not be able to reconstruct the new collection from the evaluated items. sklearn.clone only considers the built-in classes, not their subclasses.

At the moment we also open subclasses, this PR fixes that (ie makes the behavior the same as clone)

eg this fails on main but works in this PR

>>> from collections import namedtuple

>>> import skrub

>>> Pair = namedtuple("Pair", ["first", "second"])
>>> p = Pair(1, 2)
>>> skrub.as_data_op(p).skb.eval()
Pair(first=1, second=2)

(In main we get TypeError: Pair.__new__() missing 1 required positional argument: 'second' when trying to call Pair([1, 2]) )

@jeromedockes jeromedockes added bug Something isn't working data_ops Something related to the skrub DataOps labels Sep 14, 2025
@jeromedockes jeromedockes added this to the 0.6.2 milestone Sep 16, 2025

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks @jeromedockes

I wonder how many people were affected by this, besides you 😝

I noted down this should end up in the documentation somehow, maybe as a possible error

@rcap107
rcap107 merged commit 81507e7 into skrub-data:main Sep 16, 2025
26 checks passed
rcap107 pushed a commit to rcap107/skrub that referenced this pull request Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working data_ops Something related to the skrub DataOps

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants