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 2021/05/14 16:06:04 UTC
[GitHub] [airflow] Junnplus opened a new pull request #15851: Fix import order for task_instance_mutation_hook
Junnplus opened a new pull request #15851:
URL: https://github.com/apache/airflow/pull/15851
Fixed https://github.com/apache/airflow/issues/15698
import from `airflow.models.dagrun` module may cause `task_instance_mutation_hook` to be overwritten.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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).
--
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] ashb commented on a change in pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#discussion_r632659075
##########
File path: airflow/models/dagrun.py
##########
@@ -41,7 +41,6 @@
from airflow.exceptions import AirflowException, TaskNotFound
from airflow.models.base import ID_LEN, Base
from airflow.models.taskinstance import TaskInstance as TI
Review comment:
The fix is to quite literally delay the import, to avoid an import cycle.
At the point this _was_ imported, the local settings hadn't been applied, so this was always going to be a no-op.
--
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] Junnplus edited a comment on pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
Junnplus edited a comment on pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#issuecomment-841574877
> Can you rebase on latest master please?
Done.
--
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] ashb commented on a change in pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#discussion_r632659075
##########
File path: airflow/models/dagrun.py
##########
@@ -41,7 +41,6 @@
from airflow.exceptions import AirflowException, TaskNotFound
from airflow.models.base import ID_LEN, Base
from airflow.models.taskinstance import TaskInstance as TI
Review comment:
The fix is to quite literally delay the import, to avoid an import cycle causing confusion.
--
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 #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#issuecomment-841574276
Can you rebase on latest master please?
--
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] ashb merged pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15851:
URL: https://github.com/apache/airflow/pull/15851
--
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] ashb commented on a change in pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#discussion_r632659744
##########
File path: airflow/models/dagrun.py
##########
@@ -626,6 +625,7 @@ def _emit_duration_stats_for_finished_state(self):
@provide_session
def verify_integrity(self, session: Session = None):
+ from airflow.settings import task_instance_mutation_hook
Review comment:
> Avoid import in function definitions. https://docs.python.org/3.1/howto/doanddont.html#from-module-import-name1-name2
That is talking about `import *` inside functions. This is not what we are doing here.
--
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] mrbaguvix commented on a change in pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
mrbaguvix commented on a change in pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#discussion_r632654957
##########
File path: airflow/models/dagrun.py
##########
@@ -626,6 +625,7 @@ def _emit_duration_stats_for_finished_state(self):
@provide_session
def verify_integrity(self, session: Session = None):
+ from airflow.settings import task_instance_mutation_hook
Review comment:
Avoid import in function definitions. https://docs.python.org/3.1/howto/doanddont.html#from-module-import-name1-name2
```suggestion
```
##########
File path: airflow/models/dagrun.py
##########
@@ -41,7 +41,6 @@
from airflow.exceptions import AirflowException, TaskNotFound
from airflow.models.base import ID_LEN, Base
from airflow.models.taskinstance import TaskInstance as TI
Review comment:
```suggestion
from airflow.models.taskinstance import TaskInstance as TI
from airflow.settings import task_instance_mutation_hook
```
##########
File path: airflow/models/dagrun.py
##########
@@ -41,7 +41,6 @@
from airflow.exceptions import AirflowException, TaskNotFound
from airflow.models.base import ID_LEN, Base
from airflow.models.taskinstance import TaskInstance as TI
-from airflow.settings import task_instance_mutation_hook
Review comment:
Why remove it from here?
--
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] Junnplus commented on pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#issuecomment-841574877
> Can you rebase on latest master please?
done.
--
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] ashb commented on pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#issuecomment-841627304
@jhtimmins I'm just about to merge this h we need to make sure we include it in 2.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] github-actions[bot] commented on pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#issuecomment-841371557
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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 a change in pull request #15851: Fix import order for task_instance_mutation_hook
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15851:
URL: https://github.com/apache/airflow/pull/15851#discussion_r632658067
##########
File path: airflow/models/dagrun.py
##########
@@ -626,6 +625,7 @@ def _emit_duration_stats_for_finished_state(self):
@provide_session
def verify_integrity(self, session: Session = None):
+ from airflow.settings import task_instance_mutation_hook
Review comment:
Let's change L642 to `settings.task_instance_mutation_hook(ti)` in that case
--
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