You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jroachgolf84 (via GitHub)" <gi...@apache.org> on 2025/05/20 21:09:48 UTC
[PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
jroachgolf84 opened a new pull request, #50864:
URL: https://github.com/apache/airflow/pull/50864
Per #50812, margin the `TimeSensorAsync` Sensor with `TimeSensor`, and adding deferrable mode.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2099452302
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -96,36 +71,40 @@ class TimeSensorAsync(BaseSensorOperator):
)
start_from_trigger = False
- def __init__(
- self,
- *,
- target_time: datetime.time,
- start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
- end_from_trigger: bool = False,
- **kwargs,
- ) -> None:
+ def __init__(self, *, target_time: datetime.time, deferrable: bool = False, **kwargs) -> None:
super().__init__(**kwargs)
- self.start_from_trigger = start_from_trigger
- self.end_from_trigger = end_from_trigger
- self.target_time = target_time
+ # Create a "date-aware" timestamp that will be used as the "target_datetime". This is a requirement
+ # of the DateTimeTrigger
aware_time = timezone.coerce_datetime(
- datetime.datetime.combine(datetime.datetime.today(), self.target_time, self.dag.timezone)
+ datetime.datetime.combine(datetime.datetime.today(), target_time, self.dag.timezone)
)
+ # Now that the DAG's timezone has made the datetime timezone aware, we need to convert to UTC
Review Comment:
```suggestion
# Now that the dag's timezone has made the datetime timezone aware, we need to convert to UTC
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2100589211
##########
airflow-core/src/airflow/sensors/__init__.py:
##########
@@ -40,7 +40,6 @@
},
"time_sensor": {
"TimeSensor": "airflow.providers.standard.sensors.time.TimeSensor",
- "TimeSensorAsync": "airflow.providers.standard.sensors.time.TimeSensorAsync",
Review Comment:
Solved with https://github.com/apache/airflow/pull/50864/commits/2303d7d4adcaf4077ec191475a2b8aa996d7a191 by maintaining backwards-compat
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2100120888
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -96,36 +71,40 @@ class TimeSensorAsync(BaseSensorOperator):
)
start_from_trigger = False
- def __init__(
- self,
- *,
- target_time: datetime.time,
- start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
- end_from_trigger: bool = False,
- **kwargs,
- ) -> None:
+ def __init__(self, *, target_time: datetime.time, deferrable: bool = False, **kwargs) -> None:
Review Comment:
My thought here was to make this execution-agnostic (between sync and async). Happy to put these back in if you feel differently.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2101513336
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
Review Comment:
Got it, I didn't realize that dynamic task mapping would leverage `trigger_kwargs`. I will add that back!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2101522794
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
Review Comment:
It's somewhat new and not often used, haha. it's quite normal not knowing it
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2099447278
##########
airflow-core/docs/authoring-and-scheduling/deferring.rst:
##########
@@ -36,14 +36,14 @@ You can either use pre-written deferrable operators as a DAG author or write you
Using Deferrable Operators
--------------------------
-If you want to use pre-written deferrable operators that come with Airflow, such as ``TimeSensorAsync``, then you only need to complete two steps:
+If you want to use pre-written deferrable operators that come with Airflow, such as ``TimeSensor``, then you only need to complete two steps:
* Ensure your Airflow installation runs at least one ``triggerer`` process, as well as the normal ``scheduler``
* Use deferrable operators/sensors in your dags
Airflow automatically handles and implements the deferral processes for you.
-If you're upgrading existing dags to use deferrable operators, Airflow contains API-compatible sensor variants, like ``TimeSensorAsync`` for ``TimeSensor``. Add these variants into your DAG to use deferrable operators with no other changes required.
+If you're upgrading existing dags to use deferrable operators, Airflow contains API-compatible sensor variants. Add these variants into your DAG to use deferrable operators with no other changes required.
Review Comment:
```suggestion
If you're upgrading existing dags to use deferrable operators, Airflow contains API-compatible sensor variants. Add these variants into your dag to use deferrable operators with no other changes required.
```
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -96,36 +71,40 @@ class TimeSensorAsync(BaseSensorOperator):
)
start_from_trigger = False
- def __init__(
- self,
- *,
- target_time: datetime.time,
- start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
- end_from_trigger: bool = False,
- **kwargs,
- ) -> None:
+ def __init__(self, *, target_time: datetime.time, deferrable: bool = False, **kwargs) -> None:
Review Comment:
do we really need to remove `end_from_trigger` and `start_from_trigger`, `trigger_kwargs` here?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2099522705
##########
airflow-core/src/airflow/sensors/__init__.py:
##########
@@ -40,7 +40,6 @@
},
"time_sensor": {
"TimeSensor": "airflow.providers.standard.sensors.time.TimeSensor",
- "TimeSensorAsync": "airflow.providers.standard.sensors.time.TimeSensorAsync",
Review Comment:
Not sure if we can do that?
Maybe we should do
`"TimeSensorAsync": "airflow.providers.standard.sensors.time. TimeSensor",
`
and set min version of standard provider in Airflow-core for the next standard provider version?
cc @kaxil WDYT?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2100535837
##########
airflow-core/src/airflow/sensors/__init__.py:
##########
@@ -40,7 +40,6 @@
},
"time_sensor": {
"TimeSensor": "airflow.providers.standard.sensors.time.TimeSensor",
- "TimeSensorAsync": "airflow.providers.standard.sensors.time.TimeSensorAsync",
Review Comment:
Where would that minimum version be set?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #50864:
URL: https://github.com/apache/airflow/pull/50864#issuecomment-2920159005
If the https://github.com/apache/airflow/pull/51192 is merged + cherry-picked to 3.0.2 -> we should also cherry-pick this one to get latest standard provider examples.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on PR #50864:
URL: https://github.com/apache/airflow/pull/50864#issuecomment-2896443548
@eladkal, what do we want to do here with this one failing test? This is for a `TimeDeltaSensorAsync` test, which I believe is going to be refactored in a similar way to this (per #50811). Otherwise, this is ready for review.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2101512836
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
Review Comment:
Where is the pre-commit not working?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on PR #50864:
URL: https://github.com/apache/airflow/pull/50864#issuecomment-2905064562
cc: @eladkal , @Lee-W , do you mind taking a look at the updated PR?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2100123672
##########
airflow-core/src/airflow/sensors/__init__.py:
##########
@@ -40,7 +40,6 @@
},
"time_sensor": {
"TimeSensor": "airflow.providers.standard.sensors.time.TimeSensor",
- "TimeSensorAsync": "airflow.providers.standard.sensors.time.TimeSensorAsync",
Review Comment:
Thank you both; would the suggested change just be to add this line back and update it to be:
```
"TimeSensorAsync": "airflow.providers.standard.sensors.time.TimeSensor"
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2101476498
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
Review Comment:
I think this should be kept for using `start_from_trigger` in dynamic task mapping
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
Review Comment:
I think it should be
```suggestion
deferrable: bool = conf.getboolean("operators", "default_deferrable", fallback=False)
```
not sure why the pre-commit is not working, I'll take a look afterward
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2102404235
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
start_from_trigger: bool = False,
- trigger_kwargs: dict[str, Any] | None = None,
Review Comment:
Implemented!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2101523029
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
Review Comment:
https://github.com/apache/airflow/pull/50934
##########
providers/standard/src/airflow/providers/standard/sensors/time.py:
##########
@@ -100,32 +77,61 @@ def __init__(
self,
*,
target_time: datetime.time,
+ deferrable: bool = False,
Review Comment:
Fixed in this PR
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #50864:
URL: https://github.com/apache/airflow/pull/50864#discussion_r2100079171
##########
airflow-core/src/airflow/sensors/__init__.py:
##########
@@ -40,7 +40,6 @@
},
"time_sensor": {
"TimeSensor": "airflow.providers.standard.sensors.time.TimeSensor",
- "TimeSensorAsync": "airflow.providers.standard.sensors.time.TimeSensorAsync",
Review Comment:
Yeah to do this, the operator should exist, it won't work without it
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Merging `TimeSensorAsync` with `TimeSensor` [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #50864:
URL: https://github.com/apache/airflow/pull/50864
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org