Support tuple unpacking of DataOps - #2243
Conversation
0f0726b to
bca458a
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for tuple unpacking of DataOp objects (e.g. a, b, c = data_op) by inspecting the caller’s bytecode to detect UNPACK_SEQUENCE, building an AsTuple node plus one GetItem node per target, and preserving the previous “no eager iteration” behavior for other iteration forms.
Changes:
- Implement bytecode-based unpack arity detection and
DataOp.__iter__support forUNPACK_SEQUENCE. - Add an
AsTuplenode +unpack()constructor to support unpacking any iterable and to validate expected length. - Add tests + documentation/changelog updates describing supported/unsupported unpacking cases and error behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| skrub/_data_ops/_data_ops.py | Implements _unpack_arity(), updates DataOp.__iter__, and introduces AsTuple/unpack() to enable assignment unpacking. |
| skrub/_data_ops/tests/test_data_ops.py | Adds coverage for basic, iterable, and nested unpacking behavior. |
| skrub/_data_ops/tests/test_errors.py | Adds coverage for unsupported starred unpacking, bytecode-inspection failure fallbacks, and wrong-target-count errors. |
| doc/modules/data_ops/basics/control_flow.rst | Updates docs to show supported unpacking and documents bytecode-based limitations + fallback indexing. |
| CHANGES.rst | Adds a release-note entry announcing tuple unpacking support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bca458a to
157b251
Compare
`a, b, c = data_op` now creates a node per target instead of raising an error, so a deferred function returning several values can be unpacked as usual. Python calls `iter()` on the right-hand side from an `UNPACK_SEQUENCE` instruction whose argument is the number of targets, so `DataOp.__iter__` reads that count off the calling frame's bytecode and creates an `AsTuple` node (which converts the result to a tuple and checks its length) plus one `GetItem` per target. A wrong number of targets is reported eagerly when a preview value is available, and at evaluation time otherwise. Only `UNPACK_SEQUENCE` is recognised, so anything else (`for` loops, `list(data_op)`, `f(*data_op)`, `a, *rest = data_op`) keeps raising the previous TypeError, as does any case where the bytecode cannot be inspected. Indexing into the result stays available as a portable alternative.
157b251 to
904ef77
Compare
|
thanks a lot for opening this PR, @e-strauss !! for context, the original discussion where @e-strauss proposed this approach happened on the skrub discord: https://discord.com/channels/1220094555282477159/1532375520405229649 |
jeromedockes
left a comment
There was a problem hiding this comment.
looks great apart from some nitpicks! this is an issue that had come up a couple of times so it is a really neat improvement, thanks a lot @e-strauss ! :)
|
|
||
| Read from the caller's ``UNPACK_SEQUENCE`` instruction, or None if not found. | ||
| """ | ||
| if (getframe := getattr(sys, "_getframe", None)) is None: |
There was a problem hiding this comment.
I guess this could be thrown in the same "best effort / catch-all" bag as the rest but don't have a strong preference either way
There was a problem hiding this comment.
ye, I agree, this will make the code a bit more readable
| frame = getframe(2) | ||
| for instruction in dis.get_instructions(frame.f_code): | ||
| if instruction.offset == frame.f_lasti: | ||
| if instruction.opname == "UNPACK_SEQUENCE": |
There was a problem hiding this comment.
could you add a short comment saying that we intentionally don't handle UNPACK_EX? thanks!
|
|
||
| def __repr__(self): | ||
| return f"<{self.__class__.__name__} {short_repr(self.iterable)}>" |
There was a problem hiding this comment.
| def __repr__(self): | |
| return f"<{self.__class__.__name__} {short_repr(self.iterable)}>" | |
| def __repr__(self): | |
| return f"<{self.__class__.__name__}: {self.expected_length} items>" |
I know I suggested the current version but on second thought showing the repr of the parent is a bit redundant and not really consistent with most other dataops, showing the number of items is more informative and consistent with eg skrub.concat . (we know that expected_length is an actual number because AsTuple is only created by iter, though even if it wasn't the consequence would not be dramatic)
There was a problem hiding this comment.
yes, makes sense! had a similar thought when I saw the printed DAG
| # the DataOp is evaluated. | ||
| if (arity := _unpack_arity()) is not None: | ||
| values = unpack(self, arity) | ||
| return iter([values[i] for i in range(arity)]) |
There was a problem hiding this comment.
(nitpick) out of curiosity why do you prefer iter([...]) rather than returning a generator expression here?
| - It is now possible to unpack a :class:`DataOp` that evaluates to several | ||
| values, for example ``train, test = data_op``. Each target becomes a DataOp | ||
| that extracts one of the values. | ||
| :pr:`2243` by :user:`Elias Strauss <e-strauss>`. |
There was a problem hiding this comment.
| - It is now possible to unpack a :class:`DataOp` that evaluates to several | |
| values, for example ``train, test = data_op``. Each target becomes a DataOp | |
| that extracts one of the values. | |
| :pr:`2243` by :user:`Elias Strauss <e-strauss>`. | |
| - It is now possible to unpack a :class:`DataOp` that evaluates to an iterable | |
| (of known size), for example ``first, second = data_op``. Each target becomes | |
| a DataOp that extracts one of the items. | |
| :pr:`2243` by :user:`Elias Strauss <e-strauss>`. |
a, b, c = data_opnow creates a node per target instead of raising an error, so a deferred function returning several values can be unpacked as usual.Python calls
iter()on the right-hand side from anUNPACK_SEQUENCEinstruction whose argument is the number of targets, soDataOp.__iter__reads that count off the calling frame's bytecode and creates anAsTuplenode (which converts the result to a tuple and checks its length) plus oneGetItemper target. A wrong number of targets is reported eagerly when a preview value is available, and at evaluation time otherwise.Only
UNPACK_SEQUENCEis recognised, so anything else (forloops,list(data_op),f(*data_op),a, *rest = data_op) keeps raising the previous TypeError, as does any case where the bytecode cannot be inspected. Indexing into the result stays available as a portable alternative.Example: