Skip to content

Commit 6f424c8

Browse files
committed
Let an XCom read tell the value it loads which iteration it came from
loop.previous read an iterated terminal's XComIterable from the previous iteration, then patched the pointer from outside so its items were read from that iteration too: an isinstance check in loop.py, and any other read with previous_iteration (a lazy sequence item) kept the bug. The XCom read now passes its previous_iteration flag to _load, which holds it in a context variable while the value is deserialized, and XComIterable.deserialize builds the iterable with it. The flag cannot go through deserialize_value, whose signature XCom backends implement. loop.py no longer needs to know about XComIterable. LazyXComSequence.aget loaded through deserialize_value directly and so skipped a backend's load_value; it now goes through XCom._load.
1 parent 9d794c9 commit 6f424c8

5 files changed

Lines changed: 94 additions & 17 deletions

File tree

‎task-sdk/src/airflow/sdk/bases/xcom.py‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,12 @@ class _OwnTaskInstance(Protocol):
143143
"airflow_current_trigger_ti", default=None
144144
)
145145

146+
# Whether the XCom value being loaded was read from the iteration before the reader's, so a value that points
147+
# at other XComs reads them from that iteration too; see ``XComIterable.deserialize``.
148+
_loading_previous_iteration: contextvars.ContextVar[bool] = contextvars.ContextVar(
149+
"airflow_loading_previous_iteration", default=False
150+
)
151+
146152

147153
class TIKeyProtocol(Protocol):
148154
dag_id: str
@@ -527,7 +533,7 @@ def get_one(
527533
raise TypeError(f"Expected XComResult, received: {type(msg)} {msg}")
528534

529535
if msg.value is not None:
530-
return cls._load(msg)
536+
return cls._load(msg, previous_iteration=_previous_iteration)
531537
log.debug(
532538
"No XCom value found; defaulting to None.",
533539
key=key,
@@ -591,7 +597,7 @@ async def aget_one(
591597
raise TypeError(f"Expected XComResult, received: {type(msg)} {msg}")
592598

593599
if msg.value is not None:
594-
return await asyncio.to_thread(cls._load, msg)
600+
return await asyncio.to_thread(cls._load, msg, previous_iteration=_previous_iteration)
595601
log.debug(
596602
"No XCom value found; defaulting to None.",
597603
key=key,
@@ -743,7 +749,14 @@ def _load_many(cls, values: list[JsonValue]) -> list[Any]:
743749
return [cls._load(_XComValueWrapper(value)) for value in values]
744750

745751
@classmethod
746-
def _load(cls, result: Any) -> Any:
752+
def _load(cls, result: Any, *, previous_iteration: bool = False) -> Any:
753+
if previous_iteration:
754+
# The flag cannot travel through deserialize_value, whose signature backends implement.
755+
token = _loading_previous_iteration.set(True)
756+
try:
757+
return cls._load(result)
758+
finally:
759+
_loading_previous_iteration.reset(token)
747760
# Legacy backends keep receiving the result object deserialize_value has always been given.
748761
if _is_preferred_over(cls, "load_value", "deserialize_value"):
749762
return cls.load_value(result.value)
@@ -828,8 +841,8 @@ class XComIterable(Sequence):
828841
iterating and slicing: N elements are N requests. Reading several in one request needs an
829842
Execution API endpoint that takes a list of keys, which does not exist yet.
830843
831-
Inside a loop, the result an iterated terminal pushed in the previous iteration
832-
(``loop.previous``) has ``previous_iteration`` set, so its elements are read from that
844+
An iterable read from the previous iteration (``loop.previous`` inside a loop) is built with
845+
``previous_iteration`` set (see :meth:`deserialize`), so its elements are read from that
833846
iteration too and not from the current one. "Previous" is relative to whoever reads, so
834847
such an iterable cannot be pushed on: :meth:`serialize` refuses it (see there).
835848
"""
@@ -948,8 +961,13 @@ def serialize(self) -> dict:
948961

949962
@classmethod
950963
def deserialize(cls, data: dict, version: int):
951-
"""Ensure the object is JSON deserializable."""
952-
return cls(**data)
964+
"""
965+
Ensure the object is JSON deserializable.
966+
967+
The iterable is a pointer whose item reads resolve to the reader's iteration, so one loaded by a
968+
read from the previous iteration reads its items from that iteration as well.
969+
"""
970+
return cls(**data, previous_iteration=_loading_previous_iteration.get())
953971

954972

955973
class _AsyncXComIterator:

‎task-sdk/src/airflow/sdk/execution_time/lazy_sequence.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ async def aget(self, index: int) -> T:
141141
raise IndexError(index)
142142
if not isinstance(msg, XComSequenceIndexResult):
143143
raise TypeError(f"Got unexpected response to GetXComSequenceItem: {msg!r}")
144-
return XCom.deserialize_value(_XComWrapper(msg.root))
144+
return XCom._load(_XComWrapper(msg.root), previous_iteration=self._previous_iteration)
145145

146146
def __len__(self) -> int:
147147
if self._len is None:
@@ -224,7 +224,10 @@ def __getitem__(self, key: int | slice) -> T | Sequence[T]:
224224
)
225225
if not isinstance(msg, XComSequenceSliceResult):
226226
raise TypeError(f"Got unexpected response to GetXComSequenceSlice: {msg!r}")
227-
return [XCom._load(_XComWrapper(value)) for value in msg.root]
227+
return [
228+
XCom._load(_XComWrapper(value), previous_iteration=self._previous_iteration)
229+
for value in msg.root
230+
]
228231

229232
if not isinstance(key, int):
230233
if (index := getattr(key, "__index__", None)) is not None:
@@ -246,7 +249,7 @@ def __getitem__(self, key: int | slice) -> T | Sequence[T]:
246249
raise IndexError(key)
247250
if not isinstance(msg, XComSequenceIndexResult):
248251
raise TypeError(f"Got unexpected response to GetXComSequenceItem: {msg!r}")
249-
return XCom._load(_XComWrapper(msg.root))
252+
return XCom._load(_XComWrapper(msg.root), previous_iteration=self._previous_iteration)
250253

251254

252255
def _coerce_slice_index(value: Any) -> int | None:

‎task-sdk/src/airflow/sdk/execution_time/loop.py‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
from airflow.sdk._shared.secrets_masker import redact
2424
from airflow.sdk.api.datamodels._generated import LoopDecisionPayload
2525
from airflow.sdk.bases.decorator import determine_kwargs
26-
from airflow.sdk.bases.xcom import XComIterable
2726
from airflow.sdk.definitions._internal.loop import LOOP_DECISION_REASON_MAX_BYTES
2827
from airflow.sdk.definitions.loop import LoopDecision
2928
from airflow.sdk.definitions.xcom_arg import PlainXComArg
@@ -69,17 +68,13 @@ def _result(self, *, previous_iteration: bool) -> Any:
6968
return LazyXComSequence(
7069
xcom_arg=PlainXComArg(task), ti=self._ti, previous_iteration=previous_iteration
7170
)
72-
result = XCom.get_one(
71+
return XCom.get_one(
7372
key=XCom.XCOM_RETURN_KEY,
7473
dag_id=self._ti.dag_id,
7574
run_id=self._ti.run_id,
7675
task_id=task.task_id,
7776
_previous_iteration=previous_iteration,
7877
)
79-
if previous_iteration and isinstance(result, XComIterable):
80-
# An iterated terminal pushes a pointer to its items: read them in the same iteration.
81-
result.previous_iteration = True
82-
return result
8378

8479

8580
def execute_loop_gate(gate: LoopGateOperator, context: Context) -> None:

‎task-sdk/tests/task_sdk/bases/test_xcom.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,35 @@ def test_an_iterable_read_through_loop_previous_is_refused_when_pushed(self):
337337
with pytest.raises(TypeError, match="read through loop.previous cannot be pushed as is"):
338338
BaseXCom.serialize_value(iterable)
339339

340+
@pytest.mark.parametrize("previous_iteration", [False, True])
341+
@pytest.mark.parametrize("use_async", [False, True])
342+
@pytest.mark.asyncio
343+
async def test_an_iterable_read_from_the_previous_iteration_reads_its_items_from_it(
344+
self, previous_iteration, use_async, mock_supervisor_comms
345+
):
346+
"""The XCom read that loads the pointer tells it which iteration it was read from."""
347+
response = XComResult(
348+
key="return_value", value=BaseXCom.serialize_value(self.make_iterable(length=2))
349+
)
350+
coordinates = dict(key="return_value", dag_id="dag", task_id="task", run_id="run")
351+
352+
if use_async:
353+
mock_supervisor_comms.asend = AsyncMock(return_value=response)
354+
result = await XCom.aget_one(**coordinates, _previous_iteration=previous_iteration)
355+
else:
356+
mock_supervisor_comms.send.return_value = response
357+
result = XCom.get_one(**coordinates, _previous_iteration=previous_iteration)
358+
359+
assert isinstance(result, XComIterable)
360+
assert result.previous_iteration is previous_iteration
361+
# The flag lives for the load only: the next read starts from the current iteration again.
362+
assert (
363+
XComIterable.deserialize(
364+
{"task_id": "t", "dag_id": "d", "run_id": "r"}, version=1
365+
).previous_iteration
366+
is False
367+
)
368+
340369
def test_deserialize_restores_fields(self):
341370
data = {"task_id": "task", "dag_id": "dag", "run_id": "run", "map_index": 2, "length": 5}
342371
iterable = XComIterable.deserialize(data, version=1)

‎task-sdk/tests/task_sdk/execution_time/test_lazy_sequence.py‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
import pytest
2323

2424
import airflow
25-
from airflow.sdk.bases.xcom import BaseXCom
25+
from airflow.sdk.bases.xcom import BaseXCom, XComIterable
2626
from airflow.sdk.exceptions import ErrorType
2727
from airflow.sdk.execution_time.comms import (
2828
ErrorResponse,
@@ -224,6 +224,38 @@ def test_getitem_loads_through_load_value(
224224
assert lazy_sequence[index] == expected
225225

226226

227+
@pytest.mark.asyncio
228+
async def test_aget_loads_through_load_value(monkeypatch, mock_supervisor_comms, lazy_sequence):
229+
mock_supervisor_comms.asend = AsyncMock(return_value=XComSequenceIndexResult(root="ref-4"))
230+
monkeypatch.setattr(airflow.sdk.execution_time.xcom, "XCom", LoadValueXCom)
231+
232+
assert await lazy_sequence.aget(4) == "loaded ref-4"
233+
234+
235+
@pytest.mark.parametrize("previous_iteration", [False, True])
236+
@pytest.mark.parametrize("read", ["index", "slice", "aget"])
237+
@pytest.mark.asyncio
238+
async def test_an_iterable_item_read_from_the_previous_iteration_reads_its_items_from_it(
239+
read, previous_iteration, mock_supervisor_comms, mock_xcom_arg, mock_ti
240+
):
241+
"""A mapped terminal's item can be an iterated result: it reads its items from the iteration it came from."""
242+
stored = BaseXCom.serialize_value(XComIterable(task_id="t", dag_id="dag", run_id="run", length=1))
243+
sequence = LazyXComSequence(mock_xcom_arg, mock_ti, previous_iteration=previous_iteration)
244+
245+
if read == "aget":
246+
mock_supervisor_comms.asend = AsyncMock(return_value=XComSequenceIndexResult(root=stored))
247+
item = await sequence.aget(0)
248+
elif read == "slice":
249+
mock_supervisor_comms.send.return_value = XComSequenceSliceResult(root=[stored])
250+
(item,) = sequence[0:1]
251+
else:
252+
mock_supervisor_comms.send.return_value = XComSequenceIndexResult(root=stored)
253+
item = sequence[0]
254+
255+
assert isinstance(item, XComIterable)
256+
assert item.previous_iteration is previous_iteration
257+
258+
227259
def test_getitem_indexerror(mock_supervisor_comms, lazy_sequence):
228260
mock_supervisor_comms.send.return_value = ErrorResponse(
229261
error=ErrorType.XCOM_NOT_FOUND,

0 commit comments

Comments
 (0)