You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Lee-W (via GitHub)" <gi...@apache.org> on 2023/08/15 08:51:52 UTC
[GitHub] [airflow] Lee-W opened a new pull request, #33403: Respect soft fail for core async sensors
Lee-W opened a new pull request, #33403:
URL: https://github.com/apache/airflow/pull/33403
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
[GitHub] [airflow] hussein-awala commented on pull request #33403: Respect "soft_fail" for core async sensors
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #33403:
URL: https://github.com/apache/airflow/pull/33403#issuecomment-1681294660
Since it's a problem in the initialization of the trigger, and it is not a runtime issue, what about raising an exception in parsing time, to give the user a fast feedback about the wrong parameters, for example:
```diff
diff --git a/airflow/sensors/date_time.py b/airflow/sensors/date_time.py
index 1425028870..a036d09e01 100644
--- a/airflow/sensors/date_time.py
+++ b/airflow/sensors/date_time.py
@@ -69,6 +69,7 @@ class DateTimeSensor(BaseSensorOperator):
raise TypeError(
f"Expected str or datetime.datetime type for target_time. Got {type(target_time)}"
)
+ self.trigger = DateTimeTrigger(moment=timezone.parse(self.target_time))
def poke(self, context: Context) -> bool:
self.log.info("Checking if the time (%s) has come", self.target_time)
@@ -87,7 +88,7 @@ class DateTimeSensorAsync(DateTimeSensor):
def execute(self, context: Context):
self.defer(
- trigger=DateTimeTrigger(moment=timezone.parse(self.target_time)),
+ trigger=self.trigger,
method_name="execute_complete",
)
```
This way the dag parsing will fail, and the user will fix the issue.
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295291332
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
Do we need to use `from` here? I feel it’d be more concise to combine the two error messages into `AirflowSkipException` instead.
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
Do we need to use `from` here? I feel it’d be more concise to combine the two error messages into `AirflowSkipException` instead.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295330532
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
Let’s raise the skip exception directly (and include the type error message in it)
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
Let’s raise the skip exception directly (and include the type error message in 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
[GitHub] [airflow] Lee-W commented on pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #33403:
URL: https://github.com/apache/airflow/pull/33403#issuecomment-1681533783
> Since it's a problem in the initialization of the trigger, and it is not a runtime issue, what about raising an exception in parsing time, to give the user a fast feedback about the wrong parameters, for example:
>
> ```diff
> diff --git a/airflow/sensors/date_time.py b/airflow/sensors/date_time.py
> index 1425028870..a036d09e01 100644
> --- a/airflow/sensors/date_time.py
> +++ b/airflow/sensors/date_time.py
> @@ -69,6 +69,7 @@ class DateTimeSensor(BaseSensorOperator):
> raise TypeError(
> f"Expected str or datetime.datetime type for target_time. Got {type(target_time)}"
> )
> + self.trigger = DateTimeTrigger(moment=timezone.parse(self.target_time))
>
> def poke(self, context: Context) -> bool:
> self.log.info("Checking if the time (%s) has come", self.target_time)
> @@ -87,7 +88,7 @@ class DateTimeSensorAsync(DateTimeSensor):
>
> def execute(self, context: Context):
> self.defer(
> - trigger=DateTimeTrigger(moment=timezone.parse(self.target_time)),
> + trigger=self.trigger,
> method_name="execute_complete",
> )
> ```
>
> This way the dag parsing will fail, and the user will fix the issue.
>
> WDYT?
This might not be doable for https://github.com/apache/airflow/blob/a6d803303bf71a84e9e59e94d9c088e3120bedb5/airflow/sensors/time_delta.py#L65-L66
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1303946632
##########
airflow/sensors/date_time.py:
##########
@@ -85,9 +85,13 @@ class DateTimeSensorAsync(DateTimeSensor):
:param target_time: datetime after which the job succeeds. (templated)
"""
+ def __init__(self, **kwargs) -> None:
+ super().__init__(**kwargs)
+ self.trigger = DateTimeTrigger(moment=timezone.parse(self.target_time))
+
def execute(self, context: Context):
self.defer(
- trigger=DateTimeTrigger(moment=timezone.parse(self.target_time)),
+ trigger=self.trigger,
Review Comment:
Why is this change needed? I don’t see `self.trigger` used anywhere else.
--
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
[GitHub] [airflow] Lee-W commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295669974
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,30 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, *, soft_fail: bool = False) -> None:
super().__init__()
+ skipping_message_postfix = "Skipping due to soft_fail is set to True."
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ message = f"Expected datetime.datetime type for moment. Got {type(moment)}"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
+ raise TypeError(message)
# Make sure it's in UTC
elif moment.tzinfo is None:
- raise ValueError("You cannot pass naive datetimes")
+ message = "You cannot pass naive datetimes"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
Review Comment:
Or what do you think about my first commit? https://github.com/apache/airflow/pull/33403/commits/b2f2662ae1a11ea928aad57acd2892c763c2db25 I was trying not to duplicate the check logic and thus moved it to the trigger
--
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
[GitHub] [airflow] Lee-W commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295335522
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
Sure. just updated it!
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
Review Comment:
Sure. just updated 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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295290027
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
Review Comment:
```suggestion
def __init__(self, moment: datetime.datetime, *, soft_fail: bool = False) -> None:
```
--
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
[GitHub] [airflow] Lee-W commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295637676
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,30 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, *, soft_fail: bool = False) -> None:
super().__init__()
+ skipping_message_postfix = "Skipping due to soft_fail is set to True."
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ message = f"Expected datetime.datetime type for moment. Got {type(moment)}"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
+ raise TypeError(message)
# Make sure it's in UTC
elif moment.tzinfo is None:
- raise ValueError("You cannot pass naive datetimes")
+ message = "You cannot pass naive datetimes"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
Review Comment:
Take `DateTimeSensorAsync` as an example. The second line will raise the `AirflowSkipException` before we execute `self.defer`
```python
self.defer(
trigger=DateTimeTrigger(moment=target_dttm, soft_fail=self.soft_fail),
method_name="execute_complete",
)
```
--
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
[GitHub] [airflow] Lee-W commented on pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #33403:
URL: https://github.com/apache/airflow/pull/33403#issuecomment-1679821158
> Let's wait #33424 before deciding if we need to merge this one.
Hi, I just checked #33424. We might still need this one, as the exceptions are raised in the `execute` method before we enter deferring. (but the solution in #33424 can still save us time handling these kinds of stuff in `execute_complete` which is still great!)
--
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
[GitHub] [airflow] hussein-awala commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295665756
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,30 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, *, soft_fail: bool = False) -> None:
super().__init__()
+ skipping_message_postfix = "Skipping due to soft_fail is set to True."
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ message = f"Expected datetime.datetime type for moment. Got {type(moment)}"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
+ raise TypeError(message)
# Make sure it's in UTC
elif moment.tzinfo is None:
- raise ValueError("You cannot pass naive datetimes")
+ message = "You cannot pass naive datetimes"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
Review Comment:
What about:
```python
try:
self.defer(
trigger=DateTimeTrigger(moment=target_dttm, soft_fail=self.soft_fail),
method_name="execute_complete",
)
except Exception as e:
raise AirflowException(str(e))
```
or just changing `raise TypeError(message)` and `raise ValueError` to `AirflowException`.
In both cases the exception will be cached by BaseSensor, and a `AirflowSkipException` will be raised when soft_fail is True. I'm trying to avoid complicating the trigger code, and duplicating the soft fail logic.
cc: @uranusjr @potiuk 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
[GitHub] [airflow] hussein-awala commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295612738
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,30 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, *, soft_fail: bool = False) -> None:
super().__init__()
+ skipping_message_postfix = "Skipping due to soft_fail is set to True."
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ message = f"Expected datetime.datetime type for moment. Got {type(moment)}"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
+ raise TypeError(message)
# Make sure it's in UTC
elif moment.tzinfo is None:
- raise ValueError("You cannot pass naive datetimes")
+ message = "You cannot pass naive datetimes"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
Review Comment:
How could raising an `AirflowSkipException` in the trigger pass the task state to skipped? The trigger exception is not propagated to task resuming if I am not mistaken. 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
[GitHub] [airflow] hussein-awala merged pull request #33403: Respect "soft_fail" for core async sensors
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #33403:
URL: https://github.com/apache/airflow/pull/33403
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1303946632
##########
airflow/sensors/date_time.py:
##########
@@ -85,9 +85,13 @@ class DateTimeSensorAsync(DateTimeSensor):
:param target_time: datetime after which the job succeeds. (templated)
"""
+ def __init__(self, **kwargs) -> None:
+ super().__init__(**kwargs)
+ self.trigger = DateTimeTrigger(moment=timezone.parse(self.target_time))
+
def execute(self, context: Context):
self.defer(
- trigger=DateTimeTrigger(moment=timezone.parse(self.target_time)),
+ trigger=self.trigger,
Review Comment:
Why is this change needed? I don’t see `self.trigger` used anywhere else.
##########
airflow/sensors/date_time.py:
##########
@@ -85,9 +85,13 @@ class DateTimeSensorAsync(DateTimeSensor):
:param target_time: datetime after which the job succeeds. (templated)
"""
+ def __init__(self, **kwargs) -> None:
+ super().__init__(**kwargs)
+ self.trigger = DateTimeTrigger(moment=timezone.parse(self.target_time))
+
def execute(self, context: Context):
self.defer(
- trigger=DateTimeTrigger(moment=timezone.parse(self.target_time)),
+ trigger=self.trigger,
Review Comment:
Why is this change needed? I don’t see `self.trigger` used anywhere else.
--
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
[GitHub] [airflow] Lee-W commented on pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #33403:
URL: https://github.com/apache/airflow/pull/33403#issuecomment-1693681298
hi @hussein-awala, could you please check whether it works for you when you have time? Thanks!
--
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
[GitHub] [airflow] Lee-W commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1295321179
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
`from` is not necessary here. Either way works fine for me. Let me change to what you suggested. 🙂
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,29 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, soft_fail: bool = False):
super().__init__()
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ exc = TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ if soft_fail:
+ raise AirflowSkipException("Skipping due to soft_fail is set to True.") from exc
+ raise exc
Review Comment:
`from` is not necessary here. Either way works fine for me. Let me change to what you suggested. 🙂
--
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
[GitHub] [airflow] hussein-awala commented on a diff in pull request #33403: Respect "soft_fail" for core async sensors
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33403:
URL: https://github.com/apache/airflow/pull/33403#discussion_r1296445466
##########
airflow/triggers/temporal.py:
##########
@@ -34,18 +35,30 @@ class DateTimeTrigger(BaseTrigger):
The provided datetime MUST be in UTC.
"""
- def __init__(self, moment: datetime.datetime):
+ def __init__(self, moment: datetime.datetime, *, soft_fail: bool = False) -> None:
super().__init__()
+ skipping_message_postfix = "Skipping due to soft_fail is set to True."
if not isinstance(moment, datetime.datetime):
- raise TypeError(f"Expected datetime.datetime type for moment. Got {type(moment)}")
+ message = f"Expected datetime.datetime type for moment. Got {type(moment)}"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
+ raise TypeError(message)
# Make sure it's in UTC
elif moment.tzinfo is None:
- raise ValueError("You cannot pass naive datetimes")
+ message = "You cannot pass naive datetimes"
+ if soft_fail:
+ raise AirflowSkipException(f"{message}. {skipping_message_postfix}")
Review Comment:
for me it's much better
--
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