Simplify DAG kwargs construction with a declarative spec#703
Open
gyli wants to merge 4 commits intoastronomer:mainfrom
Open
Simplify DAG kwargs construction with a declarative spec#703gyli wants to merge 4 commits intoastronomer:mainfrom
gyli wants to merge 4 commits intoastronomer:mainfrom
Conversation
gyli
requested review from
pankajkoti and
tatiana
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #587 by refactoring how DagBuilder constructs kwargs for the Airflow DAG constructor: it replaces imperative/default-setting logic with a declarative parameter spec so only explicitly configured DAG params are forwarded and Airflow’s own defaults remain in effect when values are not set.
Changes:
- Introduces a module-level
_DAG_PARAM_SPECand a spec-drivenDagBuilder._build_dag_kwargs()helper to constructdag_kwargs. - Removes DAG-kwarg defaulting previously sourced from Airflow config (e.g.,
catchup,max_active_runs,default_view), relying on Airflow constructor behavior when keys are absent. - Adds unit tests covering required params, deprecation mapping, Airflow version gating, and optional value transforms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
dagfactory/dagbuilder.py |
Adds declarative DAG-kwarg spec and _build_dag_kwargs(), and wires build() to use it instead of setting defaults directly. |
tests/test_dagbuilder.py |
Adds focused tests for _build_dag_kwargs() behaviors (presence-only forwarding, deprecations, version gates, transforms). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…LOW_VERSION The warning messages in build_dag_kwargs were using the AIRFLOW_VERSION string constant instead of INSTALLED_AIRFLOW_VERSION, which is the actual parsed Version used for comparison. This made diagnostics inaccurate when INSTALLED_AIRFLOW_VERSION is patched in tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tory into simplify-dag-kwargs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #587
Key changes:
Future works: task-level spec will be added in the follow-up PR