Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor DAG.create_dagrun() arguments #45370

Merged
merged 24 commits into from
Jan 15, 2025

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Jan 3, 2025

This aims to make the interface more straightforward, removing auto inference logic from the function. Most importantly, it is now entirely the caller site's responsibility to provide valid run_id and run_type values, instead of the function automatically inferring one from the other under certain conditions.

The main goal is to make changes simpler when we make logical date an optional (nullable) value. run_id generation is currently very heavily based on the logical date, and will need to be changed a bit when logical date is None. Removing logic should help us change the run_id generation logic easier.

p.s. not yet ready, many tests should fail. Finally ready; this took way much more than I thought.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:dev-tools area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues kind:documentation labels Jan 3, 2025
@uranusjr uranusjr added the legacy api Whether legacy API changes should be allowed in PR label Jan 3, 2025
@uranusjr uranusjr force-pushed the create-dagrun-refactor branch 6 times, most recently from 34a7033 to 5392e2b Compare January 8, 2025 05:55
@uranusjr uranusjr marked this pull request as ready for review January 9, 2025 05:49
@uranusjr uranusjr force-pushed the create-dagrun-refactor branch from 296f247 to 09e98d2 Compare January 14, 2025 07:28
airflow/models/dag.py Show resolved Hide resolved
docs/apache-airflow/best-practices.rst Outdated Show resolved Hide resolved
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. Do we need to consider it as breaking change?

This aims to make the interface more straightforward, removing auto
inference logic from the function. Most importantly, it is now entirely
the caller site's responsibility to provide valid run_id and run_type
values, instead of the function automatically inferring one from the
other under certain conditions.

The main goal is to make changes simpler when we make logical date an
optional (nullable) value. run_id generation is currently very heavily
based on the logical date, and will need to be changed a bit when
logical date is None. Removing logic should help us change the run_id
generation logic easier.
We can have dag_version and run_type defaults since this is just for
tests and the actual values aren't that important most of the times.
@uranusjr uranusjr force-pushed the create-dagrun-refactor branch from acf0a03 to 0556951 Compare January 15, 2025 07:45
@uranusjr
Copy link
Member Author

re: Breaking change, I don’t think we need to since create_dagrun is considered private interface. If anyone is using it, the breakage is on them.

@uranusjr uranusjr merged commit 02d83b0 into apache:main Jan 15, 2025
91 checks passed
@uranusjr uranusjr deleted the create-dagrun-refactor branch January 15, 2025 12:08
HariGS-DB pushed a commit to HariGS-DB/airflow that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:CLI area:dev-tools area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues kind:documentation legacy api Whether legacy API changes should be allowed in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants