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

Lazy Jinja2 context #20217

Merged
merged 7 commits into from
Dec 14, 2021
Merged

Lazy Jinja2 context #20217

merged 7 commits into from
Dec 14, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 11, 2021

Fix #20213. To work around Jinja2 eagerly resolving context into a new dict, we implement a custom copy interface, and use lower level Jinja2 functions to render templates (because the higher level Template.render() simply calls dict(**data) which is not overridable).

Tests are added to ensure that

  1. Rendering a template with deprecated variable emits a deprecation warning.
  2. Rendering a template without deprecated variable does not emit a deprecation warning.

@uranusjr uranusjr force-pushed the lazy-jinja2-context branch 2 times, most recently from f594e5e to 999f2df Compare December 11, 2021 06:54
@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.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 11, 2021
@uranusjr uranusjr force-pushed the lazy-jinja2-context branch from 999f2df to 17a9ad3 Compare December 11, 2021 17:47
@uranusjr
Copy link
Member Author

uranusjr commented Dec 11, 2021

Don’t merge this yet, there’s another case we need to fix (see comment in linked issue). I’m also thinking I should enable this deprecation-as-error globally in case there are more things we have not catch.

@uranusjr uranusjr force-pushed the lazy-jinja2-context branch from 3a98fd0 to e4fc2f0 Compare December 13, 2021 07:50
@uranusjr
Copy link
Member Author

Alright I’m done.

@jedcunningham
Copy link
Member

Hmm, I'm not seeing a deprecation warning with this:

from datetime import datetime

from airflow import DAG
from airflow.operators.bash import BashOperator

with DAG(
    dag_id="no-context", schedule_interval=None, start_date=datetime(2021, 12, 10)
) as dag:
    BashOperator(task_id="sleep", bash_command="echo 'Hello! {{ execution_date }}'")

@jedcunningham jedcunningham added this to the Airflow 2.2.3 milestone Dec 13, 2021
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Dec 13, 2021
@uranusjr uranusjr force-pushed the lazy-jinja2-context branch from 9effcce to 1f11018 Compare December 14, 2021 04:30
@uranusjr uranusjr force-pushed the lazy-jinja2-context branch from 1f11018 to b9295c0 Compare December 14, 2021 05:51
@uranusjr
Copy link
Member Author

tests/executors/test_dask_executor.py::TestDaskExecutor::test_dask_executor_functions:
ValueError: The futures should have finished; there is probably an error communicating with the Dask cluster.

Only one job failing with this, likely a network issue.

@uranusjr uranusjr merged commit 181d60c into apache:main Dec 14, 2021
@uranusjr uranusjr deleted the lazy-jinja2-context branch December 14, 2021 07:28
jedcunningham pushed a commit that referenced this pull request Dec 14, 2021
Co-authored-by: Jed Cunningham <[email protected]>
(cherry picked from commit 181d60c)
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 type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context deprecation warnings when they aren't used
3 participants