muditagrawal2007 opened a new issue, #74991:
URL: https://github.com/apache/airflow/issues/74991
### Summary
`BaseOperator.template_fields_extra` is typed `ClassVar[frozenset[str]]`.
The PR that introduces it (apache/airflow#74920) iterates that value in three
places to build the merged `template_fields` tuple and the
`MappedOperator.template_fields_extra` value. **`frozenset` iteration order is
not stable across Python processes** — string hashing in CPython 3.3+ is
randomized per process via `PYTHONHASHSEED`. The result is a different
`template_fields_extra` / merged `template_fields` order in the Dag processor,
scheduler, worker, and tests, and a different serialized value in the metadata
DB on every parse.
### Reproduce
```text
$ for i in 1 2 3 4 5; do python3 -c
"print(tuple(frozenset({'alpha','beta','gamma'})))"; done
('gamma', 'beta', 'alpha')
('beta', 'alpha', 'gamma')
('alpha', 'gamma', 'beta')
('gamma', 'alpha', 'beta')
('beta', 'gamma', 'alpha')
```
### Where it bites in the existing PR
- `task-sdk/src/airflow/sdk/bases/operator.py:1269-1271` —
`BaseOperator.__init__` does `*type(self).template_fields_extra` (a
`frozenset`); the merged `template_fields` order depends on the process.
- `task-sdk/src/airflow/sdk/definitions/mappedoperator.py:254-262` —
`OperatorPartial.__call__` does
`tuple(dict.fromkeys((*self.operator_class.template_fields,
*self.operator_class.template_fields_extra)))` and
`template_fields_extra=tuple(self.operator_class.template_fields_extra)`. The
order flows into `MappedOperator.template_fields` and `template_fields_extra`.
- `task-sdk/src/airflow/sdk/bases/decorator.py:723-732` — same shape on the
`@task` decorator path.
- `airflow-core/src/airflow/serialization/serialized_objects.py:1373` — the
resulting tuple is written into `SerializedBaseOperator.template_fields_extra`
and persisted.
- `airflow-core/src/airflow/serialization/dag_version_diff.py:268` —
`template_fields_extra` is one of the fields the diff uses to compute the next
serialized Dag version, so order drift between parses triggers spurious
Dag-version churn.
### Concrete test failures
-
`task-sdk/tests/task_sdk/bases/test_operator.py::TestTemplateFieldsExtra::test_template_fields_extra_preserves_order_and_deduplicates`
asserts a deterministic order from `frozenset({"arg2", "arg3"})`; that
assertion only holds in processes whose `PYTHONHASHSEED` happens to iterate the
set in that order. Running under a different seed flips the list to `["arg1",
"arg3", "arg2"]` and the test fails.
-
`test_template_fields_extra_preserves_order_and_deduplicates_through_mapped_operator`
asserts the same property through `.partial().expand()` and is similarly
seed-dependent.
- The serialization tests in
`airflow-core/tests/unit/serialization/test_dag_serialization.py` already work
around the instability: `test_serialize_template_fields_extra_roundtrip` (line
2096) and `test_serialize_template_fields_extra_roundtrip_mapped` use
`sorted()` / `set()` on the result. The author of the PR felt the instability
and patched the test instead of fixing the data type.
### Why this contradicts the new docstring
`task-sdk/src/airflow/sdk/bases/operator.py:954-959`:
> Additional fields, declared by subclasses, that should be rendered with
Jinja and treated as `XComArg` capable in addition to `template_fields`. Use
this for string fields that are intentionally not promoted into
`template_fields` but still need to receive XCom references (such as
`some_task.output`) from upstream tasks.
The docstring promises a deterministic, declared order; `frozenset[str]`
cannot deliver either. The merge in `__init__` is described as
`dict.fromkeys((*self.template_fields, *extra))` "preserving order,
deduplicating" — but `*extra` over a `frozenset` does not preserve any
user-declared order at all.
### Impact
- Spurious Dag-version diffs on every reparse when the seed rolls
differently between processes. This is the same class of bug as the
recently-closed apache/airflow#53957 ("`@task.kubernetes` decorator causes
different dag hash on every serialized dag update").
- `MappedOperator.template_fields_extra` and the merged `template_fields`
are written to the metadata DB; reading them back across processes yields a
tuple that may not match the in-memory object.
- The order of fields in the rendered-template UI table for any operator
that opts in is process-dependent.
### Suggested fix
1. Change `BaseOperator.template_fields_extra` to `ClassVar[Sequence[str]] =
()`. Accept `tuple`, `list`, or `frozenset`; document the supported types and
the fact that declaration order is what is preserved.
2. Stop materialising `tuple(self.operator_class.template_fields_extra)`;
instead, cast / coerce to `tuple` while preserving the source order. Same
change in `OperatorPartial.__call__`, `_TaskDecorator`, and the
`BaseOperator.__init__` merge.
3. Tighten `MappedOperator.template_fields_extra` and
`SerializedBaseOperator.template_fields_extra` to `Sequence[str]` so the
contract is explicit in the type.
4. Update the order/dedup tests so they feed an ordered `tuple` and assert
that order verbatim. The `frozenset` cases should be either dropped or
rewritten to assert only the set, not the order.
5. Add a regression test that re-runs the order-preservation assertion in a
subprocess with `PYTHONHASHSEED=random` so a future change cannot silently make
the order seed-dependent again.
### Environment
- Reproduced on macOS, CPython 3.14.7.
- Discovered while reviewing apache/airflow#74920.
--
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]
