Skip to content

Commit 5ed0b48

Browse files
authored
Ensure dag_params contain schedule before operating on it (#488)
Summary This PR updates DAG Factory’s handling of the schedule argument to ensure, by adding tests, that when no schedule is provided, we do not explicitly set it to None (or any default). Instead, we allow Airflow’s own initialisation logic to apply its built-in defaults. Background & Motivation 1. Airflow 2 behavior: Omitting the schedule arg leaves it at the sentinel NOTSET, which Airflow treats as a daily schedule (timedelta(days=1)). Explicitly setting `schedule=None` is redundant/inaccurate and overrides intended defaults. 2. Airflow 3 behavior: Omitting the schedule arg leaves it at `None`, matching the core SDK default. The PR also fixes a bug by removing an unneeded conditional wherein when schedule arg in not passed, DAG Factory injected schedule_interval arg leaving the generated DAG incompatible with Airflow 3 Desired outcome: - DAG Factory should never inject its own default for schedule (or any other optional argument). - Let Airflow core apply the correct behaviour per its version and configuration. closes: #467
1 parent f35955f commit 5ed0b48

3 files changed

Lines changed: 76 additions & 5 deletions

File tree

dagfactory/dagbuilder.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -817,7 +817,11 @@ def configure_schedule(dag_params: Dict[str, Any], dag_kwargs: Dict[str, Any]) -
817817
if DagBuilder._is_asset(schedule):
818818
dag_kwargs["schedule"] = DagBuilder._asset_schedule(schedule)
819819
else:
820-
if isinstance(dag_params["schedule"], str) and dag_params["schedule"].lower() == "none":
820+
if (
821+
utils.check_dict_key(dag_params, "schedule")
822+
and isinstance(dag_params["schedule"], str)
823+
and dag_params["schedule"].strip().lower() == "none"
824+
):
821825
dag_kwargs["schedule"] = None
822826
else:
823827
dag_kwargs["schedule"] = schedule
@@ -921,9 +925,6 @@ def build(self) -> Dict[str, Union[str, DAG]]:
921925
if version.parse(AIRFLOW_VERSION) >= version.parse("2.9.0"):
922926
dag_kwargs["dag_display_name"] = dag_params.get("dag_display_name", dag_params["dag_id"])
923927

924-
if not dag_params.get("timetable") and not utils.check_dict_key(dag_params, "schedule"):
925-
dag_kwargs["schedule_interval"] = dag_params.get("schedule_interval", timedelta(days=1))
926-
927928
dag_kwargs["description"] = dag_params.get("description", None)
928929

929930
if "concurrency" in dag_params:
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
example_dag_no_schedule:
2+
default_args:
3+
owner: custom_owner
4+
start_date: 2 days
5+
description: this is an example dag
6+
doc_md: '##here is a doc md string'
7+
tasks:
8+
task_1:
9+
bash_command: echo 1
10+
operator: airflow.operators.bash.BashOperator
11+
task_2:
12+
bash_command: echo 2
13+
dependencies:
14+
- task_1
15+
operator: airflow.operators.bash.BashOperator
16+
task_3:
17+
bash_command: echo 3
18+
dependencies:
19+
- task_1
20+
operator: airflow.operators.bash.BashOperator
21+
22+
23+
example_dag_none_string_schedule:
24+
default_args:
25+
owner: custom_owner
26+
start_date: 2 days
27+
schedule_interval: ' none '
28+
description: this is an example dag
29+
doc_md: '##here is a doc md string'
30+
tasks:
31+
task_1:
32+
bash_command: echo 1
33+
operator: airflow.operators.bash.BashOperator
34+
task_2:
35+
bash_command: echo 2
36+
dependencies:
37+
- task_1
38+
operator: airflow.operators.bash.BashOperator
39+
task_3:
40+
bash_command: echo 3
41+
dependencies:
42+
- task_1
43+
operator: airflow.operators.bash.BashOperator

tests/test_dagfactory.py

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
from dagfactory import dagfactory, load_yaml_dags
2020

2121
TEST_DAG_FACTORY = os.path.join(here, "fixtures/dag_factory.yml")
22+
DAG_FACTORY_NO_OR_NONE_STRING_SCHEDULE = os.path.join(here, "fixtures/dag_factory_no_or_none_string_schedule.yml")
2223
INVALID_YAML = os.path.join(here, "fixtures/invalid_yaml.yml")
2324
INVALID_DAG_FACTORY = os.path.join(here, "fixtures/invalid_dag_factory.yml")
2425
DEFAULT_ARGS_CONFIG_ROOT = os.path.join(here, "fixtures/")
@@ -422,9 +423,35 @@ def test_schedule_interval():
422423
td.generate_dags(globals())
423424
if version.parse(AIRFLOW_VERSION) < version.parse("3.0.0"):
424425
schedule_interval = globals()["example_dag2"].schedule_interval
426+
expected_schedule_interval = datetime.timedelta(days=1)
425427
else:
426428
schedule_interval = globals()["example_dag2"].schedule
427-
assert schedule_interval is None
429+
expected_schedule_interval = None
430+
assert schedule_interval == expected_schedule_interval
431+
432+
433+
def test_no_schedule_supplied():
434+
td = dagfactory.DagFactory(DAG_FACTORY_NO_OR_NONE_STRING_SCHEDULE)
435+
td.generate_dags(globals())
436+
if version.parse(AIRFLOW_VERSION) < version.parse("3.0.0"):
437+
schedule_interval = globals()["example_dag_no_schedule"].schedule_interval
438+
expected_schedule_interval = datetime.timedelta(days=1)
439+
else:
440+
schedule_interval = globals()["example_dag_no_schedule"].schedule
441+
expected_schedule_interval = None
442+
assert schedule_interval == expected_schedule_interval
443+
444+
445+
def test_none_string_schedule_supplied():
446+
td = dagfactory.DagFactory(DAG_FACTORY_NO_OR_NONE_STRING_SCHEDULE)
447+
td.generate_dags(globals())
448+
if version.parse(AIRFLOW_VERSION) < version.parse("3.0.0"):
449+
schedule_interval = globals()["example_dag_none_string_schedule"].schedule_interval
450+
expected_schedule_interval = datetime.timedelta(days=1)
451+
else:
452+
schedule_interval = globals()["example_dag_none_string_schedule"].schedule
453+
expected_schedule_interval = None
454+
assert schedule_interval == expected_schedule_interval
428455

429456

430457
def test_dagfactory_dict():

0 commit comments

Comments
 (0)