[Serialization] Fix: Serialize DagParam like a normal Param. - #68901
[Serialization] Fix: Serialize DagParam like a normal Param.#68901Desdroid wants to merge 1 commit into
Conversation
|
@ephraimbuddy @wjddn279 Since you were working on serialization in #63871 - what do you think of this fix I propose here? Does this make sense or is it wrong and some change is needed in |
| elif isinstance(var, DagParam): | ||
| return cls._encode( | ||
| cls._serialize_param(Param(default=var._default, source="dag")), type_=DAT.PARAM | ||
| ) |
There was a problem hiding this comment.
Have you tested this? Looking at the code, it seems only _default is stored and _name is dropped, and since DagParam resolves at runtime via dag_run.conf[self._name], I think this could be a problem. Or am I misreading the flow?
There was a problem hiding this comment.
You're right, I don't think that's the correct thing to do. In op_args the serialized representation is used after DagParam went through serialize_template_field. For the partial_kwargs this path is not taken and what I did here will not work properly on runtime, i got confused while debugging in the heat.
I don't really know how a fix for this should look like - I think either serialize_mapped_operator needs to be adapted and possibly should use serialize_template_field for the partial_kwargs ops or an own Serialization class is needed for DagParam possibly similar to XComArg?
If somebody is willing to point me in a direction, I am also open to contribute a fix.
I'm closing this PR and will open a bug report instead.
We found a workaround using Jinja Templates for DagParams in partial for now.
add.partial(value="{{ params.p }}").expand(value=[1, 2, 3])
Sorry for the confusion.
|
Converted to Issue #68941 |
When we use a DagParam as a kwarg to
partial()of a dynamically mapped task, the DagParam will be serialized with its memory address. This leads to a DAG version inflation, as the memory address is not stable.This is a simple repro:
This PR fixes this issue by serializing the DagParam as a normal Param. This behavior matches the serialization that using the
DagParamas a normal arg does (like in thedotask - it ends up being serialized as aParam).Note: I am not 100% sure that this is the correct fix as I'm not super deep in the DAG serialization code, but it seems to work.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.