-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add support for catchup="ignore_first" to disable catchup for the first DagRun #35392
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
Add support for catchup="ignore_first" to disable catchup for the first DagRun #35392
Conversation
|
+1 for this👍 However, instead of adding yet another setting, I think this should be the default behavior of Airflow. I have yet to find the first user that finds it logical to get one DAG run when unpausing a DAG with catchup=False. That would mean a breaking change and thus release in Airflow 3, so I was thinking this could be offered as a stop-gap solution for Airflow >=2.8,<3 users, but with a deprecation flag added to it? |
|
I think we bump into the boolean trap here, seems like logic around So my proposal, if it possible to try get rid of boolean logic for the catchup, and replace it by Enum or string literal with boolean fallback logic, and deprecation warning about Some simple enum class Catchup(str, Enum):
ENABLED = "enabled"
DISABLED = "disabled"
IGNORE_FIRST = "ignore_first"
@classmethod
def from_string(cls, value: str | Enum):
values = cls.__members__.values()
for v in values:
if v.value == value:
return v
msg = (
f"Unsupported Enum value for {cls!r}. Expected one of {', '.join(repr(v.value) for v in values)}, "
f"but got {value!r}."
)
raise ValueError(msg)
def __eq__(self, other):
if isinstance(other, str):
other = Catchup.from_string(other)
elif isinstance(other, bool):
return bool(self) is other
return self == other
def __bool__(self):
# Deprecation warning here
return self != self.DISABLEDHowever it still might break someone pipeline (I'm not sure) and we need to wait until Airflow 3 for use something different hen boolean here, and for now just add new attribute. Same is valid for already existed |
|
@BasPH @Taragolis I merged the new param in catchup, and I kept the bool values for both provided and default values supported with a deprecation warning, but I had to update the value in all the examples to avoid the warning in the CI and to invite the users to use the new values. |
| $ref: '#/components/schemas/Timezone' | ||
| catchup: | ||
| type: boolean | ||
| type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pierrejeambrun, is this considered a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should still keep True and False as accepted values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DAGDetail is used as a response type for the endpoint /dags/{dag_id}/details, IMHO it's better to return one of the supported catchup values instead of a boolean even if the user uses a boolean in its dag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally changing the type of one field would be breaking. (Either in response or request).
Here, if True False are still accepted value, it should be fine.
We would need to confirm in the client what happens if we pass a Boolean object to a string field. Does it get silently cast to string or does it crash I am not sure. That could break there, appart from that I don’t see an issue (I don’t know if the API client tries type casting in case types are wrong, or if it breaks right away)
It runs immediately because of I do agree that any place of confusion is something that we should handle and simply but this specific pain feels out of scope for this PR. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
What is the proper |
|
This would be great to have |
What I have been having to do is update the start_date to a future date and then turn on the DAG, this is very inconvenient and I would surmise almost impossible for people with many dags. |
|
No more new configs for this please. We already have config sprawl of dag behaviours If this is being added to core the correct way of doing this is i feel a custom/new timetable |
|
Can we just decide on the correct behavior and fix it. We make too many things configurable in Airflow. |
|
Closing this, it's been inactive for a year too, plus consensus seems to be no new config option specifically |
|
Is there an issue to actually resolve what the desired behavior is? I get closing PRs if there is not movement being made on it because of a blocker on UX design, but the problem that this PR is attempting to address remains. |
I'm not sure. You could look in issues or discussions, and if none found, create one for it, and possibly try to engage to help as come to something approaching a consensus on what to do. Airflow 3 is the time to fix things like this (assuming fix is needed) so we should get moving on it if it's to be resolved in 3.0. |
This PR updates the type of catchup to string and adds support for the
ignore_firstvalue, to ignore catchup only for the first DagRub.Motivations:
The
start_dateis a mandatory parameter, andcatchup=Trueis very useful to avoid Airflow skip creating some dag run when there are some pressures on its services, there is a downtime, or the duration of some dag run is greater than the schedule interval, and max_active_runs is set to 1.When creating dags dynamically from a file/API response, setting catchup to True will create all the dag runs since the start_date, and setting start_date equal to the current moment is a very bad practice and can lead to missing DagRuns.
The new test will only succeed with #35391, which fixes the bug in the data interval date for the dag runs.