Skip to content

Commit a67defe

Browse files
committed
fixup! Resolve live producers by region and read their XCom by attempt
Remove the loaded_as_persistent listener once the lookup test is done. The session fixture yields the thread-local Session that the scoped session hands out to every caller, so event.listen(session, ...) registers on an object that outlives the test and the listener was never removed. Its closure kept every instance loaded by later tests alive in a list. The identity map only holds weak references, and Variable.update writes with a Core upsert that does not expire the Variable it just loaded, while XComModel.set only expires XComModelV2 instances and not XComModel ones. A row loaded earlier in the same test was therefore returned stale ('value1' instead of 'value2') by the next read. test_variable_update and test_xcom_set_again_replace passed alone and failed whenever this test ran first on SQLite. Register a named listener and remove it in a finally block, as the other loaded_as_persistent tests in the stack already do.
1 parent e1731ac commit a67defe

4 files changed

Lines changed: 69 additions & 15 deletions

File tree

‎airflow-core/src/airflow/models/dagrun.py‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1738,8 +1738,8 @@ def _expand_mapped_task_if_needed(ti: TI) -> Iterable[TI] | None:
17381738

17391739
# Check dependencies.
17401740
expansion_happened = False
1741-
# Set of task ids for which was already done _revise_map_indexes_if_mapped
1742-
revised_map_index_task_ids: set[tuple[str, UUID]] = set()
1741+
# (task id, region id) pairs for which _revise_map_indexes_if_mapped was already done
1742+
revised_expansion_keys: set[tuple[str, UUID]] = set()
17431743
for schedulable in itertools.chain(schedulable_tis, additional_tis):
17441744
if TYPE_CHECKING:
17451745
assert isinstance(schedulable.task, Operator)
@@ -1764,18 +1764,18 @@ def _expand_mapped_task_if_needed(ti: TI) -> Iterable[TI] | None:
17641764
# later in this same pass must see the post-expansion count).
17651765
dep_context.invalidate_upstream_task_id_counts()
17661766
if new_tis is None and schedulable.state in SCHEDULEABLE_STATES:
1767-
# It's enough to revise map index once per task id,
1767+
# It's enough to revise map index once per task and region,
17681768
# checking the map index for each mapped task significantly slows down scheduling
17691769
expansion_key = (schedulable.task.task_id, schedulable.region_id)
1770-
if expansion_key not in revised_map_index_task_ids:
1770+
if expansion_key not in revised_expansion_keys:
17711771
revised_tis = self._revise_map_indexes_if_mapped(
17721772
schedulable.task,
17731773
dag_version_id=schedulable.dag_version_id,
17741774
region_id=schedulable.region_id,
17751775
session=session,
17761776
)
17771777
ready_tis.extend(revised_tis)
1778-
revised_map_index_task_ids.add(expansion_key)
1778+
revised_expansion_keys.add(expansion_key)
17791779
if revised_tis:
17801780
# Revising a mapped task can add new instances, growing its instance count
17811781
# the same way expansion does. Drop the upstream-count memo so a downstream

‎airflow-core/src/airflow/models/xcom.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,7 @@ def get_many(
357357
if (
358358
any(value is not None for value in (task_ids, dag_ids, map_indexes))
359359
or include_prior_dates
360+
or try_number is not None
360361
or region_id != SENTINEL_REGION_ID
361362
):
362363
raise ValueError("producer_ids cannot be combined with coordinate filters")

‎airflow-core/tests/unit/models/test_dynamic_region.py‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -251,19 +251,71 @@ def test_loop_iteration_lookup_loads_only_the_selected_iterations_rows(producer_
251251
expected_ids = {ti.id for ti in expected}
252252
session.expunge_all()
253253
loaded = []
254-
event.listen(session, "loaded_as_persistent", lambda _, instance: loaded.append(instance))
255-
selected = resolve_current_producers(
256-
dag_id=mapped.dag_id,
257-
run_id=mapped.run_id,
258-
task_id="mapped",
259-
is_mapped=True,
260-
context=ProducerContext(consumer.region_id, consumer.region_index, "loop"),
261-
session=session,
262-
)
254+
255+
def record_loaded(_, instance):
256+
loaded.append(instance)
257+
258+
event.listen(session, "loaded_as_persistent", record_loaded)
259+
try:
260+
selected = resolve_current_producers(
261+
dag_id=mapped.dag_id,
262+
run_id=mapped.run_id,
263+
task_id="mapped",
264+
is_mapped=True,
265+
context=ProducerContext(consumer.region_id, consumer.region_index, "loop"),
266+
session=session,
267+
)
268+
finally:
269+
event.remove(session, "loaded_as_persistent", record_loaded)
263270
assert {ti.id for ti in selected} == expected_ids
264271
assert {instance.id for instance in loaded if isinstance(instance, TaskInstance)} == expected_ids
265272

266273

274+
def test_loop_iteration_lookup_ignores_another_loops_nested_regions(producer_tis, session):
275+
tis, regions, _ = producer_tis
276+
consumer, mapped = tis["consumer"], tis["mapped"]
277+
other_loop = DynamicRegion(dag_id=mapped.dag_id, run_id=mapped.run_id, node_id="other_loop")
278+
session.add(other_loop)
279+
session.flush()
280+
placed = {}
281+
for owner, parent in (("own", regions[0]), ("other", other_loop)):
282+
nested = DynamicRegion(
283+
dag_id=mapped.dag_id,
284+
run_id=mapped.run_id,
285+
node_id="mapped",
286+
parent_region_id=parent.id,
287+
parent_region_index=consumer.region_index,
288+
)
289+
session.add(nested)
290+
session.flush()
291+
placed[owner] = TaskInstance(
292+
mapped.task, mapped.dag_version_id, run_id=mapped.run_id, map_index=0, region_id=nested.id
293+
)
294+
session.add(placed[owner])
295+
session.flush()
296+
own_id, other_id = placed["own"].id, placed["other"].id
297+
session.expunge_all()
298+
loaded = []
299+
300+
def record_loaded(_, instance):
301+
loaded.append(instance)
302+
303+
event.listen(session, "loaded_as_persistent", record_loaded)
304+
try:
305+
selected = resolve_current_producers(
306+
dag_id=mapped.dag_id,
307+
run_id=mapped.run_id,
308+
task_id="mapped",
309+
is_mapped=True,
310+
context=ProducerContext(consumer.region_id, consumer.region_index, "loop"),
311+
session=session,
312+
)
313+
finally:
314+
event.remove(session, "loaded_as_persistent", record_loaded)
315+
assert [ti.id for ti in selected] == [own_id]
316+
assert other_id not in {instance.id for instance in loaded if isinstance(instance, TaskInstance)}
317+
318+
267319
def test_previous_iteration_zero_is_missing(producer_tis, session):
268320
tis, _, _ = producer_tis
269321
consumer = tis["consumer"]
@@ -404,6 +456,7 @@ def test_get_many_rejects_region_filter_with_prior_dates(regional_tis):
404456
{"region_id": None},
405457
{"region_id": uuid4()},
406458
{"include_prior_dates": True},
459+
{"try_number": 2},
407460
],
408461
)
409462
def test_get_many_rejects_producer_ids_with_coordinate_filters(coordinate_filter):

‎airflow-core/tests/unit/models/test_xcom_arg.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
from airflow.models.xcom_arg import XComArg
2626
from airflow.providers.standard.operators.bash import BashOperator
2727
from airflow.providers.standard.operators.python import PythonOperator
28+
from airflow.sdk import task_group
2829
from airflow.serialization.definitions.mappedoperator import get_mapped_ti_count
2930
from airflow.serialization.definitions.notset import NOTSET
3031
from airflow.serialization.definitions.xcom_arg import (
@@ -420,7 +421,6 @@ def source(value):
420421

421422

422423
def test_member_of_mapped_task_group_has_no_map_length(dag_maker, session):
423-
from airflow.sdk import task_group
424424

425425
with dag_maker(session=session, serialized=True):
426426

0 commit comments

Comments
 (0)