fix task_instance_mutation_hook by waleedsamy · Pull Request #9910 · apache/airflow

waleedsamy · 2020-07-21T09:50:54Z

Fixes #9902.
Importing task_instance_mutation_hook will keep the default impl. To fix this I updated Dagrun model to import settings and then later call settings.task_instance_mutation_hook; The same way used while calling policy function.

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

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.

(cherry picked from commit 021bb88bb82adf54b2299f723db0dc4d88fb6ab9)

Python 2.7 is no longer supported in Pygments 2.6.x

* add git sync sidecars * add a helm test * add more tests * allow users to provide git username and pass via a k8s secrets * set default values for airflow worker repository & tag * change ci timeout * fix link * add credentials_secret to airflow.cfg configmap * set GIT_SYNC_ADD_USER on kubernetes worker pods, set uid * add fsGroup to webserver and kubernete workers * move gitSync to dags.gitSync * rename valueFields * turn off git sync and dag persistence by default * provide option to specify known_hosts * add git-sync details into the chart documentation * Update .gitignore Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> * make git sync max failures configurable * Apply suggestions from code review Co-authored-by: Jarek Potiuk <jarek@potiuk.com> * add back requirements.lock Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> Co-authored-by: Jarek Potiuk <jarek@potiuk.com> (cherry picked from commit d93555b)

…9816) Rather than only allowing specific pre-determined config settings, this change allows the user to place _any_ config setting they like in the generated airflow.cfg, including overwriting the "generated defaults". This providers a nicer interface for the users of the chart (even if the could already set these via the env vars). (cherry picked from commit e4790d5)

boring-cyborg · 2020-07-21T09:50:56Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://apache-airflow-slack.herokuapp.com/

waleedsamy · 2020-07-21T09:52:21Z

cc @milton0825 @ashb

milton0825 · 2020-07-21T17:28:22Z

Looks like static check is failing:

+from airflow import settings
 from airflow.exceptions import AirflowException
 from airflow.models.base import ID_LEN, Base
 from airflow.models.taskinstance import TaskInstance as TI
-from airflow import settings
 from airflow.stats import Stats
 from airflow.ti_deps.dep_context import DepContext
 from airflow.ti_deps.dependencies_states import SCHEDULEABLE_STATES

kaxil · 2020-07-21T17:31:34Z

that is odd, I don't think it makes any difference. The module is always fully imported into the sys.modules mapping

waleedsamy · 2020-07-21T20:10:13Z

@kaxil In dagrun.py, having

# airflow/__init__.py > airflow/models/__init__.py > airflow/models/dagrun.py
from airflow.settings import task_instance_mutation_hook

will load task_instance_mutation_hook with the default implementation inside settings.py which will later updated (when settings.initialize() called, and airflow_local_settings provided). The import of task_instance_mutation_hook will result in caching the old implementation and the change in this PR will drop this caching so calling settings.task_instance_mutation_hook will get you the fresh implementation of task_instance_mutation_hook( after settings.initialize load the airflow_local_settings).

milton0825 · 2020-07-21T20:19:21Z

This is good finding! Do you mind look into the test failures? I assume they are related to the change.

waleedsamy · 2020-07-21T20:25:24Z

@milton0825 I'm checking them now (Static checks is passing now)

kaxil · 2020-07-21T20:25:47Z

@kaxil In dagrun.py, having
# airflow/__init__.py > airflow/models/__init__.py > airflow/models/dagrun.py
from airflow.settings import task_instance_mutation_hook
will load task_instance_mutation_hook with the default implementation inside settings.py which will later updated (when settings.initialize() called, and airflow_local_settings provided). The import of task_instance_mutation_hook will result in caching the old implementation and the change in this PR will drop this caching so calling settings.task_instance_mutation_hook will get you the fresh implementation of task_instance_mutation_hook( after settings.initialize load the airflow_local_settings).

when you run airflow scheduler or airflow webserver it loads from airflow import settings that already initializes the settings

airflow/airflow/cli/commands/scheduler_command.py

Line 24 in 4eddce2

from airflow import settings

airflow/airflow/__init__.py

Line 41 in 4eddce2

settings.initialize()

kaxil

Running any airflow cli command calls

airflow/airflow/__init__.py

Line 41 in 4eddce2

settings.initialize()

so I don't see the change in this PR making it any different.

waleedsamy · 2020-07-21T20:34:47Z

This issue is happening with 1.10.11 - not related to master branch.
Looking into v1.10-stable airflow/__init__.py

from airflow import utils
from airflow import settings
from airflow.configuration import conf
from airflow.models import DAG
from flask_admin import BaseView
from importlib import import_module
from airflow.exceptions import AirflowException

settings.initialize()

it will do a bunch of imports before calling initialize. These import will reach dagrun.py and from airflow.settings import task_instance_mutation_hook will cache task_instance_mutation_hook so even when calling settings.initialize() it won't affect the task_instance_mutation_hook in dagrun.py anymore.

milton0825 · 2020-07-21T20:38:05Z

Should we target the change with v1-10-test branch then?

waleedsamy · 2020-07-21T20:38:57Z

Ok, I'll create a new PR for v1-10-test.

kaxil · 2020-07-21T20:44:13Z

That is a very good point, in that case the correct solution for v1-10-test would be to move the from airflow.models import DAG after settings.initialize or have it similar to master as this will fix other things too (i.e other airflow_local_settings functions that would used old cached functions)

WDYT @waleedsamy @milton0825 ?

milton0825 · 2020-07-21T20:49:34Z

+1

waleedsamy · 2020-07-21T20:53:34Z

@kaxil Ok, I'll move from airflow.models import DAG after settings.initialize.

kaxil · 2020-07-21T20:55:43Z

@kaxil Ok, I'll move from airflow.models import DAG after settings.initialize.

Thanks, really appreciate the fix. Can you also please add a comment in that file on why the import line is below settings.initialize?

Also pylint or isort might complain, so you might have to add some #pylint: disable and/or #noqa after the comments to disable the checks

waleedsamy · 2020-07-21T20:59:14Z

Can you also please add a comment in that file on why the import line is below settings.initialize?

I'll, thanks guys :)

…ation (#9902) Resolves #9902

boring-cyborg · 2020-07-22T14:25:19Z

Awesome work, congrats on your first merged pull request!

Fixes #9902

Fixes apache#9902

potiuk and others added 6 commits July 14, 2020 17:15

Python 3.8.4 release breaks our builds

699382b

Fixes sqlalchemy flask failure with python 3.8.4

f1fba28

(cherry picked from commit 021bb88bb82adf54b2299f723db0dc4d88fb6ab9)

Fix failing docker image builds on python 2.7

8cd08b6

Python 2.7 is no longer supported in Pygments 2.6.x

Bump k8s cache version

8ea13cb

fixup! Python 3.8.4 release breaks our builds

6e18575

kaxil requested changes Jul 21, 2020

View reviewed changes

waleedsamy changed the base branch from master to v1-10-test July 21, 2020 20:43

kaxil approved these changes Jul 21, 2020

View reviewed changes

kaxil added this to the Airflow 1.10.12 milestone Jul 21, 2020

milton0825 approved these changes Jul 21, 2020

View reviewed changes

Delay the import of airflow.models to be after the settings initializ…

9c67554

…ation (#9902) Resolves #9902

potiuk force-pushed the v1-10-test branch from 46d1785 to 0dc20fd Compare July 22, 2020 12:46

kaxil changed the base branch from v1-10-test to v1-10-stable July 22, 2020 14:23

kaxil changed the base branch from v1-10-stable to v1-10-test July 22, 2020 14:24

kaxil merged this pull request into apache:v1-10-test Jul 22, 2020

kaxil pushed a commit that referenced this pull request Jul 22, 2020

Fix task_instance_mutation_hook (#9910)

69cc837

Fixes #9902

potiuk added a commit that referenced this pull request Jul 22, 2020

Fix task_instance_mutation_hook (#9910)

320ccad

Fixes #9902

waleedsamy deleted the fix-task_instance_mutation_hook branch July 22, 2020 15:33

waleedsamy mentioned this pull request Jul 22, 2020

task_instance_mutation_hook not using my custom implementation #9902

Closed

kaxil pushed a commit that referenced this pull request Aug 11, 2020

Fix task_instance_mutation_hook (#9910)

e6b017a

Fixes #9902

cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021

Fix task_instance_mutation_hook (apache#9910)

1e040d9

Fixes apache#9902

Conversation

waleedsamy commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Jul 21, 2020

Uh oh!

waleedsamy commented Jul 21, 2020

Uh oh!

milton0825 commented Jul 21, 2020

Uh oh!

kaxil commented Jul 21, 2020

Uh oh!

waleedsamy commented Jul 21, 2020

Uh oh!

milton0825 commented Jul 21, 2020

Uh oh!

waleedsamy commented Jul 21, 2020

Uh oh!

kaxil commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

waleedsamy commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milton0825 commented Jul 21, 2020

Uh oh!

waleedsamy commented Jul 21, 2020

Uh oh!

kaxil commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milton0825 commented Jul 21, 2020

Uh oh!

waleedsamy commented Jul 21, 2020

Uh oh!

kaxil commented Jul 21, 2020

Uh oh!

waleedsamy commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

waleedsamy commented Jul 21, 2020 •

edited

Loading

kaxil commented Jul 21, 2020 •

edited

Loading

waleedsamy commented Jul 21, 2020 •

edited

Loading

kaxil commented Jul 21, 2020 •

edited

Loading

waleedsamy commented Jul 21, 2020 •

edited

Loading