Performance: Eager load dag_model in fetch_task_instances and optimize DagUnpausedDep#70051
Performance: Eager load dag_model in fetch_task_instances and optimize DagUnpausedDep#70051rajat315315 wants to merge 2 commits into
Conversation
|
Benchmarking file attached. |
SameerMesiah97
left a comment
There was a problem hiding this comment.
The CI failures might have revealed a flaw in this implementation. If you look at this error:
AttributeError: 'NoneType' object has no attribute 'is_paused'
It looks like this change assumes ti.dag_model is always present, whereas previously _is_dag_paused() only relied on dag_id. Is there a reason we can guarantee the relationship is never None here, or does the dependency need to handle the missing relationship gracefully? Is it possible there are execution paths where ti.dag_model hasn't been eagerly loaded (or isn't present), making this optimization unsafe?
Theoretically, this change should improve performance. But I think the implemenation needs to be refined.
|
If |
Description
Motivation
Addresses issue #70050 .
Eliminates the N+1 database queries storm when checking if DAGs are paused (
DagUnpausedDep) in the scheduling loop. By utilizing SQLAlchemyjoinedloadforTI.dag_modelwhen fetching task instances for a DAG run, the pause check can be resolved entirely in memory.Proposed Changes
DagRun.fetch_task_instancesinairflow/models/dagrun.pyto addjoinedload(TI.dag_model)to theselect(TI)statement.DagUnpausedDep._get_dep_statusesinairflow/ti_deps/deps/dag_unpaused_dep.pyto useti.dag_model.is_pausedinstead of running_is_dag_paused()database selects.Verification
Ran benchmark tests on SQLite simulating a scheduler batch evaluation of 1,000 task instances:
airflow-core/tests/unit/models/test_dagrun.py.Was generative AI tooling used to co-author this PR?
Generated-by: Gemini Pro 3.1 following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.