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/01/08 01:40:31 UTC
[GitHub] [airflow] TV4Fun opened a new pull request #7098: [AIRFLOW-4453]
Make behavior of `none_failed` consistent with documentation
TV4Fun opened a new pull request #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098
The documentation for the `none_failed` trigger rule
(https://airflow.apache.org/docs/stable/concepts.html#trigger-rules)
describes its behavior as "all parents have not failed (`failed` or
`upstream_failed`) i.e. all parents have succeeded or been skipped."
With that definition in mind, there is no reason that the check for
`none_failed` should ever have to check for skipped upstream tasks or
set the current task to `skipped`. The current behavior causes the rule
to skip if all upstream tasks have skipped, which is more than a little
confusing. This fixes the behavior to be consistent with the documentation.
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
- [X] Description above provides context of the change
- [X] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
- [X] Unit tests coverage for changes (not needed for documentation changes)
- [X] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [X] 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).
<sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
---
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-571854759
Yes, but not right now. Might be a little while before I have time.
On Tue, Jan 7, 2020, 5:45 PM Kamil Breguła <no...@github.com> wrote:
> Can you add tests to avoid regression?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/7098?email_source=notifications&email_token=ABD4ECGKFNQ3RMCRDWPCUVLQ4UV4LA5CNFSM4KEBP7VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIK45KI#issuecomment-571854505>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABD4ECFYBSG45WUQHOTOXI3Q4UV4LANCNFSM4KEBP7VA>
> .
>
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] jared-martin commented on issue #7098: [AIRFLOW-4453]
Make behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
jared-martin commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-582060266
@yuqian90 I agree with what @ChrisEverling said [here](https://github.com/apache/airflow/pull/4182#discussion_r362862918), and with the folks who created the issue, "none failed" includes "all skipped", and the documentation backs that up. It took some debugging to find this extra condition, and I still don't understand the rationale behind it.
Why not make the current trigger rule behave as documented, and add a new one to preserve the unexpected "none failed and not all skipped" behavior?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] yuqian90 commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
yuqian90 commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-587231424
Hi, @TV4Fun would you like to continue working on this PR? This is definitely a good change to make. The issue regarding nested `BranchPythonOperator` can be addressed with something like the `create_join()` in my previous comment. Please consider it. Thanks!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] yuqian90 commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
yuqian90 commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-580306040
@TV4Fun, the Airflow doc and example docs suggest none_failed is the trigger_rule to use for the join DummyOperator task in DAGs with branching. (See branch_with_trigger.py here
https://airflow.apache.org/docs/stable/concepts.html)
one_success won't work because the join task won't wait for all the branches to finish.
There are various real uses. Anything that requires branching and then run subsequent downstream tasks needs the join task. E.g a DAG that does task A on weekdays but task B and C on weekends in parallel and always run task D after either A or B+C is finished. More complicated examples exist if such branching are nested. After this PR, problems happen if the branches are nested like the previous example I gave.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-571862145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=h1) Report
> Merging [#7098](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/fe20ef6147f157b7c963ad46e13aff7c16458632?src=pr&el=desc) will **decrease** coverage by `0.27%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7098 +/- ##
==========================================
- Coverage 85.15% 84.87% -0.28%
==========================================
Files 680 680
Lines 38824 38822 -2
==========================================
- Hits 33061 32951 -110
- Misses 5763 5871 +108
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/ti\_deps/deps/trigger\_rule\_dep.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcHMvdHJpZ2dlcl9ydWxlX2RlcC5weQ==) | `93.24% <ø> (+2.45%)` | :arrow_up: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
| [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
| [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
| [...rflow/contrib/operators/kubernetes\_pod\_operator.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZF9vcGVyYXRvci5weQ==) | `78.75% <0%> (-20%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=footer). Last update [fe20ef6...19f9fe8](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] yuqian90 commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
yuqian90 commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-581009288
@TV4Fun this sounds like a good suggestion. How about calling the new trigger rule `all_success_or_skip` ? That makes it easier to comprehend.
> 1. Keep the current behavior of `none_failed` and create a new trigger rule (`none_failed_including_skips` maybe? I don't know) that behaves as in this PR. I saw keep the current behavior for `none_failed` so as to minimize the amount of refactoring we make people do, even though from a documentation perspective, it would make more sense to change the behavior of `none_failed` and move the current behavior to a new operator like `none_failed_one_success` or something like that just because this behavior is more in line with the intuitive reading of `none_failed` and less likely to cause confusion.
I do suggest we update the documentation about `none_failed` trigger rule to match what it actually does. Current the doc says this:
```
none_failed: all parents have not failed (failed or upstream_failed) i.e. all parents have succeeded or been skipped
```
To describe `none_failed` more precisely, it should say something like this instead:
```
none_failed: all parents have not failed (failed or upstream_failed) i.e. all parents have succeeded or been skipped. However if all parents are skipped, the task with none_failed trigger_rule will also be skipped.
```
Whereas the new trigger_rule you are going to add can described something like this:
```
all_success_or_skip: All parents have succeeded or been skipped. If all parents are skipped, the task with `all_success_or_skip` still succeeds.
```
Since this involves updating the documentation and no change to the behavior of the existing `none_failed` trigger_rule, it should minimize the amount of changes users have to make.
The addition of `all_success_or_skip` can be made in the same PR or a different one. It will make it possible to achieve what this PR originally want to do, having a trigger_rule that succeeds whenever parents are successful or skipped.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-580496345
@yuqian90, you do raise a valid point. I see one of two possible solutions then:
1. Keep the current behavior of `none_failed` and create a new trigger rule (`none_failed_including_skips` maybe? I don't know) that behaves as in this PR. I saw keep the current behavior for `none_failed` so as to minimize the amount of refactoring we make people do, even though from a documentation perspective, it would make more sense to change the behavior of `none_failed` and move the current behavior to a new operator like `none_failed_one_success` or something like that just because this behavior is more in line with the intuitive reading of `none_failed` and less likely to cause confusion.
2. It is easy if slightly annoying to work around this by giving an operator with a `none_failed` trigger rule a single upstream `DummyOperator` dependency so that it has at least one success. If we just want to keep the status quo, we definitely want to update the documentation to make it clear what the actual behavior is and how to get the current documented behavior if you want 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] yuqian90 edited a comment on issue #7098: [AIRFLOW-4453]
Make behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
yuqian90 edited a comment on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-581009288
@TV4Fun this sounds like a good suggestion. How about calling the new trigger rule `all_success_or_skip` ? That makes it easier to comprehend.
> 1. Keep the current behavior of `none_failed` and create a new trigger rule (`none_failed_including_skips` maybe? I don't know) that behaves as in this PR. I saw keep the current behavior for `none_failed` so as to minimize the amount of refactoring we make people do, even though from a documentation perspective, it would make more sense to change the behavior of `none_failed` and move the current behavior to a new operator like `none_failed_one_success` or something like that just because this behavior is more in line with the intuitive reading of `none_failed` and less likely to cause confusion.
I do suggest we update the documentation about `none_failed` trigger rule to match what it actually does. Current the doc says this:
```
none_failed: all parents have not failed (failed or upstream_failed) i.e. all parents have succeeded or been skipped
```
To describe `none_failed` more precisely, it should say something like this instead:
```
none_failed: all parents have not failed (failed or upstream_failed) i.e. all parents have succeeded or been skipped. However if all parents are skipped, the task with none_failed trigger_rule will also be skipped.
```
The doc also has a paragraph that says this, which is not 100% accurate:
```
The join task will be triggered as soon as branch_false has been skipped (a valid completion state) and follow_branch_a has succeeded. Because skipped tasks will not cascade through none_failed.
```
A more accurate description is like:
```
The join task will be triggered as soon as branch_false has been skipped (a valid completion state) and follow_branch_a has succeeded. Because skipped tasks will not cascade through none_failed as long as some other parent tasks have succeeded.
```
Whereas the new trigger_rule you are going to add can described something like this:
```
all_success_or_skip: All parents have succeeded or been skipped. If all parents are skipped, the task with `all_success_or_skip` still succeeds.
```
Since this involves updating the documentation and no change to the behavior of the existing `none_failed` trigger_rule, it should minimize the amount of changes users have to make.
The addition of `all_success_or_skip` can be made in the same PR or a different one. It will make it possible to achieve what this PR originally want to do, having a trigger_rule that succeeds whenever parents are successful or skipped.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-589475906
Closing this in favor of #7464.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-587240265
Hi @yuqian90! I would like to work more on this, but am going to he very
busy for the foreseeable future. If someone would like to take this over or
make their own version, I have no objection.
On Mon, Feb 17, 2020, 5:28 PM yuqian90 <no...@github.com> wrote:
> Hi, @TV4Fun <https://github.com/TV4Fun> would you like to continue
> working on this PR? This is definitely a good change to make. The issue
> regarding nested BranchPythonOperator can be addressed with something
> like the create_join() in my previous comment. Please consider it. Thanks!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/7098?email_source=notifications&email_token=ABD4ECE6QP4BQNOAQCR5SJ3RDM2T5A5CNFSM4KEBP7VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMAHBQA#issuecomment-587231424>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABD4ECFHJWESFZCYKYAVELTRDM2T5ANCNFSM4KEBP7VA>
> .
>
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] yuqian90 commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
yuqian90 commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-587298029
> Hi @yuqian90! I would like to work more on this, but am going to he very busy for the foreseeable future. If someone would like to take this over or make their own version, I have no objection.
> […](#)
> On Mon, Feb 17, 2020, 5:28 PM yuqian90 ***@***.***> wrote: Hi, @TV4Fun <https://github.com/TV4Fun> would you like to continue working on this PR? This is definitely a good change to make. The issue regarding nested BranchPythonOperator can be addressed with something like the create_join() in my previous comment. Please consider it. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7098?email_source=notifications&email_token=ABD4ECE6QP4BQNOAQCR5SJ3RDM2T5A5CNFSM4KEBP7VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMAHBQA#issuecomment-587231424>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD4ECFHJWESFZCYKYAVELTRDM2T5ANCNFSM4KEBP7VA> .
Thanks! I'll take it from where you left it and put up a new PR soon.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-571854915
I think that a note in UPDATING.md could also be useful. Some people may have DAGs that relied on this condition, so we should tell them how to update their DAGs.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun closed pull request #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun closed pull request #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-571861953
Adding a note. I'm trying to think of a workaround if anyone actually was relying on the previous behavior. I don't know if anyone was, but if they were, we may want to add another `trigger_rule` that explicitly implements that.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-587300834
Thank you @yuqian90! 👍
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] yuqian90 commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
yuqian90 commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-582219010
> @yuqian90 I agree with what @ChrisEverling said [here](https://github.com/apache/airflow/pull/4182#discussion_r362862918), and with the folks who created the issue, "none failed" includes "all skipped", and the documentation backs that up. It took some debugging to find this extra condition, and I still don't understand the rationale behind it.
>
> Why not make the current trigger rule behave as documented, and add a new one to preserve the unexpected "none failed and not all skipped" behavior?
@jared-martin Sure. I think continuing what @TV4Fun was originally doing in this PR is also fine. However, I was pointing out in my previous comments the side-effect of making this change when e.g. when join tasks with `none_failed` trigger_rule are nested in branching logic.
If people think it's better to proceed with fixing the existing `none_failed` to match what the doc says, I would suggest we update the parts in the doc about using `none_failed` task to create join for `BranchPythonOperator`. It can no longer be a `none_failed` `DummyOperator`. It needs to be something like the following, where the join task checks the state of its parent `BranchPythonOperator` and skip itself if the parent is skipped. I would highly recommend making `create_join` a free function inside `airflow/operators/python.py` to make it more convenient for users of `BranchPythonOperator` and `ShortCircuitOperator`.
```python
from airflow.models import DAG
from airflow.operators.dummy_operator import DummyOperator
from airflow.operators.python import BranchPythonOperator, PythonOperator
from airflow.utils.dates import days_ago
def create_join(branch_task_id, *args, **kwargs):
"""
:param branch_task_id: The task_id of the parent BranchPythonOperator task that created the branching logic.
:type branch_task_id: str
"""
def python_callable(ti, **_):
from airflow.utils.session import create_session
from airflow.exceptions import AirflowSkipException
from airflow.utils.state import State
from airflow.models import TaskInstance
with create_session() as session:
branch_ti = session.query(TaskInstance).filter(TaskInstance.dag_id == ti.dag_id,
TaskInstance.task_id == branch_task_id,
TaskInstance.execution_date == ti.execution_date
).first()
if not branch_ti:
return
if branch_ti.state == State.SKIPPED:
raise AirflowSkipException("Skipping {} because parent BranchPythonOperator {} is skipped."
.format(ti.task_id, branch_task_id))
return PythonOperator(trigger_rule="none_failed", python_callable=python_callable, *args, **kwargs)
with DAG(dag_id="example_nested_branch_dag", start_date=days_ago(2), schedule_interval="@daily") as dag:
branch_1 = BranchPythonOperator(task_id="branch_1", python_callable=lambda: "false_1")
true_1 = DummyOperator(task_id="true_1")
false_1 = DummyOperator(task_id="false_1")
branch_2 = BranchPythonOperator(task_id="branch_2", python_callable=lambda: "true_2")
true_2 = DummyOperator(task_id="true_2")
false_2 = DummyOperator(task_id="false_2")
join_2 = create_join(task_id="join_2", branch_task_id=branch_2.task_id)
true_3 = DummyOperator(task_id="true_3")
join_1 = create_join(task_id="join_1", branch_task_id=branch_1.task_id)
branch_1 >> [true_1, false_1]
true_1 >> branch_2 >> [true_2, false_2] >> join_2 >> true_3 >> join_1
false_1 >> join_1
```
You can see the DAG now behaves the same as what it should have been before this PR:
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7098:
[AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-571862145
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=h1) Report
> Merging [#7098](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/fe20ef6147f157b7c963ad46e13aff7c16458632?src=pr&el=desc) will **decrease** coverage by `0.27%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7098 +/- ##
==========================================
- Coverage 85.15% 84.87% -0.28%
==========================================
Files 680 680
Lines 38824 38822 -2
==========================================
- Hits 33061 32951 -110
- Misses 5763 5871 +108
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/ti\_deps/deps/trigger\_rule\_dep.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcHMvdHJpZ2dlcl9ydWxlX2RlcC5weQ==) | `93.24% <ø> (+2.45%)` | :arrow_up: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
| [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
| [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
| [...rflow/contrib/operators/kubernetes\_pod\_operator.py](https://codecov.io/gh/apache/airflow/pull/7098/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZF9vcGVyYXRvci5weQ==) | `78.75% <0%> (-20%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=footer). Last update [fe20ef6...19f9fe8](https://codecov.io/gh/apache/airflow/pull/7098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-571854505
Can you add tests to avoid regression?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-575017704
@mik-laj, unit test and `UPDATING` note added.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] TV4Fun commented on issue #7098: [AIRFLOW-4453] Make
behavior of `none_failed` consistent with documentation
Posted by GitBox <gi...@apache.org>.
TV4Fun commented on issue #7098: [AIRFLOW-4453] Make behavior of `none_failed` consistent with documentation
URL: https://github.com/apache/airflow/pull/7098#issuecomment-580194622
@yuqian90, why would you use the `none_failed` trigger rule here? I can understand wanting `join_2` to execute for either branch chosen by `branch_2`, but why not use `one_success` to get that effect? As I read the documentation, the difference between `one_success` and `none_failed` are that `one_success` does not wait for all parents to be done, and it requires at least one parent success. What is a real use case where you would want to use `none_failed` as in your example?
----------------------------------------------------------------
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
With regards,
Apache Git Services