viiccwen commented on code in PR #69933:
URL: https://github.com/apache/airflow/pull/69933#discussion_r3595126396
##########
airflow-core/tests/unit/utils/test_task_group.py:
##########
@@ -493,17 +493,11 @@ def test_task_group_to_dict_and_dag_edges(dag_maker):
nodes = task_group_to_dict(dag.task_group)
edges = dag_edges(dag)
+ # group_d depends on group_c (`group_d << group_c`), so it must sort after
group_c,
+ # not before task1 as it did prior to the #65291/#67964 topological-sort
fix.
Review Comment:
Could we mention the intention of the testing instead of using issue number?
##########
airflow-core/tests/unit/utils/test_task_group.py:
##########
@@ -1178,6 +1183,80 @@ def test_topological_sort_serialized_layered():
)
+def test_topological_group_dep_list_syntax():
+ """List-based deps (`[b0, b1] >> a`) must produce the same topological
order as individual deps.
+
+ Regression test for apache/airflow#65291: declaring a group dependency via
a list
Review Comment:
same here.
##########
airflow-core/src/airflow/serialization/definitions/taskgroup.py:
##########
Review Comment:
This rebuilds the complete Dag-level TaskGroup mapping every time
`topological_sort()` is called. The Grid / Graph serializers recursively call
`topological_sort()` for every nested TaskGroup, so a dag with G non-empty
groups now performs approximately G full traversals of the same G-group tree.
That changes the overall group-sorting cost to $O(G^2)$.
Could we build the mapping once for the entire recursive render and pass it
through to each nested `topological_sort()` call? The same in the Task SDK's
`topological_sort()` path.
--
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]
