Skip to content

Commit aafd9c9

Browse files
committed
🐛 Map: fix gather race for async source tasks
`update_map_task_state` gathered from the gather_item's own clones. For async process-type sources (CalcJob, WorkChain, @task.graph) the awaitable cascade can reach the gather before `continue_workgraph` has scheduled those clones, raising `KeyError` or leaving them PLANNED and hanging the finalize path. `generate_mapped_tasks` no longer clones the gather_item: its executor is a pure pass-through, so the clones only ever held copies. The gather reads each value from the mapped source task's clones instead, which is race-free because source results are recorded before the cascade reaches the zone. A failed iteration produces no result, so the zone now goes FAILED and names the affected map keys, rather than gathering `None` and finishing with a namespace that is silently one entry short. Also drop a stale commented-out gather block from `update_template_task_state`; the aggregation lives in `update_map_task_state`.
1 parent c29e77b commit aafd9c9

3 files changed

Lines changed: 121 additions & 27 deletions

File tree

src/aiida_workgraph/engine/task_manager.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,15 @@ def generate_mapped_tasks(self, zone_task: Task, prefix: str) -> None:
472472
all_links = []
473473
child_tasks = self.get_all_children(zone_task.name)
474474
for child_task in child_tasks:
475+
# The gather_item task is a pure pass-through aggregator
476+
# (executor=return_input); the map zone reads directly from the
477+
# mapped source tasks in `update_map_task_state`, so cloning
478+
# gather_item would just create unused clones. Skipping the
479+
# clone also avoids a race where, for async process-type source
480+
# tasks (CalcJob, WorkChain, @task.graph), the gather_item
481+
# clones stay PLANNED and hang the engine's finalize path.
482+
if self.process.wg.tasks[child_task].identifier == 'workgraph.gather_item':
483+
continue
475484
# since the child task is mapped, it should be skipped
476485
self.state_manager.set_task_runtime_info(child_task, 'state', TaskState.MAPPED)
477486
task = self.copy_task(child_task, prefix)

src/aiida_workgraph/engine/task_state.py

Lines changed: 56 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -297,43 +297,72 @@ def update_map_task_state(self, name: str) -> None:
297297
2) gather the results of all the mapped tasks.
298298
3) update the parent task state.
299299
"""
300+
from aiida_workgraph.utils import get_nested_dict
301+
300302
finished, _ = self.are_childen_finished(name)
301-
if finished:
302-
map_zone = self.process.wg.tasks[name]
303-
# gather the results of all the mapped tasks
304-
gather_task = map_zone.gather_item_task
305-
for input in gather_task.inputs:
306-
if input._name.startswith('_'):
307-
continue
308-
results = {}
309-
link = input._links[0]
310-
for prefix, mapped_task in self.process.wg.tasks[gather_task.name].mapped_tasks.items():
311-
results[prefix] = self.ctx._task_results[mapped_task.name][link.to_socket._name]
312-
self.ctx._task_results[name][link.to_socket._name] = results
313-
self.set_task_runtime_info(name, 'state', TaskState.FINISHED)
314-
# self.update_meta_tasks(name)
315-
self.process.report(f'Task: {name} finished.')
316-
self.update_meta_tasks(name)
303+
if not finished:
304+
return
305+
map_zone = self.process.wg.tasks[name]
306+
# Gather the results of all the mapped tasks.
307+
#
308+
# We aggregate directly from each mapped SOURCE task (the task whose
309+
# output is linked into the template gather_item), not via the
310+
# gather_item itself. The gather_item template is a pure pass-through
311+
# aggregator (executor=return_input) and is intentionally not cloned
312+
# per item in `generate_mapped_tasks`, so there are no gather_item
313+
# clones to read from. Reading directly from the source's
314+
# `_task_results` is also race-free: the source's results are
315+
# populated by `update_task_state` before any cascade can reach here,
316+
# which matters when the source is an async process-type task
317+
# (CalcJob, WorkChain, or a @task.graph sub-workflow).
318+
gather_task = map_zone.gather_item_task
319+
gather_links = [
320+
input_socket._links[0]
321+
for input_socket in gather_task.inputs
322+
if not input_socket._name.startswith('_') and input_socket._links
323+
]
324+
# An iteration that raised is FAILED, and `on_task_failed` SKIPs its
325+
# downstream children, so a broken item surfaces as either state on the
326+
# source clone. Both mean it produced nothing to gather, so fail the
327+
# whole zone instead of emitting a namespace that is silently missing
328+
# that item: a FINISHED zone whose gathered dict is short reads as a
329+
# complete result and quietly corrupts whatever aggregates it.
330+
failed_prefixes = sorted(
331+
prefix
332+
for link in gather_links
333+
for prefix, clone in (self.process.wg.tasks[link.from_task.name].mapped_tasks or {}).items()
334+
if self.get_task_runtime_info(clone.name, 'state') in (TaskState.FAILED, TaskState.SKIPPED)
335+
)
336+
if failed_prefixes:
337+
self.set_task_runtime_info(name, 'state', TaskState.FAILED)
338+
self.process.report(f'Task: {name} failed, no result from mapped item(s): {", ".join(failed_prefixes)}.')
317339
self.update_parent_task_state(name)
340+
return
341+
for link in gather_links:
342+
source_clones = self.process.wg.tasks[link.from_task.name].mapped_tasks or {}
343+
results = {}
344+
for prefix, clone in source_clones.items():
345+
# Defensive only: every surviving clone is FINISHED with its
346+
# result recorded, since the failure paths returned above.
347+
results[prefix] = get_nested_dict(
348+
self.ctx._task_results[clone.name],
349+
link.from_socket._scoped_name,
350+
default=None,
351+
)
352+
self.ctx._task_results[name][link.to_socket._name] = results
353+
self.set_task_runtime_info(name, 'state', TaskState.FINISHED)
354+
self.process.report(f'Task: {name} finished.')
355+
self.update_meta_tasks(name)
356+
self.update_parent_task_state(name)
318357

319358
def update_template_task_state(self, name: str) -> None:
320359
"""Update the template task state.
321360
1) check if all child tasks are finished.
322-
2) gather the results of all the mapped tasks.
323-
3) update the parent task state.
361+
2) update the parent task state.
324362
"""
325363
finished, _ = self.are_childen_finished(name)
326364
if finished:
327-
# # gather the results of all the mapped tasks
328-
# results = {}
329-
# for prefix, mapped_task in self.process.wg.tasks[name].mapped_tasks.items():
330-
# for output in mapped_task.outputs:
331-
# if output._name in self.ctx._task_results[mapped_task.name]:
332-
# results.setdefault(output._name, {})
333-
# results[output._name][prefix] = self.ctx._task_results[mapped_task.name][output._name]
334-
# self.ctx._task_results[name] = results
335365
self.set_task_runtime_info(name, 'state', TaskState.FINISHED)
336-
# self.update_meta_tasks(name)
337366
self.process.report(f'Task: {name} finished.')
338367
self.update_parent_task_state(name)
339368

tests/test_map.py

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,20 @@ def add(x, y):
2222
return x + y
2323

2424

25+
@task.graph
26+
def add_workflow(x, y) -> int:
27+
"""Async process-type source task (runs as its own sub-process) for use inside a Map zone."""
28+
return add(x=x, y=y).result
29+
30+
31+
@task()
32+
def maybe_fail(x, y):
33+
"""Add two numbers, but fail for one specific item to exercise the failure path."""
34+
if x == 1:
35+
raise ValueError('boom on x==1')
36+
return x + y
37+
38+
2539
@task()
2640
def calc_sum(data: Annotated[dict, dynamic(orm.Int)]) -> float:
2741
"""Compute the sum of all provided values."""
@@ -71,3 +85,45 @@ def test_map_value_and_key():
7185
# values 0, 1 -> (0+10)+(1+10) = 21; keys are the user's own source keys
7286
assert total.value == 21
7387
assert joined.value == 'key_0,key_1'
88+
89+
90+
def test_map_zone_async_source():
91+
"""Map over an async process-type source task (`@task.graph`) and gather it.
92+
93+
Regression test for the gather race: when the mapped source is a process-type
94+
task, the awaitable cascade can reach the gather phase before the gather_item
95+
clones are scheduled. Before the fix this raised ``KeyError`` and excepted the
96+
engine; the existing ``test_map_zone`` does not catch it because it maps over
97+
plain synchronous ``@task`` functions that all succeed.
98+
"""
99+
x = 1
100+
n = 3
101+
with WorkGraph('map_async_source') as wg:
102+
data = generate_data(n=n).data
103+
with Map(data) as map_zone:
104+
out1 = add_workflow(x=map_zone.value, y=x).result
105+
map_zone.gather({'sum1': out1})
106+
out3 = calc_sum(data=map_zone.outputs.sum1).result
107+
wg.run()
108+
# values are 0+1, 1+1, 2+1 -> 1 + 2 + 3 = 6
109+
assert out3.value == 6
110+
111+
112+
def test_map_zone_failed_iteration_fails_the_zone():
113+
"""A failed mapped iteration must fail the zone, not except and not under-report.
114+
115+
Before the fix the gather ``KeyError``-ed on the item that produced no
116+
result, excepting the engine and burying the real cause. The zone now goes
117+
FAILED and gathers nothing, rather than reporting FINISHED with a namespace
118+
that is silently missing the failed item.
119+
"""
120+
n = 3
121+
with WorkGraph('map_fail') as wg:
122+
data = generate_data(n=n).data
123+
with Map(data) as map_zone:
124+
out1 = maybe_fail(x=map_zone.value, y=10).result
125+
map_zone.gather({'sum1': out1})
126+
wg.run()
127+
assert map_zone.state == 'FAILED'
128+
assert wg.process.exit_status == 302
129+
assert 'key_1_maybe_fail' in wg.process.exit_message

0 commit comments

Comments
 (0)