Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix accessing rendered task.x attributes from within templates #18516

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Sep 25, 2021

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


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@potiuk
Copy link
Member Author

potiuk commented Sep 25, 2021

@turbaszek -> I am not 100% sure if I have thought about all side effects, but I see no problem with having the locked-down copy of the task in the context rather than the original one (unless there were some reasons why we wanted to keep a different copy in the context - but I believe it was not intentional)

@potiuk potiuk force-pushed the fix-rendering-nested-task-fields branch 2 times, most recently from d44b4df to cec7015 Compare September 25, 2021 16:06
@potiuk
Copy link
Member Author

potiuk commented Sep 25, 2021

Also - with the last change you do not need to pass task to the _execute method.

@potiuk potiuk force-pushed the fix-rendering-nested-task-fields branch from cec7015 to 2101fec Compare September 25, 2021 16:12
@potiuk
Copy link
Member Author

potiuk commented Sep 25, 2021

And the last version removes the need for task variable at all (it was assigned from self.task few lines above 😕

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me! But would be get some other approval. @kaxil @uranusjr ?

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Sep 25, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member Author

potiuk commented Sep 25, 2021

I also tested the task from #13559 with the most recent changes:

image

@potiuk
Copy link
Member Author

potiuk commented Sep 25, 2021

But yeah @kaxil @uranusjr @XD-DENG @ashb - second pair of eyes for this crucial part of Airflow would be useful :)

@potiuk potiuk force-pushed the fix-rendering-nested-task-fields branch from 2101fec to c406b32 Compare September 25, 2021 18:08
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look in detail on Monday

airflow/models/taskinstance.py Show resolved Hide resolved
@potiuk potiuk closed this Sep 25, 2021
@potiuk potiuk reopened this Sep 25, 2021
@potiuk potiuk closed this Sep 26, 2021
@potiuk potiuk reopened this Sep 26, 2021
@potiuk potiuk closed this Sep 26, 2021
@potiuk potiuk reopened this Sep 26, 2021
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
@potiuk potiuk force-pushed the fix-rendering-nested-task-fields branch from c406b32 to edb8d9d Compare September 27, 2021 09:52
@potiuk
Copy link
Member Author

potiuk commented Sep 27, 2021

Rebased to make use of the more stable (hopefully) main with merged flakiness fixes :)

@potiuk
Copy link
Member Author

potiuk commented Sep 27, 2021

kind reminder @ashb :)

@ashb
Copy link
Member

ashb commented Sep 27, 2021

Sorry house full of back to school colds here. Might get to it tomorrow but if @turbaszek is okay don't wait for me

@potiuk
Copy link
Member Author

potiuk commented Sep 27, 2021

Sorry house full of back to school colds here. Might get to it tomorrow but if @turbaszek is okay don't wait for me

Can wait - I think would be good to get it to 2.2 but I think this is still "default" so I am not in a hurry.

@ashb ashb added this to the Airflow 2.2.0 milestone Sep 27, 2021
@ashb ashb changed the title Fix rendering nested task fields Fix accessing rendered task.x attributes from within templates Sep 28, 2021
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup all good!

@potiuk potiuk merged commit 1ac63cd into apache:main Sep 28, 2021
@potiuk potiuk deleted the fix-rendering-nested-task-fields branch September 28, 2021 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested templated variables do not always render
4 participants