add a new conf to wait past_deps before skipping a task #27710
Conversation
by default, all unit tests should pass
hussein-awala
force-pushed
the
skipping_dep
branch
2 times, most recently
from
b08bde6 to
e765a20
Compare
hussein-awala
force-pushed
the
skipping_dep
branch
from
e765a20 to
91e5201
Compare
hussein-awala
requested review from
uranusjr,
dstandish,
jedcunningham,
kaxil,
ashb and
XD-DENG
as code owners
|
conflicts, I am afraid. |
hussein-awala
force-pushed
the
skipping_dep
branch
from
bafc974 to
060b2dd
Compare
|
@potiuk conflicts are resolved. |
airflow/cli/cli_parser.py
Outdated
| ARG_WAIT_FOR_PAST_DEPENDS_BEFORE_SKIPPING = Arg( | ||
| ("-W", "--wait-for-past-depends-before-skipping"), | ||
| help="Wait for past dependencies before skipping the task when --ignore-depends-on-past is not set", | ||
| action="store_true", | ||
| ) |
There was a problem hiding this comment.
I wonder if we should merge this and --ignore-depends-on-past into one flag, say a --depends-on-past option that allows three possible values check (default), ignore, and wait. This can be done in a subsequent, separate PR, but we should start considering the design.
There was a problem hiding this comment.
I agree. We should simplify.
There was a problem hiding this comment.
I agree but I was trying to add a feature safe with b/c.
I will create a new PR to implement this first proposition and we can discuss the design there.
airflow/cli/commands/task_command.py
Outdated
| ignore_depends_on_past=args.ignore_depends_on_past, | ||
| wait_for_past_depends_before_skipping=args.wait_for_past_depends_before_skipping, |
There was a problem hiding this comment.
And we can merge these two now into an enum, it both is clearer and makes more sense (since as it’s designed now it is nonsensical to set both flags).
|
Comments/responses to the comments @hussein-awala ? Still working on it? |
|
Hi @potiuk, sorry I was ooo |
|
I guesss this needs rebase after merging #28113 |
hussein-awala
requested review from
uranusjr
and removed request for
ashb,
kaxil,
XD-DENG,
dstandish and
jedcunningham
|
@potiuk it's ready, can you check it please? |
|
Do we still need to keep the old argument for backward compatibility? |
@uranusjr which old argument? we changed just the CLI arguments, do you prefer to simplify operator arguments (and/or |
|
Oh sorry, I was reading old code (before we merged this into |
|
@hussein-awala @uranusjr was this included in the most recent release of Airflow or will it be included in a following version? |
|
@gaboc623 since this is a new feature, it will be included in the next minor version 2.6.0 (as mentioned in the PR milestone). |
|
thanks so much! |
|
@hussein-awala can you help me understand why we need to involve xcom here? it seems like a real abuse of xcom system. |
|
also @hussein-awala, can you clarify the behavior change here? what did it do differently before? |
closes: #26200 and #26460
In this PR, I add a new config
wait_for_past_depends_before_skippingwhich isFalseby default. When it isTrue, anddepends_on_pastisTrue, then when it is decided to skip a task because of trigger rules, the task will stay inNonestate waiting the past dependencies to be met.In this case, we can ensure that the task instance will be scheduled only when all the past instances are finished with one of the states
succeededorskipped.