Vamsi-klu commented on PR #69746:
URL: https://github.com/apache/airflow/pull/69746#issuecomment-5140602825
@potiuk Thanks for the careful review — you're right that removing
start_from_trigger was the wrong trade-off. I've reworked this to keep the
feature.
Two defects (not one)
---------------------
Worth leading with the shared-class bug, because it is independent of the
API discussion:
1. start_trigger_args is a class-level StartTriggerArgs. __init__ did
self.start_trigger_args.trigger_kwargs = {...}, which mutates the *shared class
object*. Every TimeSensor ended up with the last-constructed sensor's moment.
That is a real bug regardless of whether we keep start_from_trigger.
2. The mutated trigger_kwargs["moment"] was an absolute datetime from
datetime.now() at parse time, so every Dag-processor parse produced a different
serialized blob and a new DagVersion.
Design: TimeOfDayTrigger (API preserved, no major bump)
-------------------------------------------------------
I took the lazy-moment path you suggested rather than dropping the feature.
- New TimeOfDayTrigger stores only parse-stable kwargs: target_time (ISO
string — datetime.time is not serde-safe), timezone (IANA name or fixed offset
seconds), end_from_trigger.
- Concrete moment is resolved when the trigger *starts*, then the wait loop
is delegated to DateTimeTrigger (no duplicated sleep logic).
- TimeSensor.__init__ keeps start_from_trigger in the explicit signature (so
inspect/IDE/docs still see it — addresses your kwargs.pop note).
- When start_from_trigger=True it builds a *per-instance* StartTriggerArgs
and never mutates the class attribute (fixes defect 1).
- execute()/poke() resolve once per attempt and cache (no midnight recompute
drift). target_datetime is documented as cached-on-first-access for the life of
the instance.
- trigger_kwargs stays accepted-and-ignored, matching main (agreed — not a
real change).
No major bump: public API is preserved, so this is a minor bugfix. Providers
don't use newsfragments; no breaking-change Changelog entry under the Changelog
header.
Inline notes
------------
- Signature: start_from_trigger is back in __init__ (not kwargs.pop).
- target_datetime property: docstring now states it is resolved once and
cached for the attempt; value semantics are "today at first access", not
"recompute every read".
- Dag timezone: uses self._dag (no bare except Exception). Falls back to UTC
only when the operator is not yet attached to a Dag (unit tests /
construct-before-add_task). When a Dag is present, its timezone is used.
DST
---
- Spring-forward gap (e.g. 02:30 America/New_York on 2024-03-10): shift
forward to next valid local time (03:30).
- Fall-back ambiguous: fold=0 (first occurrence).
Tests (39 passed)
-----------------
Headline: serialize the same Dag twice with datetime.now() mocked to two
different values → LazyDeserializedDAG hash and to_dict() identical.
Also: already-passed today, midnight cache, DST spring/fall, no-Dag UTC
fallback, TimeSensorAsync, FixedTimezone + named tz round-trip, two sensors
don't share start_trigger_args, end_from_trigger on both start_from_trigger and
defer paths, start_from_trigger still in inspect.signature.
Happy to adjust further if other maintainers want a different DST policy or
want TimeOfDayTrigger to live elsewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
