kaxil commented on code in PR #68837:
URL: https://github.com/apache/airflow/pull/68837#discussion_r3711266263
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -1293,7 +1293,8 @@ def test(
log.debug("Getting dagrun for dag %s", self.dag_id)
logical_date = timezone.coerce_datetime(logical_date)
- run_after = timezone.coerce_datetime(run_after) or
timezone.coerce_datetime(timezone.utcnow())
+ run_start_date = timezone.coerce_datetime(timezone.utcnow())
Review Comment:
This is captured before the bundle sync below, which does a full
`BundleDagBag` parse of the dags folder, so `run_duration` still carries the
parse time. I measured 0.29s of skew on a bundle with two DAGs, and it scales
with the dags folder. Since accurate duration is the whole point here, would
capturing `utcnow()` at the `get_or_create_dagrun` call be better, and leaving
`run_after` with its own default?
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -1361,7 +1362,7 @@ def test(
dr: DagRun = get_or_create_dagrun(
dag=scheduler_dag,
- start_date=logical_date or run_after,
+ start_date=run_start_date,
Review Comment:
`get_or_create_dagrun` still falls back to [`start_date or
logical_date`](https://github.com/apache/airflow/blob/b99aa145c4/airflow-core/src/airflow/models/dagrun.py#L2563),
and its other caller passes exactly the [`start_date=logical_date or
run_after`](https://github.com/apache/airflow/blob/b99aa145c4/airflow-core/src/airflow/cli/commands/task_command.py#L157)
you're removing here. `airflow tasks test` deletes that run in its `finally`,
so nobody sees an inflated duration on that path, but the fallback is what let
this happen in the first place. Would defaulting `start_date` to
`timezone.utcnow()` inside `get_or_create_dagrun` and dropping the `or
logical_date` be a better place to fix it, so neither caller can anchor a run
to a logical date?
##########
task-sdk/tests/task_sdk/definitions/test_dag.py:
##########
@@ -490,6 +491,39 @@ def execute(self, context):
with pytest.raises(FailFastDagInvalidTriggerRule):
fail_fast_dag.add_task(task_with_non_default_trigger_rule)
+ def test_dag_test_runtime_start_date_decoupled_from_logical_date(self,
time_machine):
+ """
+ Ensure DAG.test decouples its execution start_date from historical
logical_dates
+ by isolating the runtime creation point from background metadata
dependencies.
+ """
+ # 1. Setup anchor dates using Pendulum
+ past_logical_date = pendulum.datetime(2024, 1, 1, tz="UTC")
+ frozen_now = pendulum.datetime(2026, 6, 22, 12, 0, 0, tz="UTC")
+
+ time_machine.move_to(frozen_now, tick=False)
+
+ # 2. Instantiate minimal DAG context
+ with DAG(dag_id="test_runtime_duration_isolation",
start_date=past_logical_date) as dag:
+ pass
+
+ # 3. Suppress all core metadata lookups by isolating the active
execution scope
+ with (
+ mock.patch("airflow.models.dagrun.get_or_create_dagrun") as
mock_create_run,
Review Comment:
Five patches into airflow-core internals to cover two lines is a lot of
surface, and the test breaks if any of those import paths move.
[`test_dag_test_with_custom_timetable`](https://github.com/apache/airflow/blob/b99aa145c4/airflow-core/tests/unit/cli/commands/test_dag_command.py#L993)
makes the same shape of assertion with a single patch on
`get_or_create_dagrun`, and `airflow-core/tests/unit/models/test_dag.py` calls
`dr = dag.test()` against a real DB where `dr.start_date` can be asserted
directly (I tried that version: `run_duration` comes out at ~2.5s with
`logical_date=2024-01-01`). Any reason to add this under task-sdk rather than
extend one of those?
--
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]
