You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/07/21 09:51:02 UTC

[GitHub] [airflow] waleedsamy opened a new pull request #9910: fix task_instance_mutation_hook

waleedsamy opened a new pull request #9910:
URL: https://github.com/apache/airflow/pull/9910


   Fixes #9902.
   Importing task_instance_mutation_hook will keep the default impl. To fix this I updated Dagrun model to import settings and then latter call `settings.task_instance_mutation_hook`; The [same](https://github.com/apache/airflow/blob/c8c52e69c8d9cc1f26f63d95aecc0a6498d40b6f/airflow/models/dagbag.py#L364) way used while calling `policy` function.
   
   ---
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [ ] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Target Github ISSUE in description if exists
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [ ] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662088004


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy edited a comment on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy edited a comment on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662092184


   This issue is happening with **1.10.11** - not related to master branch.
   Looking into [v1.10-stable](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/__init__.py#L48) airflow/\_\_init\_\_.py
   
   ```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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil edited a comment on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662096504


   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 ?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662096504


   > v1.10-stable
   
   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)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662100716


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662103247


   >  Can you also please add a comment in that file on why the import line is below settings.initialize?
   I'll, thanks guys :)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil edited a comment on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662088192


   > @kaxil In `dagrun.py`, having
   > 
   > ```python
   > # 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()](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/__init__.py#L48) called, and [airflow_local_settings](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/settings.py#L400) 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
   
   https://github.com/apache/airflow/blob/4eddce22a3e0eb605f5661204a005262bbaa54cd/airflow/cli/commands/scheduler_command.py#L24
   https://github.com/apache/airflow/blob/4eddce22a3e0eb605f5661204a005262bbaa54cd/airflow/__init__.py#L41


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662092184


   This issue is happening with **1.10.11** - not related to master branch.
   Looking into [v1.10-stable](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/__init__.py#L48) airflow/\_\_init\_\_.py#L48
   
   ```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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662080850


   @kaxil In `dagrun.py`, having 
   ```python
   # 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()](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/__init__.py#L48) called, and [airflow_local_settings](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/settings.py#L400) 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_).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] milton0825 commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
milton0825 commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-661999260


   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
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662484614


   Awesome work, congrats on your first merged pull request!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-661757558


   cc @milton0825 @ashb 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] milton0825 commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
milton0825 commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662085185


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662094108


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662088192


   > @kaxil In `dagrun.py`, having
   > 
   > ```python
   > # 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()](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/__init__.py#L48) called, and [airflow_local_settings](https://github.com/apache/airflow/blob/d23fa2f8896c5e31745131e6917f1970b4aa590f/airflow/settings.py#L400) 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
   
   https://github.com/apache/airflow/blob/4eddce22a3e0eb605f5661204a005262bbaa54cd/airflow/cli/commands/scheduler_command.py#L24
   https://github.com/apache/airflow/blob/4eddce22a3e0eb605f5661204a005262bbaa54cd/airflow/settings.py


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil edited a comment on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662096504


   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 ?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil merged pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #9910:
URL: https://github.com/apache/airflow/pull/9910


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662001192


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662101614


   > @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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-661756838


   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]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
   - Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
   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/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] milton0825 commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
milton0825 commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662098919


   +1


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] milton0825 commented on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
milton0825 commented on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662093700


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] waleedsamy edited a comment on pull request #9910: fix task_instance_mutation_hook

Posted by GitBox <gi...@apache.org>.
waleedsamy edited a comment on pull request #9910:
URL: https://github.com/apache/airflow/pull/9910#issuecomment-662103247


   >  Can you also please add a comment in that file on why the import line is below settings.initialize?
   
   
   I'll, thanks guys :)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org