Resolve upstream tasks when template field is XComArg #8805
Conversation
turbaszek
force-pushed
the
aip31-set-relations
branch
from
d40c9d9 to
252be10
Compare
airflow/models/baseoperator.py
Outdated
There was a problem hiding this comment.
this traversing part should be very "error prune". wee are starting to traverse all user templated args with no exception (old code is affected), it can be that something will fail and this feature will become a "blocker" to update to the new version
There was a problem hiding this comment.
it can be that something will fail and this feature will become a "blocker" to update to the new version
Can you provide an example? We perform an action only when we encounter XComArg so self.set_upstream should work. If it doesn't work then it means that our implementation is corrupted.
airflow/providers/google/marketing_platform/operators/display_video.py
Outdated
Show resolved
Hide resolved
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
Please add a test that checks the up/down stream dependencies work with Dag Serialization enabled.
turbaszek
force-pushed
the
aip31-set-relations
branch
from
0660ed3 to
4b4af57
Compare
|
@ash @kaxil @evgenyshulman are we still sure that this way it's better than having a metaclass? |
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
templated classes aren't handled yet.
Additionally, https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-31%3A+Airflow+functional+DAG+definition doesn't have this .output attribute.
Where has this concept come from? It wasn't in the AIP.
It was proposed in #8055 . Without it mixing functional and non-functional operators will be hard. |
turbaszek
force-pushed
the
aip31-set-relations
branch
3 times, most recently
from
46eab09 to
6743e4e
Compare
|
@ashb @evgenyshulman @casassg I think all questions were addressed |
There was a problem hiding this comment.
Because:
In [3]: class meta(type):
...: def __call__(cls, *args, **kwargs):
...: return type.__call__(cls, *args, **kwargs)
...:
In [7]: class Klass(metaclass=meta):
...: def __init__(self, a, b, c):
...: pass
...:
In [8]: signature(Klass).parameters
Out[8]: mappingproxy({'args': <Parameter "*args">, 'kwargs': <Parameter "**kwargs">})
In [9]: signature(Klass.__init__).parameters
Out[9]:
mappingproxy({'self': <Parameter "self">,
'a': <Parameter "a">,
'b': <Parameter "b">,
'c': <Parameter "c">})
turbaszek
force-pushed
the
aip31-set-relations
branch
from
3536855 to
745d0a3
Compare
Co-authored-by: Ash Berlin-Taylor <[email protected]>
When we are referring ``{{task}}` in jinja templates we can also
refer some of the fields, which are templated. We are not
able to solve all the problems with such rendering (specifically
recursive rendering of the fields used in JINJA templating might
be problematic. Currently whether you see original, or rendered
field depends solely on the sequence in templated_fields.
However that would not even explain the rendering problem
described in apache#13559 where kwargs were defined after opargs and
the rendering of opargs **should** work. It turned out that
the problem was with a change introduced in apache#8805 which made
the context effectively holds a DIFFERENT task than the current
one. Context held an original task, and the curren task was
actually a locked copy of it (to allow resolving upstream
args before locking). As a result, any changes done by
rendering templates were not visible in the task accessed
via {{ task }} jinja variable.
This change replaces the the task stored in context with the
same copy that is then used later during execution so that
at least the "sequential" rendering works and templated
fields which are 'earlier' in the list of templated fields
can be used (and render correctly) in the following fields.
Fixes: apache#13559
When we are referring ``{{task}}` in jinja templates we can also
refer some of the fields, which are templated. We are not
able to solve all the problems with such rendering (specifically
recursive rendering of the fields used in JINJA templating might
be problematic. Currently whether you see original, or rendered
field depends solely on the sequence in templated_fields.
However that would not even explain the rendering problem
described in #13559 where kwargs were defined after opargs and
the rendering of opargs **should** work. It turned out that
the problem was with a change introduced in #8805 which made
the context effectively holds a DIFFERENT task than the current
one. Context held an original task, and the curren task was
actually a locked copy of it (to allow resolving upstream
args before locking). As a result, any changes done by
rendering templates were not visible in the task accessed
via {{ task }} jinja variable.
This change replaces the the task stored in context with the
same copy that is then used later during execution so that
at least the "sequential" rendering works and templated
fields which are 'earlier' in the list of templated fields
can be used (and render correctly) in the following fields.
Fixes: #13559
4 participants
closes: #8054
Make sure to mark the boxes below before creating PR: [x]
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.