dheerajturaga commented on code in PR #73724:
URL: https://github.com/apache/airflow/pull/73724#discussion_r4211164936
##########
airflow-core/src/airflow/serialization/definitions/taskgroup.py:
##########
@@ -236,10 +236,13 @@ def topological_sort(
"""
Sort children topologically — a task always comes after its upstream
dependencies.
- See ``TaskGroup.topological_sort`` in task-sdk for the algorithm.
Cycles are
- treated as corrupt input: ``DAG.check_cycle`` rejects cyclic Dags
before
- serialization, so a cycle reaching this code indicates malformed
serialized data,
- and we raise ``ValueError`` rather than silently looping forever.
+ See ``TaskGroup.topological_sort`` in task-sdk for the algorithm.
Unlike the task-sdk
Review Comment:
Good point, the raise isn't needed: #73746 detects these Dags with its own
check, and that part of the description was out of date. The SDK sort now gets
the same fallback. To avoid a second copy, `_compute_pass_order`,
`_sort_cyclic_projection` and `_find_projection_components` moved into
`TaskGroupMixin` in the shared `dagnode` library, and both sorts call them.
This also fixes the deprecated `DAG.topological_sort()`, which has raised
`AirflowDagCycleException` for these Dags since 3.3.1
(`test_dag_topological_sort_task_group_cycle`), and
`test_topological_sort_task_group_cycle` now asserts the two sorts give the
same order. #73746 adds its own copy of `_find_projection_components` to the
SDK `TaskGroup`; whichever of the two PRs lands second will drop it. I've
updated the PR description.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @dheerajturaga before posting
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
