dabla commented on code in PR #62922:
URL: https://github.com/apache/airflow/pull/62922#discussion_r4268803151
##########
task-sdk/src/airflow/sdk/definitions/mappedoperator.py:
##########
@@ -763,10 +853,15 @@ def _get_unmap_kwargs(self, mapped_kwargs: Mapping[str,
Any], *, strict: bool) -
)
# If params appears in the mapped kwargs, we need to merge it into the
- # partial params, overriding existing keys.
- params = copy.copy(self.params)
- with contextlib.suppress(KeyError):
+ # partial params, overriding existing keys. Setting a value resolves
it into the
+ # existing Param, which a shallow copy shares with this operator, so
the params are
+ # copied deeply then: iterate() unmaps every index in one process, and
one index's
+ # params would otherwise become the next one's.
+ if "params" in mapped_kwargs:
+ params = copy.deepcopy(self.params)
params.update(mapped_kwargs["params"])
+ else:
+ params = copy.copy(self.params)
Review Comment:
Fixed in c620ab4ead: `_get_unmap_kwargs` now always deep-copies `params`, so
each index's operator gets `Param` objects of its own. Two new tests reproduced
both scenarios on the previous head and pass now: an operator setting
`self.params["p"] = "own"` in its `__init__` reads `"own"` in
`context["params"]` (every index read the parent's `1` before, and the iterated
task's params stay as declared), and a write to `self.params` in one index's
`execute` no longer reaches its siblings (they read `changed by 1`, `changed by
2` before).
On processing every index's params: with its own copy, an index whose
operator set a param differs from the iterated task again, so the `!=` shortcut
is correct and I kept it. Measured with CodSpeed simulation over 20 indexed
tasks, two rounds each:
| | sync items | async items |
|---|---:|---:|
| always deep-copy, `!=` shortcut kept | −0.1% | +0.7% (noise) |
| always deep-copy + `process_params` for every index | +1.7% | +2.9% |
The one case the shortcut cannot see is an `__init__` that changes only a
`Param`'s schema and no value, since `ParamsDict.__eq__` compares values; that
seemed too narrow to pay 2–3% per index for, but say if you see it differently.
Drafted-by: Claude Fable 5.1; reviewed by @dabla before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
