Skip to content

Fix TimeSensor dag_version churn via TimeOfDayTrigger - #69746

Closed
Vamsi-klu wants to merge 7 commits into
apache:mainfrom
Vamsi-klu:fix/time-sensor-dag-version-69543
Closed

Vamsi-klu wants to merge 7 commits into
apache:mainfrom
Vamsi-klu:fix/time-sensor-dag-version-69543

Conversation

@Vamsi-klu

@Vamsi-klu Vamsi-klu commented Jul 11, 2026 •

Copy link
Copy Markdown

What is the change?

TimeSensor(start_from_trigger=True) now starts through a new TimeOfDayTrigger that stores only the parse-stable target_time and tz and resolves the concrete UTC moment when the trigger starts. TimeSensor.__init__ builds a per-instance StartTriggerArgs instead of mutating the class attribute.

Why did I do it?

Two defects. __init__ mutated the shared class-level StartTriggerArgs, so every sensor advertised the last-constructed sensor's moment. And that moment came from datetime.now() at parse time, so each Dag-processor parse produced a different serialized blob and a new DagVersion.

Fixes: #69543

How did I do it?

I added TimeOfDayTrigger plus resolve_time_of_day_moment and serializable_timezone helpers in triggers/temporal.py. Spring-forward gaps shift forward, ambiguous fall-back times use fold=0, and serialize() persists the resolved moment so a reconstruct after midnight does not move the wait target. execute() and poke() resolve the moment once per attempt and cache it. A sensor with no Dag falls back to UTC when poking and raises for start_from_trigger.

What's the impact?

No more dag_version churn from repeated parses. The start_from_trigger API is preserved, so no major bump of apache-airflow-providers-standard. trigger_kwargs stays accepted and ignored.

What's the test plan?

pytest providers/standard/tests/unit/standard/sensors/test_time.py -q and pytest providers/standard/tests/unit/standard/triggers/test_temporal.py -q. New tests cover serialization stability across mocked now(), midnight caching, DST spring and fall, fixed and named timezones, shared-class isolation, and end_from_trigger propagation.

@Vamsi-klu

This comment was marked as spam.

potiuk
potiuk previously requested changes Jul 30, 2026

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — the underlying diagnosis is right and worth fixing: baking target_datetime into start_trigger_args at parse time means the serialized Dag hash changes every day, and the hash-stability test you added is a good way to pin that.

But the fix is to remove start-from-trigger support from TimeSensor, and that's a bigger call than the PR framing suggests. Anyone using TimeSensor(start_from_trigger=True) today has their sensor start directly in the Triggerer; after this it warns, is ignored, and the task instead occupies a worker slot to poke or defer. That's a functional regression for those users, not just churn cleanup.

Concretely that needs: a major version bump on apache-airflow-providers-standard (currently 1.16.0) and an entry under the Changelog header in providers/standard/docs/changelog.rst spelling out the required user action. Neither is in the diff. (No newsfragment — providers don't use those.)

The design question I'd like your view on: was removal the only option? Computing moment lazily — so start_trigger_args serializes a stable placeholder and the concrete datetime is resolved when the trigger actually starts — would fix the hash churn while keeping the feature. If that was tried and doesn't work, saying why in the description would help; if it wasn't, it seems worth a look before dropping a documented capability.

One correction to something I'd have flagged: trigger_kwargs is already accepted-and-ignored on main (it's declared in __init__ but never read), so dropping it here changes nothing. Not an issue with this PR.


To be clear about where this stands: the points above are mostly design questions rather than defects, and I'd rather you formed your own view on them than took mine as settled. My review here was AI-assisted, and I'd guess parts of this PR were too — that's fine on both sides, but it means neither of us should treat the output as authoritative. Please push back where you think the reasoning is wrong, and say what you think the right trade-off is between fixing the hash churn and keeping the feature.

I'd also like other maintainers to weigh in before this is decided. Removing a documented capability from a released provider isn't a call I want to make on my own, and folks closer to the start-from-trigger design will have better context here than I do.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

Comment thread providers/standard/src/airflow/providers/standard/sensors/time.py Outdated
Comment thread providers/standard/src/airflow/providers/standard/sensors/time.py
Comment thread providers/standard/src/airflow/providers/standard/sensors/time.py Outdated
@Vamsi-klu

This comment was marked as spam.

@Vamsi-klu

This comment was marked as spam.

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This and #69925 fix the same thing in different ways. We should first figure out the correct approach in the issue.

Vamsi-klu and others added 4 commits August 24, 2026 00:19
TimeSensor(start_from_trigger=True) had two defects:

1. __init__ mutated the class-level StartTriggerArgs.trigger_kwargs, so
   every TimeSensor shared the last-constructed sensor's moment.
2. That moment was an absolute datetime from datetime.now() at parse
   time, so each Dag-processor parse produced a different serialized
   blob and a new DagVersion.

Preserve start_from_trigger by introducing TimeOfDayTrigger, which
stores only parse-stable target_time + timezone and resolves the
concrete moment when the trigger starts. TimeSensor builds a
per-instance StartTriggerArgs (never mutates the class attribute).

No major version bump: the public API is preserved.

Fixes: apache#69543
The temporal trigger docstring broke the generated API docs page (bullet
list without a preceding blank line). Pendulum's in_timezone rejects a
generic tzinfo in its type stubs, so normalize to a pendulum zone at the
boundary. The serialization-stability tests used LazyDeserializedDAG.from_dag,
which does not exist on Airflow 2.11/3.0-3.2 - pick the serialization API
available on the running version instead.
A class-level TimeOfDayTrigger template leaked dummy kwargs into every
TimeSensor serialization, and start_from_trigger silently wrote UTC when
no Dag was attached, hiding a missing timezone. The trigger parameter
is named tz so it does not shadow the SDK timezone module.
Delegating to a fresh DateTimeTrigger and omitting the resolved moment
from serialize() meant a reconstruct after midnight could wait another
day. Resolve at construction and persist moment so the wait target
does not move.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor
cursor Bot force-pushed the fix/time-sensor-dag-version-69543 branch from a604a90 to 92fee51 Compare August 24, 2026 00:19
@potiuk
potiuk dismissed their stale review August 29, 2026 13:35

I see there is another discussion relaated to how to solve it . I dismiss my review to unblock it.

@Vamsi-klu

This comment was marked as spam.

Comment thread providers/standard/src/airflow/providers/standard/sensors/time.py Outdated

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Although we could import the timezone encode/decode logic from SDK, I think it’s probably not be worst idea to have them directly inside the provider code. The important thing here is both sides must agree, and having them both in the same place is more important for the provider than reducing duplication from Airflow Core or SDK.

Only one minor suggestion on using replace instead of the class directly.

deepinsight coder and others added 3 commits September 7, 2026 05:13
The class-level template is the parse-stable source, matching DateTimeSensor
and the previous code on main. Rebuilding StartTriggerArgs by hand dropped
that shared object and made two sensors look like they constructed args from
scratch.
…-version-69543

Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
kaxil added a commit to astronomer/airflow that referenced this pull request Sep 18, 2026
DayOfWeekSensor's xref in the standard provider's sensor guide pointed at
the pre-provider-split airflow.sensors.weekday path, which no longer
exists; the class now lives under airflow.providers.standard.sensors.weekday.

BaseXCom's xref in the common.io XCom backend guide pointed at
airflow.models.xcom.BaseXCom, which is a deprecated compat shim; the real
class lives at airflow.sdk.bases.xcom.BaseXCom.

The TimeSensor xref in the same sensor guide is also broken (points at a
non-existent sensors.time_sensor module) but is left alone here since apache#69610,
apache#69746, and apache#69925 are all open against that same paragraph.
kaxil added a commit to astronomer/airflow that referenced this pull request Sep 18, 2026
DayOfWeekSensor's xref in the standard provider's sensor guide pointed at
the pre-provider-split airflow.sensors.weekday path, which no longer
exists; the class now lives under airflow.providers.standard.sensors.weekday.

BaseXCom's xref in the common.io XCom backend guide pointed at
airflow.models.xcom.BaseXCom, a deprecated compat shim. The task-sdk docs
only expose it at the flat airflow.sdk.BaseXCom path (autoapiclass in
task-sdk/docs/api.rst, autoapi_generate_api_docs=False elsewhere), matching
the existing convention for this class of xref (e.g. airflow.sdk.ResumableJobMixin).

The TimeSensor xref in the same sensor guide is also broken (points at a
non-existent sensors.time_sensor module) but is left alone here since apache#69610,
apache#69746, and apache#69925 are all open against that same paragraph.
kaxil added a commit that referenced this pull request Sep 19, 2026
DayOfWeekSensor's xref in the standard provider's sensor guide pointed at
the pre-provider-split airflow.sensors.weekday path, which no longer
exists; the class now lives under airflow.providers.standard.sensors.weekday.

BaseXCom's xref in the common.io XCom backend guide pointed at
airflow.models.xcom.BaseXCom, a deprecated compat shim. The task-sdk docs
only expose it at the flat airflow.sdk.BaseXCom path (autoapiclass in
task-sdk/docs/api.rst, autoapi_generate_api_docs=False elsewhere), matching
the existing convention for this class of xref (e.g. airflow.sdk.ResumableJobMixin).

The TimeSensor xref in the same sensor guide is also broken (points at a
non-existent sensors.time_sensor module) but is left alone here since #69610,
#69746, and #69925 are all open against that same paragraph.
@shahar1 shahar1 closed this Sep 21, 2026
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.

dag_version inflection when using TimeSensor with start_from_trigger = True

5 participants