Skip to content

Conversation

@ashb
Copy link
Member

@ashb ashb commented Apr 22, 2025

Comparing to now means that if you clear a dagrun and it re-runs it will
always show up as latest!

Additionally remove the check/don't skip logic whe the data interval is
zero-wdith. Even if a DAG doesn't have the concept of a data-interval (i.e. it
is zero width) it still is logically consistent for it to have to concept of
latest or not, so we now only compare against the end date of the interval.

(And a few drive-by refactors too, context["task"] is self,
context["dag"] is self.dag)

Reported by @jscheffl here #48945 (comment)


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

Comparing to now means that if you clear a dagrun and it re-runs it will
always show up as latest!

Additionally remove the check/don't skip logic whe the data interval is
zero-wdith. Even if a DAG doesn't have the concept of a data-interval (i.e. it
is zero width) it still is logically consistent for it to have to concept of
latest or not, so we now only compare against the end date of the interval.

(And a few drive-by refactors too, `context["task"]` is `self`,
`context["dag"]` is `self.dag`)
return list(context["task"].get_direct_relative_ids(upstream=False))
return list(self.get_direct_relative_ids(upstream=False))

if not left_window < now <= right_window:
Copy link
Member

Choose a reason for hiding this comment

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

The change looks correct, but I have no idea why now was ever used

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it has been such since last 9 years.. So we should double-verify around the expectation. Maybe the test might help with that:;

edf033b

Copy link
Member

Choose a reason for hiding this comment

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

Some discussions in the original PR: #1752

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Dear. God.

Has this operator ever actually been useful? Is this change now actually a breaking change then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually convinced the discussion in the original PR is right. I think Sid is confusing dag.start_date with dag_run.start_date or maybe task_instance.start_date.

Either way, I'll park this conversation.

@ashb
Copy link
Member Author

ashb commented Apr 22, 2025

I'm going to close this -- the switch away from now is a bigger change; and re-open it with just correct behaviour for the new default "interval-less" timetables on v3

@ashb ashb closed this Apr 22, 2025
@ashb ashb deleted the latest-only-dagrun-compare-to-dagrun-date-not-now branch April 22, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants