You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ephraimbuddy (via GitHub)" <gi...@apache.org> on 2023/03/01 10:27:42 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request, #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
ephraimbuddy opened a new pull request, #29832:
URL: https://github.com/apache/airflow/pull/29832
This adds basic implementation of on_failure_fail_dagrun params to teardown tasks
Part of https://github.com/orgs/apache/projects/207?pane=issue&itemId=18760456
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1124058690
##########
airflow/utils/task_group.py:
##########
@@ -245,6 +245,8 @@ def add(self, task: DAGNode) -> None:
elif SetupTeardownContext.is_teardown:
if isinstance(task, AbstractOperator):
setattr(task, "_is_teardown", True)
+ if getattr(SetupTeardownContext, "on_failure_fail_dagrun", False):
+ setattr(task, "_on_failure_fail_dagrun", True)
Review Comment:
If that's the case, should we still have @setup/@teardown decorator because it seems to me that users would now be able to set their tasks as setup/teardown when defining it in the task? e.g:
```python
BashOperator(task_id='mytask', is_setup=True, bash_command="echo 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] norm commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "norm (via GitHub)" <gi...@apache.org>.
norm commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1124355379
##########
airflow/utils/task_group.py:
##########
@@ -245,6 +245,8 @@ def add(self, task: DAGNode) -> None:
elif SetupTeardownContext.is_teardown:
if isinstance(task, AbstractOperator):
setattr(task, "_is_teardown", True)
+ if getattr(SetupTeardownContext, "on_failure_fail_dagrun", False):
+ setattr(task, "_on_failure_fail_dagrun", True)
Review Comment:
The decorators would still be needed for non-classic operators, wouldn't they?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1124232287
##########
airflow/utils/task_group.py:
##########
@@ -245,6 +245,8 @@ def add(self, task: DAGNode) -> None:
elif SetupTeardownContext.is_teardown:
if isinstance(task, AbstractOperator):
setattr(task, "_is_teardown", True)
+ if getattr(SetupTeardownContext, "on_failure_fail_dagrun", False):
+ setattr(task, "_on_failure_fail_dagrun", True)
Review Comment:
We can control this in the init of the BaseOperator but I doubt it's a good user experience
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1124231372
##########
airflow/utils/task_group.py:
##########
@@ -245,6 +245,8 @@ def add(self, task: DAGNode) -> None:
elif SetupTeardownContext.is_teardown:
if isinstance(task, AbstractOperator):
setattr(task, "_is_teardown", True)
+ if getattr(SetupTeardownContext, "on_failure_fail_dagrun", False):
+ setattr(task, "_on_failure_fail_dagrun", True)
Review Comment:
And also all tasks would now have `on_failure_fail_dagrun` making dag authors to use it on normal tasks which are not teardown tasks. cc @norm
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy merged pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy merged PR #29832:
URL: https://github.com/apache/airflow/pull/29832
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] norm commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "norm (via GitHub)" <gi...@apache.org>.
norm commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1124360657
##########
airflow/utils/task_group.py:
##########
@@ -245,6 +245,8 @@ def add(self, task: DAGNode) -> None:
elif SetupTeardownContext.is_teardown:
if isinstance(task, AbstractOperator):
setattr(task, "_is_teardown", True)
+ if getattr(SetupTeardownContext, "on_failure_fail_dagrun", False):
+ setattr(task, "_on_failure_fail_dagrun", True)
Review Comment:
Setting `on_failure_fail_dagrun` as a default in the BaseOperator seems weird. At least to me. Like, it should only apply to a teardown task as that's how it's described. And the default of `False` in this case is opposite to the usual behaviour of an operator of failing the run, isn't it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1123432211
##########
airflow/utils/setup_teardown.py:
##########
@@ -43,15 +43,19 @@ def setup(cls):
@classmethod
@contextmanager
- def teardown(cls):
+ def teardown(cls, on_failure_fail_dagrun=None):
if cls.is_setup or cls.is_teardown:
raise AirflowException(
"A teardown task or taskgroup cannot be nested inside another"
" setup/teardown task or taskgroup"
)
cls.is_teardown = True
+ if on_failure_fail_dagrun:
+ setattr(cls, "on_failure_fail_dagrun", on_failure_fail_dagrun)
Review Comment:
We should probably always have this attr in class as false, and simply toggle it true.
##########
airflow/utils/task_group.py:
##########
@@ -245,6 +245,8 @@ def add(self, task: DAGNode) -> None:
elif SetupTeardownContext.is_teardown:
if isinstance(task, AbstractOperator):
setattr(task, "_is_teardown", True)
+ if getattr(SetupTeardownContext, "on_failure_fail_dagrun", False):
+ setattr(task, "_on_failure_fail_dagrun", True)
Review Comment:
Same here, we shouldn't just graft this in, but add a false default to BaseOperator. Should probably do the same for _is_setup/_is_teardown.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #29832: Add `_on_failure_fail_dagrun` attribute for teardown tasks
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #29832:
URL: https://github.com/apache/airflow/pull/29832#discussion_r1138755790
##########
airflow/utils/setup_teardown.py:
##########
@@ -43,15 +44,17 @@ def setup(cls):
@classmethod
@contextmanager
- def teardown(cls):
+ def teardown(cls, on_failure_fail_dagrun=False):
Review Comment:
```suggestion
def teardown(cls, *, on_failure_fail_dagrun=False):
```
##########
airflow/models/baseoperator.py:
##########
@@ -929,7 +930,8 @@ def as_setup(cls, *args, **kwargs):
def as_teardown(cls, *args, **kwargs):
from airflow.utils.setup_teardown import SetupTeardownContext
- with SetupTeardownContext.teardown():
+ on_failure_fail_dagrun = kwargs.pop("on_failure_fail_dagrun", False)
+ with SetupTeardownContext.teardown(on_failure_fail_dagrun):
Review Comment:
```suggestion
with SetupTeardownContext.teardown(on_failure_fail_dagrun=on_failure_fail_dagrun):
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org