muditagrawal2007 commented on PR #74920:
URL: https://github.com/apache/airflow/pull/74920#issuecomment-6238212419
@SameerMesiah97 — circling back to your review specifically since my
previous reply tagged the wrong person. To answer your three review comments
point-by-point:
1. **"Could we also add a mapped-task test and a serialization round-trip
test?"** — done in commit `5bbd9a4`:
-
`task-sdk/tests/task_sdk/bases/test_operator.py::test_template_fields_extra_propagates_through_mapped_operator`
— `.partial().expand()` path, asserts both the merged `template_fields` and
that `unmap(...).render_template_fields(...)` substitutes Jinja in the extras.
-
`airflow-core/tests/unit/serialization/test_dag_serialization.py::test_serialize_template_fields_extra_roundtrip`
— `SerializedBaseOperator` round-trip.
-
`airflow-core/tests/unit/serialization/test_dag_serialization.py::test_serialize_template_fields_extra_roundtrip_mapped`
— `SerializedMappedOperator` round-trip.
2. **"What about ordering and deduplication?"** — `OperatorPartial.__call__`
and `DecoratedMappedOperator.__init__` use `dict.fromkeys((*template_fields,
*template_fields_extra))`, which preserves first-seen order and removes
duplicates. So `template_fields=("a","b")` plus
`template_fields_extra=frozenset({"b","c"})` yields `["a","b","c"]`. Happy to
add a dedicated `test_*_preserves_order_and_deduplicates` if the existing
`test_template_fields_extra_class_level_merges_into_template_fields` doesn't
satisfy you; my current assertions do cover the dedup case via `sorted(...) ==
["arg1","arg2","arg3"]`.
3. **"Does this not need an `__init__`? Also, since this helper class is
being used multiple times, you could make it module level and reuse it in the
tests that require it."** — yes, every test helper now declares an `__init__`
matching its template fields (otherwise `BaseOperatorMeta._apply_defaults`
rejects the constructor kwargs as `TypeError: Invalid arguments were passed
…`). I kept them inline because each helper pins a slightly different
`template_fields` / `template_fields_extra` combination; happy to lift to a
module-level factory parametrized on those two if you'd prefer.
The commit also fixed three real bugs in the original code path that the
tests surfaced: `frozenset` serialization in `BaseSerialization.serialize`, a
missing `template_fields_extra` kwarg in `DecoratedMappedOperator`, and a
missing field on `SerializedMappedOperator`. Without those, the PR
description's claim that mapped and serialized tasks round-trip the override
was not actually true. All updated tests pass; full diff and reply are in the
previous comment.
--
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]
