Restoring include_downstream_dags logic to clear downstream Dags - #65314
Restoring include_downstream_dags logic to clear downstream Dags#65314jroachgolf84 wants to merge 15 commits into
include_downstream_dags logic to clear downstream Dags#65314Conversation
|
Hey, any updates on this? |
|
@jason810496 - can you take a look at this one for me? |
|
@bugraoz93 - do you mind taking a look at this one? |
issue-61451: Restoring include_downstream_dags logicinclude_downstream_dags logic to clear downstream Dags
|
@kaxil - do you mind taking another look? |
|
Quickest fix: git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-leaseAutomated nudge — ignore if you're not ready to rebase. This comment is updated in place on future |
8ada24e to
bd41ac1
Compare
|
@kaxil - feedback implemented. Do you mind taking another look? Thanks! |
Lee-W
left a comment
There was a problem hiding this comment.
Mostly nits, but overall this looks good. The CI failure is unrelated—rebasing on the main branch should fix it.
Co-authored-by: Wei Lee <hello@wei-lee.me>
Co-authored-by: Wei Lee <hello@wei-lee.me>
|
@Lee-W - thanks for the review! I've implemented all your feedback. |
Lee-W
left a comment
There was a problem hiding this comment.
mostly good on my end. would appreciate a second pair of eyes
Co-authored-by: Wei Lee <hello@wei-lee.me>
| dag_bag = DBDagBag(load_op_links=False) | ||
|
|
||
| for tii in external_tis: | ||
| external_dag = dag_bag.get_latest_version_of_dag(tii.dag_id, session=session) |
There was a problem hiding this comment.
This resolves the child Dag from its latest version, but the code being restored used the run-pinned one: dag_bag.get_dag_for_run(tii.dag_run, session=session), at dag.py:1029 before #54383 removed it. This route already calls get_dag_for_run in two other spots (lines 335 and 882), so the helper is right there.
Why it matters: the partial_subset(include_downstream=True) just below becomes the task_id.in_(...) filter for the child's TI query. With bundle versioning on, that means we work out the downstream closure from a graph the historical child run may never have had. Say child v1 is wait >> transform >> load and v2 drops transform: clearing the parent reruns load and leaves transform sitting at SUCCESS in the v1 run.
get_dag_for_run(...) or get_latest_version_of_dag(...) keeps a fallback for a missing serialized row, and still routes through _get_dag, so the deserialize-once behaviour you added earlier holds.
|
|
||
| # Follow ExternalTaskMarker connections when explicitly requested via include_downstream_dags, or | ||
| # automatically whenever downstream clearing is selected (restoring Airflow 2 behavior) | ||
| include_dependent_dags = body.include_downstream_dags or downstream |
There was a problem hiding this comment.
Coming back to the flag overlap I asked about in #65314 (comment), now that I've read the code this restores.
include_downstream_dags: false can't actually turn the cascade off, because or downstream only ever reacts to true. And the comment doesn't quite line up with what was there before: the pre-removal clear() hard-coded include_dependent_dags=True (dag.py:1404), it wasn't keyed off the same-Dag downstream flag at all.
The UI side is what bothers me more. ClearTaskInstanceDialog seeds selectedOptions with ["downstream"] (line 80, and the group dialog does the same at 54), so a default single-task clear from the grid now walks into other Dags and the caller has no way to say no.
run_on_latest_version in this same body model is already bool | None. Same treatment here gets you the opt-out and keeps the current UI default:
include_downstream_dags: bool | None = None
...
downstream if body.include_downstream_dags is None else body.include_downstream_dags| } | ||
|
|
||
| # list of all TI's that can be cleared (TI's within the Dags from above) | ||
| task_instances = [ti for ti in task_instances if ti.dag_id in editable_dag_ids] |
There was a problem hiding this comment.
Now that this list can span Dags, the single resolved_run_on_latest computed up at line 863 from the path dag_id gets applied to child Dag runs too. resolve_run_on_latest_version only takes one dag_id and reads the parent's Dag-level setting; clear_task_instances then rewrites each run's created_dag_version_id and calls verify_integrity(), which can add or drop TIs in that run.
Since the clear dialog always sends the field, the explicit_value is not None short-circuit means a child Dag with rerun_with_latest_version=False gets upgraded regardless of its own setting.
Grouping the TIs by dag_id and resolving per Dag when body.run_on_latest_version is None would respect each Dag's policy, with an explicit value from the caller still winning everywhere.
|
|
||
| except MaxRecursionDepthError as e: | ||
| raise HTTPException(status.HTTP_400_BAD_REQUEST, str(e)) from e | ||
| except ParserError as e: |
There was a problem hiding this comment.
Still ParserError-only, so this misses the inputs that raise ValueError directly. Same point as #65314 (comment), which got resolved without the change.
Two shapes reach it on the pinned pendulum 3.2.0. pendulum.parse("") raises ValueError: year 0 is out of range, which isn't a ParserError. And pendulum.parse("P1D") returns a Duration, which then goes into DagRun.logical_date == <Duration> and comes back as a StatementError. Both end up as a 500. A marker with logical_date="{{ dag_run.conf.get('x', '') }}" rendering empty gets you the first one.
Catching ValueError picks up the bare case plus the ParserError ones, since it's the parent. The Duration shape needs a check on what parse actually returned.
One thing to flag: test_invalid_external_task_marker_logical_date_returns_400 can't catch either of these. It sets side_effect=ParserError(...) on a fully mocked clear(), so it only proves the except clause catches what the test hands it, not what the real call site raises.
Description
Restoring behavior to clear downstream DAGs when a Task state is cleared (if applicable).
closes: #61451
Testing
Unit-tests were added/updated to test these changes. The changes were also tested E2E.
Unit Tests
The unit-tests added/updated as part of this effort can be found in these files:
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.pyairflow-core/tests/unit/serialization/definitions/test_dag.pyThese tests can be executed using the commands below:
E2E Testing
The DAG below was used to test the changes int his PR. This DAG is made up of a parent and child DAG. The
ExternalTaskMarkerOperator is cleared in the parent DAG, the downstream Tasks in the child DAG are also cleared. That's showed in the video below.Video
issue-61451-include-downstream-tasks.mov