-
Notifications
You must be signed in to change notification settings - Fork 17.5k
Fix DateTimeSensorAsync crashing Dag parsing with templated target_time #70310
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,9 @@ class DateTimeSensorAsync(DateTimeSensor): | |
|
|
||
| :param target_time: datetime after which the job succeeds. (templated) | ||
| :param start_from_trigger: Start the task directly from the triggerer without going into the worker. | ||
| This requires a static ``target_time`` (a datetime or ISO-8601 string). A templated | ||
| ``target_time`` is not supported here because the trigger is created at Dag-parse time, | ||
| before Jinja templates are rendered. | ||
| :param trigger_kwargs: The keyword arguments passed to the trigger when start_from_trigger is set to True | ||
| during dynamic task mapping. This argument is not used in standard usage. | ||
| :param end_from_trigger: End the task directly from the triggerer without going into the worker. | ||
|
|
@@ -125,8 +128,14 @@ def __init__( | |
|
|
||
| self.start_from_trigger = start_from_trigger | ||
| if self.start_from_trigger: | ||
| try: | ||
| moment = timezone.parse(self.target_time) | ||
| except ValueError as e: | ||
| raise ValueError( | ||
| f"start_from_trigger=True requires a static target_time, not a template: {self.target_time!r}" | ||
| ) from e | ||
| self.start_trigger_args.trigger_kwargs = dict( | ||
| moment=timezone.parse(self.target_time), | ||
| moment=moment, | ||
|
Comment on lines
+131
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does it solve the crashing? It seems to improve the error message rather than fix the issue.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shahar1 - I think the goal was to replicate something like this: https://github.com/apache/airflow/pull/69610/changes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point. As is, this just turns a cryptic ParserError into a clearer ValueError, the Dag file still fails to parse. And yes, the intent was to mirror #69610 The difference is that for TimeSensor, start_from_trigger could never work, since it computes "today at target_time" from the wall clock at parse time. Here target_time is a template field, so there are I think two ways to go about it:
|
||
| end_from_trigger=self.end_from_trigger, | ||
| ) | ||
|
|
||
|
|
||
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.
The trigger is not created at Dag-parse time, but either in the worker or the triggerer
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.
Yes you're right, I overlooked that. Will reword the docstring.