Skip to content

Ensure TaskMap only checks "relevant" dependencies#23053

Merged
ashb merged 2 commits into
apache:mainfrom
astronomer:task-dependency-only-check-relevant
Apr 19, 2022
Merged

Ensure TaskMap only checks "relevant" dependencies#23053
ashb merged 2 commits into
apache:mainfrom
astronomer:task-dependency-only-check-relevant

Conversation

@uranusjr
Copy link
Copy Markdown
Member

When looking for "mapped dependants" of a task, we only want a task if it not only is a direct downstream of the task, but also it actually "uses" the task's pushed XCom for task mapping. So we need to peek into the mapped downstream task's expansion kwargs, and only count it as a mapped dependant if the upstream is referenced there.

Fix #23018.

@uranusjr uranusjr requested review from XD-DENG, ashb and kaxil as code owners April 18, 2022 08:29
@boring-cyborg boring-cyborg Bot added the area:Scheduler including HA (high availability) scheduler label Apr 18, 2022
@uranusjr uranusjr added this to the Airflow 2.3.0 milestone Apr 18, 2022
@uranusjr uranusjr force-pushed the task-dependency-only-check-relevant branch from a52034e to 69d53b7 Compare April 18, 2022 08:32
@uranusjr uranusjr changed the title Test case for TaskMap only check "relevant" dependencies Ensure TaskMap only checks "relevant" dependencies Apr 18, 2022
@uranusjr uranusjr force-pushed the task-dependency-only-check-relevant branch from 69d53b7 to ce7f207 Compare April 18, 2022 14:26
@uranusjr

This comment was marked as outdated.

@uranusjr uranusjr force-pushed the task-dependency-only-check-relevant branch 3 times, most recently from 4546adb to 6bb09ab Compare April 19, 2022 02:25
Copy link
Copy Markdown
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nit is that maybe we don't need iter_ in the function name?

@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 19, 2022
@github-actions
Copy link
Copy Markdown
Contributor

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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

When looking for "mapped dependants" of a task, we only want a task if
it not only is a direct downstream of the task, but also it actually
"uses" the task's pushed XCom for task mapping. So we need to peek into
the mapped downstream task's expansion kwargs, and only count it as a
mapped dependant if the upstream is referenced there.
@ashb ashb force-pushed the task-dependency-only-check-relevant branch from 6bb09ab to 0194859 Compare April 19, 2022 16:12
@ashb ashb merged commit 197cff3 into apache:main Apr 19, 2022
@ashb ashb deleted the task-dependency-only-check-relevant branch April 19, 2022 18:02
@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dynamic-task-mapping AIP-42 area:Scheduler including HA (high availability) scheduler changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A task's returned object should not be checked for mappability if the dag doesn't use it in an expansion.

3 participants