Skip to content

Commit d758c78

Browse files
authored
Refactor group_nodes_by_folder hierarchical naming to be opt-in (#2862)
#2824 fixed `group_nodes_by_folder` collapsing folders that share a leaf name under different parents (e.g. `staging/aurora` and `marts/aurora`) — thanks @anor4k for catching and fixing this! My only concern is that it changes the names of the generated Cosmos task groups, which is a breaking change for users who reference those task groups from other tasks, e.g.: ``` non_cosmos_task = ... cosmos_dbt_task_group = task_group.children["tg_a.models"].children["tg_a.models.marts"] non_cosmos_task >> cosmos_dbt_task_group ``` To avoid this breaking change in a 1.x release, this PR makes the fix configurable, defaulting it to `False` and following the same config pattern as the existing Cosmos settings: https://astronomer.github.io/astronomer-cosmos/reference/configs/cosmos-conf.html When `enable_hierarchical_naming_for_group_nodes_by_folder` is disabled (the default), Cosmos keeps the previous task-group names. When it is enabled — through the setting or the `AIRFLOW__COSMOS__ENABLE_HIERARCHICAL_NAMING_FOR_GROUP_NODES_BY_FOLDER` environment variable — Cosmos applies the #2824 fix, and folders that share a leaf name render as distinct task groups. We intend to make hierarchical naming the only behaviour, and drop this setting altogether, in Cosmos 2.0: #2863
1 parent 908e12f commit d758c78

6 files changed

Lines changed: 161 additions & 36 deletions

File tree

cosmos/airflow/graph.py

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from collections import OrderedDict, defaultdict
44
from collections.abc import Callable
55
from copy import deepcopy
6+
from pathlib import Path
67
from typing import Any
78

89
try: # Airflow 3
@@ -948,26 +949,35 @@ def identify_detached_nodes(
948949

949950

950951
def create_task_groups_based_on_folder(
951-
dag: DAG, node: DbtNode, parent_task_group: TaskGroup | None, task_groups: dict[str, TaskGroup]
952+
dag: DAG,
953+
node: DbtNode,
954+
parent_task_group: TaskGroup | None,
955+
task_groups: dict[str, TaskGroup],
956+
use_hierarchical_naming: bool = False,
952957
) -> TaskGroup | None:
953958
"""
954959
Generate the parent task group for the given node based on the node's file path. If a TaskGroup is given, it will
955960
be used as the parent group.
961+
962+
When ``use_hierarchical_naming`` is True, groups are cached by their cumulative folder path, so that folders
963+
sharing a leaf name under different parents (e.g. ``staging/aurora`` and ``marts/aurora``) become distinct task
964+
groups instead of collapsing into the first one created. When False (default), groups are cached by the bare
965+
folder name, preserving the legacy behaviour — and the existing task-group ids that users may reference — at the
966+
cost of collapsing same-named folders. See https://github.com/astronomer/astronomer-cosmos/pull/2824.
956967
"""
957968
task_group = None
958-
resource_file_path_parts = str(node.original_file_path).split("/")[:-1]
959-
# Cache groups by their cumulative path rather than the bare folder name, so
960-
# that folders sharing a leaf name under different parents (e.g.
961-
# ``staging/aurora`` and ``marts/aurora``) become distinct task groups instead
962-
# of collapsing into the first one created.
969+
# Use ``Path.parts`` (OS-agnostic) rather than ``str(path).split("/")`` so folder grouping also
970+
# works on Windows, where ``str(Path(...))`` uses backslash separators.
971+
resource_file_path_parts = Path(node.original_file_path).parts[:-1]
963972
cumulative_path = ""
964973
for resource_file_path_part in resource_file_path_parts:
965974
cumulative_path = f"{cumulative_path}/{resource_file_path_part}" if cumulative_path else resource_file_path_part
966-
if cumulative_path in task_groups:
967-
task_group = task_groups[cumulative_path]
975+
cache_key = cumulative_path if use_hierarchical_naming else resource_file_path_part
976+
if cache_key in task_groups:
977+
task_group = task_groups[cache_key]
968978
else:
969979
task_group = TaskGroup(dag=dag, group_id=resource_file_path_part, parent_group=parent_task_group)
970-
task_groups[cumulative_path] = task_group
980+
task_groups[cache_key] = task_group
971981
parent_task_group = task_group
972982
return task_group
973983

@@ -1145,6 +1155,7 @@ def build_airflow_graph(
11451155
:return: Dictionary mapping dbt nodes (node.unique_id to Airflow task)
11461156
"""
11471157
group_nodes_by_folder = render_config.group_nodes_by_folder
1158+
use_hierarchical_naming = settings.enable_hierarchical_naming_for_group_nodes_by_folder
11481159
tasks_map: dict[str, TaskGroup | BaseOperator] = {}
11491160
task_groups: dict[str, TaskGroup] = {}
11501161
task_or_group: TaskGroup | BaseOperator | None
@@ -1180,7 +1191,7 @@ def build_airflow_graph(
11801191

11811192
for node_id, node in nodes.items():
11821193
task_group = (
1183-
create_task_groups_based_on_folder(dag, node, parent_task_group, task_groups)
1194+
create_task_groups_based_on_folder(dag, node, parent_task_group, task_groups, use_hierarchical_naming)
11841195
if group_nodes_by_folder
11851196
else task_group
11861197
)

cosmos/settings.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,13 @@
3333
enable_cache_dbt_ls = conf.getboolean("cosmos", "enable_cache_dbt_ls", fallback=True)
3434
enable_cache_dbt_yaml_selectors = conf.getboolean("cosmos", "enable_cache_dbt_yaml_selectors", fallback=True)
3535
enable_lax_selector_parsing = conf.getboolean("cosmos", "enable_lax_selector_parsing", fallback=False)
36+
# When RenderConfig.group_nodes_by_folder is enabled, key folder task groups by their full path so
37+
# that folders sharing a leaf name under different parents render as distinct task groups. Defaults
38+
# to False to preserve existing task-group ids (enabling it is a breaking change for DAGs that
39+
# reference Cosmos task groups by id); expected to become the default in Cosmos 2.0. See #2824.
40+
enable_hierarchical_naming_for_group_nodes_by_folder = conf.getboolean(
41+
"cosmos", "enable_hierarchical_naming_for_group_nodes_by_folder", fallback=False
42+
)
3643
rich_logging = conf.getboolean("cosmos", "rich_logging", fallback=False)
3744
dbt_docs_dir = conf.get("cosmos", "dbt_docs_dir", fallback=None)
3845
dbt_docs_conn_id = conf.get("cosmos", "dbt_docs_conn_id", fallback=None)

docs/guides/translate_dbt_to_airflow/render-config.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ The ``RenderConfig`` class takes the following arguments:
3333
- ``should_detach_multiple_parents_tests``: A boolean to control if tests that depend on multiple parents should be run as standalone tasks. See :doc:`Testing Behavior </guides/translate_dbt_to_airflow/testing-behavior>` for more information.
3434
- ``enable_owner_inheritance``: (introduced in 1.10.2) A boolean to control if dbt owners should be imported as part of the airflow DAG owners. Defaults to True.
3535
- ``ephemeral_models_as_empty_operator``: (new in v1.15.0) A boolean to control how ephemeral models are rendered. Ephemeral models are inlined as CTEs into downstream models and never written to the warehouse, so running them via a dbt operator is effectively a no-op. When ``True`` (default), they are rendered as ``EmptyOperator`` tasks, which preserves the dependency chain that passes through them while avoiding wasted dbt invocations and decluttering the DAG. Because the ``EmptyOperator`` does not run dbt, behaviour tied to the ephemeral model's run task no longer occurs: no Airflow Dataset/Asset is emitted for it (so dataset-scheduled DAGs keyed on it are not triggered), task callbacks are not invoked, no OpenLineage events are produced, and per-node operator arguments (e.g. ``profile_args``) do not apply. Set to ``False`` to render them as regular dbt run tasks.
36-
- ``group_nodes_by_folder``: When enabled, groups nodes by folder structure, creating a ``TaskGroup`` per resource type and folder. Disabled by default.
36+
- ``group_nodes_by_folder``: When enabled, groups nodes by folder structure, creating a ``TaskGroup`` per resource type and folder. Disabled by default. By default, folders that share a leaf name under different parents (e.g. ``staging/aurora`` and ``marts/aurora``) collapse into a single task group; set the :ref:`enable_hierarchical_naming_for_group_nodes_by_folder` Cosmos config (or the ``AIRFLOW__COSMOS__ENABLE_HIERARCHICAL_NAMING_FOR_GROUP_NODES_BY_FOLDER`` environment variable) to ``True`` to render them as distinct task groups. It is disabled by default because enabling it changes task-group ids, which is a breaking change for DAGs that reference Cosmos task groups by id, and is expected to become the default in Cosmos 2.0.
3737

3838
How to run dbt ls (invocation mode)
3939
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

docs/reference/configs/cosmos-conf.rst

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,22 @@ This page lists all available `Apache Airflow® <https://airflow.apache.org/>`_
7373
- Default: ``False``
7474
- Environment Variable: ``AIRFLOW__COSMOS__ENABLE_LAX_SELECTOR_PARSING``
7575

76+
.. _enable_hierarchical_naming_for_group_nodes_by_folder:
77+
78+
`enable_hierarchical_naming_for_group_nodes_by_folder`_:
79+
Only relevant when ``RenderConfig.group_nodes_by_folder`` is enabled. When enabled, folder task groups
80+
are keyed by their full folder path, so folders that share a leaf name under different parents
81+
(e.g. ``staging/aurora`` and ``marts/aurora``) render as distinct task groups instead of collapsing into
82+
the first one created. Disabled by default to preserve the existing task-group ids — enabling it changes
83+
those ids, which is a breaking change for DAGs that reference Cosmos task groups by id. This is expected
84+
to become the default in Cosmos 2.0.
85+
86+
Being an Airflow configuration setting, it applies **globally to every Cosmos DAG** in the deployment
87+
and cannot be configured per DAG (unlike the ``RenderConfig.group_nodes_by_folder`` option itself).
88+
89+
- Default: ``False``
90+
- Environment Variable: ``AIRFLOW__COSMOS__ENABLE_HIERARCHICAL_NAMING_FOR_GROUP_NODES_BY_FOLDER``
91+
7692
.. _enable_cache_partial_parse:
7793

7894
`enable_cache_partial_parse`_:

tests/airflow/test_graph.py

Lines changed: 100 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,16 @@ def test_calculate_datached_node_name_under_is_under_250():
146146
assert calculate_detached_node_name(node) == "detached_1_test"
147147

148148

149+
@pytest.fixture
150+
def hierarchical_naming_disabled(monkeypatch):
151+
"""Pin the global hierarchical-naming setting to False so the folder-grouping tests stay hermetic
152+
regardless of any AIRFLOW__COSMOS__ENABLE_HIERARCHICAL_NAMING_FOR_GROUP_NODES_BY_FOLDER set in the
153+
developer's environment. See https://github.com/astronomer/astronomer-cosmos/issues/1763."""
154+
monkeypatch.setattr("cosmos.settings.enable_hierarchical_naming_for_group_nodes_by_folder", False)
155+
156+
149157
@pytest.mark.integration
150-
def test_build_airflow_graph_with_after_each():
158+
def test_build_airflow_graph_with_after_each(hierarchical_naming_disabled):
151159
with DAG("test-id", start_date=datetime(2022, 1, 1)) as dag:
152160
task_args = {
153161
"project_dir": SAMPLE_PROJ_PATH,
@@ -179,13 +187,13 @@ def test_build_airflow_graph_with_after_each():
179187
"seed_parent_seed",
180188
"gen2.models.parent.run",
181189
"gen2.models.parent.test",
182-
"gen3.models.child_run",
183-
"gen3.models.child2_v2_run",
190+
"gen2.models.child_run",
191+
"gen2.models.child2_v2_run",
184192
]
185193

186194
assert topological_sort == expected_sort
187195
task_groups = dag.task_group_dict
188-
assert len(task_groups) == 5
196+
assert len(task_groups) == 4
189197

190198
assert task_groups["gen2.models.parent"].upstream_task_ids == {"seed_parent_seed"}
191199
assert list(task_groups["gen2.models.parent"].children.keys()) == [
@@ -194,8 +202,8 @@ def test_build_airflow_graph_with_after_each():
194202
]
195203

196204
assert len(dag.leaves) == 2
197-
assert dag.leaves[0].task_id == "gen3.models.child_run"
198-
assert dag.leaves[1].task_id == "gen3.models.child2_v2_run"
205+
assert dag.leaves[0].task_id == "gen2.models.child_run"
206+
assert dag.leaves[1].task_id == "gen2.models.child2_v2_run"
199207

200208
task_seed_parent_seed = dag.tasks[0]
201209
task_parent_run = dag.tasks[1]
@@ -341,7 +349,48 @@ def test_generate_task_or_group_with_dynamic_node_type_no_converter_returns_none
341349

342350

343351
@pytest.mark.integration
344-
def test_build_airflow_graph_with_after_all():
352+
def test_build_airflow_graph_hierarchical_naming_enabled(monkeypatch):
353+
"""With the global setting enabled, folders that share a leaf name under different parents render
354+
as distinct task groups (``gen3.models`` instead of collapsing into ``gen2.models``) — the opt-in
355+
#2824 behaviour, wired through build_airflow_graph. See
356+
https://github.com/astronomer/astronomer-cosmos/issues/1763."""
357+
monkeypatch.setattr("cosmos.settings.enable_hierarchical_naming_for_group_nodes_by_folder", True)
358+
with DAG("test-id", start_date=datetime(2022, 1, 1)) as dag:
359+
task_args = {
360+
"project_dir": SAMPLE_PROJ_PATH,
361+
"conn_id": "fake_conn",
362+
"profile_config": ProfileConfig(
363+
profile_name="default",
364+
target_name="default",
365+
profile_mapping=PostgresUserPasswordProfileMapping(
366+
conn_id="fake_conn",
367+
profile_args={"schema": "public"},
368+
),
369+
),
370+
}
371+
build_airflow_graph(
372+
nodes=sample_nodes,
373+
dag=dag,
374+
execution_mode=ExecutionMode.LOCAL,
375+
test_indirect_selection=TestIndirectSelection.EAGER,
376+
task_args=task_args,
377+
render_config=RenderConfig(
378+
group_nodes_by_folder=True,
379+
test_behavior=TestBehavior.AFTER_EACH,
380+
source_rendering_behavior=SOURCE_RENDERING_BEHAVIOR,
381+
),
382+
dbt_project_name="astro_shop",
383+
)
384+
task_groups = dag.task_group_dict
385+
# gen3/models becomes its own task group instead of collapsing into gen2.models
386+
assert "gen3.models" in task_groups
387+
assert len(task_groups) == 5
388+
leaf_ids = {leaf.task_id for leaf in dag.leaves}
389+
assert leaf_ids == {"gen3.models.child_run", "gen3.models.child2_v2_run"}
390+
391+
392+
@pytest.mark.integration
393+
def test_build_airflow_graph_with_after_all(hierarchical_naming_disabled):
345394
with DAG("test-id", start_date=datetime(2022, 1, 1)) as dag:
346395
task_args = {
347396
"project_dir": SAMPLE_PROJ_PATH,
@@ -374,14 +423,14 @@ def test_build_airflow_graph_with_after_all():
374423
expected_sort = [
375424
"seed_parent_seed",
376425
"gen2.models.parent_run",
377-
"gen3.models.child_run",
378-
"gen3.models.child2_v2_run",
426+
"gen2.models.child_run",
427+
"gen2.models.child2_v2_run",
379428
"astro_shop_test",
380429
]
381430
assert topological_sort == expected_sort
382431

383432
task_groups = dag.task_group_dict
384-
assert len(task_groups) == 4
433+
assert len(task_groups) == 3
385434

386435
assert len(dag.leaves) == 1
387436
assert dag.leaves[0].task_id == "astro_shop_test"
@@ -426,7 +475,7 @@ def test_build_airflow_graph_with_after_all_and_empty_nodes():
426475

427476

428477
@pytest.mark.integration
429-
def test_build_airflow_graph_with_build():
478+
def test_build_airflow_graph_with_build(hierarchical_naming_disabled):
430479
with DAG("test-id", start_date=datetime(2022, 1, 1)) as dag:
431480
task_args = {
432481
"project_dir": SAMPLE_PROJ_PATH,
@@ -457,17 +506,17 @@ def test_build_airflow_graph_with_build():
457506
expected_sort = [
458507
"seed_parent_seed_build",
459508
"gen2.models.parent_model_build",
460-
"gen3.models.child_model_build",
461-
"gen3.models.child2_v2_model_build",
509+
"gen2.models.child_model_build",
510+
"gen2.models.child2_v2_model_build",
462511
]
463512
assert topological_sort == expected_sort
464513

465514
task_groups = dag.task_group_dict
466-
assert len(task_groups) == 4
515+
assert len(task_groups) == 3
467516

468517
assert len(dag.leaves) == 2
469-
assert dag.leaves[0].task_id in ("gen3.models.child_model_build", "gen3.models.child2_v2_model_build")
470-
assert dag.leaves[1].task_id in ("gen3.models.child_model_build", "gen3.models.child2_v2_model_build")
518+
assert dag.leaves[0].task_id in ("gen2.models.child_model_build", "gen2.models.child2_v2_model_build")
519+
assert dag.leaves[1].task_id in ("gen2.models.child_model_build", "gen2.models.child2_v2_model_build")
471520

472521

473522
@pytest.mark.integration
@@ -506,10 +555,7 @@ def test_build_airflow_graph_with_override_profile_config():
506555
assert generated_parent_profile_config.profile_mapping.profile_args["schema"] == "public"
507556

508557

509-
def test_create_task_groups_based_on_folder_distinguishes_same_leaf_name():
510-
"""Folders sharing a leaf name under different parents (e.g. ``staging/aurora``
511-
and ``marts/aurora``) must map to distinct TaskGroups instead of collapsing
512-
into the first one created."""
558+
def _same_leaf_name_nodes() -> tuple[DbtNode, DbtNode]:
513559
staging_node = DbtNode(
514560
unique_id=f"{DbtResourceType.MODEL.value}.{SAMPLE_PROJ_PATH.stem}.staging_aurora_orders",
515561
resource_type=DbtResourceType.MODEL,
@@ -524,14 +570,22 @@ def test_create_task_groups_based_on_folder_distinguishes_same_leaf_name():
524570
path_base=SAMPLE_PROJ_PATH,
525571
original_file_path=Path("models/marts/aurora/orders.sql"),
526572
)
573+
return staging_node, marts_node
574+
575+
576+
def test_create_task_groups_based_on_folder_distinguishes_same_leaf_name():
577+
"""With ``use_hierarchical_naming=True``, folders sharing a leaf name under different parents
578+
(e.g. ``staging/aurora`` and ``marts/aurora``) must map to distinct TaskGroups instead of
579+
collapsing into the first one created."""
580+
staging_node, marts_node = _same_leaf_name_nodes()
527581

528582
task_groups: dict = {}
529583
with DAG("test-folder-collision", start_date=datetime(2022, 1, 1)) as dag:
530584
staging_group = create_task_groups_based_on_folder(
531-
dag=dag, node=staging_node, parent_task_group=None, task_groups=task_groups
585+
dag=dag, node=staging_node, parent_task_group=None, task_groups=task_groups, use_hierarchical_naming=True
532586
)
533587
marts_group = create_task_groups_based_on_folder(
534-
dag=dag, node=marts_node, parent_task_group=None, task_groups=task_groups
588+
dag=dag, node=marts_node, parent_task_group=None, task_groups=task_groups, use_hierarchical_naming=True
535589
)
536590

537591
assert staging_group is not None
@@ -550,6 +604,27 @@ def test_create_task_groups_based_on_folder_distinguishes_same_leaf_name():
550604
}
551605

552606

607+
def test_create_task_groups_based_on_folder_collapses_same_leaf_name_by_default():
608+
"""Default (``use_hierarchical_naming=False``) preserves the legacy behaviour: folders sharing a
609+
leaf name are keyed by the bare folder name, so the second ``aurora`` reuses the first group.
610+
This keeps existing task-group ids stable (avoiding a breaking change) until the behaviour is
611+
flipped in a future major release."""
612+
staging_node, marts_node = _same_leaf_name_nodes()
613+
614+
task_groups: dict = {}
615+
with DAG("test-folder-collision-legacy", start_date=datetime(2022, 1, 1)) as dag:
616+
staging_group = create_task_groups_based_on_folder(
617+
dag=dag, node=staging_node, parent_task_group=None, task_groups=task_groups
618+
)
619+
marts_group = create_task_groups_based_on_folder(
620+
dag=dag, node=marts_node, parent_task_group=None, task_groups=task_groups
621+
)
622+
623+
# Both folders collapse onto the same cached ``aurora`` group (legacy behaviour).
624+
assert staging_group is marts_group
625+
assert set(task_groups) == {"models", "staging", "marts", "aurora"}
626+
627+
553628
def test_calculate_operator_class():
554629
class_module_import_path = calculate_operator_class(execution_mode=ExecutionMode.KUBERNETES, dbt_class="DbtSeed")
555630
assert class_module_import_path == "cosmos.operators.kubernetes.DbtSeedKubernetesOperator"
@@ -1640,7 +1715,7 @@ def test_watcher_producer_preserves_existing_dbt_cmd_flags(test_behavior):
16401715
assert "--resource-type" in producer_task.dbt_cmd_flags
16411716

16421717

1643-
def test_custom_meta():
1718+
def test_custom_meta(hierarchical_naming_disabled):
16441719
with DAG("test-id", start_date=datetime(2022, 1, 1)) as dag:
16451720
task_args = {
16461721
"project_dir": SAMPLE_PROJ_PATH,
@@ -1669,12 +1744,12 @@ def test_custom_meta():
16691744
)
16701745
# test custom meta (queue, pool)
16711746
for task in dag.tasks:
1672-
if task.task_id == "gen3.models.child2_v2_run":
1747+
if task.task_id == "gen2.models.child2_v2_run":
16731748
assert task.pool == "custom_pool"
16741749
else:
16751750
assert task.pool == "default_pool"
16761751

1677-
if task.task_id == "gen3.models.child_run":
1752+
if task.task_id == "gen2.models.child_run":
16781753
assert task.queue == "custom_queue"
16791754
else:
16801755
assert task.queue == "default"

tests/test_settings.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,22 @@ def test_enable_cache_env_var():
1111
assert settings.enable_cache is False
1212

1313

14+
@patch.dict(
15+
os.environ,
16+
{"AIRFLOW__COSMOS__ENABLE_HIERARCHICAL_NAMING_FOR_GROUP_NODES_BY_FOLDER": "True"},
17+
clear=True,
18+
)
19+
def test_enable_hierarchical_naming_for_group_nodes_by_folder_env_var():
20+
reload(settings)
21+
assert settings.enable_hierarchical_naming_for_group_nodes_by_folder is True
22+
23+
24+
@patch.dict(os.environ, {}, clear=True)
25+
def test_enable_hierarchical_naming_for_group_nodes_by_folder_defaults_to_false():
26+
reload(settings)
27+
assert settings.enable_hierarchical_naming_for_group_nodes_by_folder is False
28+
29+
1430
@patch.dict(os.environ, {"AIRFLOW__COSMOS__ENABLE_DEBUG_MODE": "True"}, clear=True)
1531
def test_enable_debug_mode_env_var():
1632
reload(settings)

0 commit comments

Comments
 (0)