Skip to content

Move Dag cycle detection from the Task SDK to shared - #74034

Merged
pierrejeambrun merged 2 commits into
mainfrom
jason/lang-sdk-e2e/02b-shared-dag-cycle-detection
Oct 2, 2026
Merged

pierrejeambrun merged 2 commits into
mainfrom
jason/lang-sdk-e2e/02b-shared-dag-cycle-detection

Conversation

@jason810496

@jason810496 jason810496 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Stack (bottom to top): #74034, #74040, #74041, #74042, #74035, #74043, #74036, #74037, #73841, #73845, #73846, #73847

Why

Move the Cycle detection part to share so that all the further SDKs native Dag can be validated by the same single source.

How

Core cannot reuse DAG.check_cycle as it is: it is a method on the authoring class and reads task objects, while core holds serialized data it should not hydrate just to run this check.

  • The traversal moves to shared/dagnode, which task-sdk and airflow-core already depend on.
  • The graph is described by callbacks, not node objects, so a caller answers from whatever data it holds.
  • It returns the offending task id instead of raising. AirflowDagCycleException lives in the Task SDK, and importing it here would invert the dependency, so each caller raises its own error.
from airflow._shared.dagnode.cycle import detect_cycle

downstream = {"extract": ["load"], "load": ["extract"]}
detect_cycle(downstream, downstream.__getitem__)  # "load", or None when there is no cycle

Was generative AI tooling used to co-author this PR?
  • Yes, with help of Claude Code (Claude Opus 4.5) following the guidelines

@jason810496 jason810496 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The current static check failure is not related and will be resolved once #73963 (review) finish.

Comment thread shared/dagnode/tests/dagnode/test_cycle.py Outdated
@jason810496
jason810496 force-pushed the jason/lang-sdk-e2e/02b-shared-dag-cycle-detection branch 2 times, most recently from 541001b to e5ae872 Compare October 2, 2026 04:07
jason810496 and others added 2 commits October 2, 2026 05:40
Cycle detection only runs for Dags that go through the Python SDK's
DAG object. Dags authored with non-Python language SDKs arrive at core
already serialized and never pass through that object, so nothing
rejects a cycle in them today.

Core cannot reuse the SDK implementation as-is: it is a method on the
authoring class and reaches for task objects, while core holds raw
serialized data it should not have to hydrate just to run a check.
Moving the traversal into the shared distribution both distributions
already depend on, and describing the graph through callbacks instead
of node objects, lets core add that check against the serialized form
in a follow-up without duplicating the algorithm or inverting the
dependency by importing the SDK's exception.
The reversed-edge test passed or failed with the acyclic cases, so it is
replaced by a cyclic case whose node id is empty. It fails if the
traversal treats an empty id as no child.

Co-Authored-By: Claude <noreply@anthropic.com>
@jason810496
jason810496 force-pushed the jason/lang-sdk-e2e/02b-shared-dag-cycle-detection branch from e5ae872 to 2060349 Compare October 2, 2026 05:44
@uranusjr

uranusjr commented Oct 2, 2026

Copy link
Copy Markdown
Member

From what I can tell this does not make anything in core call the shared code yet. Is this planned to be done in a later PR? (The PR title probably should be adjusted slightly if that’s the case.)

@pierrejeambrun pierrejeambrun left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

From what I can tell this does not make anything in core call the shared code yet. Is this planned to be done in a later PR? (The PR title probably should be adjusted slightly if that’s the case.)

I updated the description which is misleading at this point. Core calling this is done in #74041 I believe and then wired in the dag processor in #74035 (by calling validate_serialized_dag)

@pierrejeambrun pierrejeambrun changed the title Share Dag cycle detection between the Task SDK and core Move Dag cycle detection from the Task SDK to shared Oct 2, 2026
@pierrejeambrun
pierrejeambrun merged commit 7792739 into main Oct 2, 2026
108 checks passed
@pierrejeambrun
pierrejeambrun deleted the jason/lang-sdk-e2e/02b-shared-dag-cycle-detection branch October 2, 2026 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants