Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

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.
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. If target_time is not parseable at parse time, log a warning and fall back to deferring from the worker. The template renders as usual and the Dag parses fine on all supported versions.
  2. On Airflow 3.3+, pass the raw target_time string through trigger_kwargs and let the triggerer render it before the trigger runs. Since Re-enable start_from_trigger feature with rendering of template fields #55068 the triggerer renders any trigger kwarg whose name matches an operator template field, which FileSensor already relies on for filepath. That would make start_from_trigger actually work with a templated target_time, but it means extending DateTimeTrigger to accept a target_time kwarg and gating on the Airflow version, falling back to (1) on older versions.

end_from_trigger=self.end_from_trigger,
)

Expand Down
37 changes: 36 additions & 1 deletion providers/standard/tests/unit/standard/sensors/test_date_time.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

from airflow import macros
from airflow.models.dag import DAG
from airflow.providers.standard.sensors.date_time import DateTimeSensor
from airflow.providers.standard.sensors.date_time import DateTimeSensor, DateTimeSensorAsync

from tests_common.test_utils.version_compat import timezone

Expand Down Expand Up @@ -124,3 +124,38 @@ def test_moment(self, native, target_time, expected_type):
sensor.render_template_fields(ctx)

assert isinstance(sensor._moment, expected_type)


class TestDateTimeSensorAsync:
@pytest.mark.parametrize(
"target_time",
[
pendulum.datetime(2020, 7, 6, 13, tz="UTC"),
"2020-07-06T13:00:00+00:00",
],
)
def test_start_from_trigger_with_static_target_time(self, target_time):
with DAG(
dag_id="test_static_target_time",
schedule=None,
start_date=pendulum.datetime(2020, 1, 1, tz="UTC"),
):
op = DateTimeSensorAsync(task_id="test", target_time=target_time, start_from_trigger=True)

assert op.start_trigger_args.trigger_kwargs == {
"moment": pendulum.datetime(2020, 7, 6, 13, tz="UTC"),
"end_from_trigger": False,
}

def test_start_from_trigger_with_templated_target_time_raises(self):
with DAG(
dag_id="test_templated_target_time",
schedule=None,
start_date=pendulum.datetime(2020, 1, 1, tz="UTC"),
):
with pytest.raises(ValueError, match="requires a static target_time"):
DateTimeSensorAsync(
task_id="test",
target_time="{{ data_interval_end.tomorrow().replace(hour=1) }}",
start_from_trigger=True,
)
Loading