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 10:40:51 UTC
[GitHub] [airflow] Lee-W opened a new pull request, #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Lee-W opened a new pull request, #33405:
URL: https://github.com/apache/airflow/pull/33405
This is the following PR to https://github.com/apache/airflow/pull/33196 and intends to solve the issue that the soft_fail argument might not work on all sensors.
This pull request addresses the issue of running BatchSensor in deferrable mode with `soft_fail=True` still raises an `AirflowException`, which should be `AirflowSkipException`. As
<!--
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] Lee-W commented on a diff in pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1295333987
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
Review Comment:
I don't, but my guess is 2.7.1?
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1307320384
##########
tests/providers/amazon/aws/sensors/test_batch.py:
##########
@@ -100,6 +100,12 @@ def test_execute_failure_in_deferrable_mode(self, deferrable_batch_sensor: Batch
with pytest.raises(AirflowException):
deferrable_batch_sensor.execute_complete(context={}, event={"status": "failure"})
+ def test_execute_failure_in_deferrable_mode_with_soft_fail(self, deferrable_batch_sensor: BatchSensor):
+ """Tests that an AirflowSkipException is raised in case of error event and soft_fail is set to True"""
+ deferrable_batch_sensor.soft_fail = True
+ with pytest.raises(AirflowException):
Review Comment:
Yep, you're right. Just updated it. 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] eladkal commented on a diff in pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1304789252
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +116,11 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # changed in https://github.com/apache/airflow/pull/33424 is released
Review Comment:
what if-else block are you referring to?
--
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] eladkal commented on a diff in pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1304793525
##########
airflow/providers/amazon/aws/sensors/utils.py:
##########
@@ -0,0 +1,28 @@
+# 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.
+from __future__ import annotations
+
+from airflow.exceptions import AirflowException, AirflowSkipException
+
+
+def raise_failed_or_skiping_exception(
+ *, soft_fail: bool = False, failed_message: str, skipping_message: str = ""
+) -> None:
+ """Raise AirflowSkipException if self.soft_fail is set to True. Otherwise raise AirflowException."""
+ if soft_fail:
+ raise AirflowSkipException(skipping_message or failed_message)
+ raise AirflowException(failed_message)
Review Comment:
I didn't track the flow of PR around the soft_fail but that is exactly what troubles me.
I can be the one that pick up the todo in future releases when we want to do the cleanup. I do not understand what happens here. What is the old and what is the new.
I trust you guys have clarity on this thus i'm removing my review and you can proceed as you see fit (as I'm not in the details)
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1295327281
##########
airflow/providers/amazon/aws/sensors/__init__.py:
##########
Review Comment:
Sounds good. I just move it to `airflow/providers/amazon/aws/sensors/utils.py`
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1295349323
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
Review Comment:
I can send another PR after the version is decided
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1306449016
##########
airflow/providers/amazon/aws/sensors/utils.py:
##########
@@ -0,0 +1,28 @@
+# 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.
+from __future__ import annotations
+
+from airflow.exceptions import AirflowException, AirflowSkipException
+
+
+def raise_failed_or_skiping_exception(
+ *, soft_fail: bool = False, failed_message: str, skipping_message: str = ""
+) -> None:
+ """Raise AirflowSkipException if self.soft_fail is set to True. Otherwise raise AirflowException."""
+ if soft_fail:
+ raise AirflowSkipException(skipping_message or failed_message)
+ raise AirflowException(failed_message)
Review Comment:
Could we remove this method and add this to each operator:
```python
if self.soft_fail:
raise AirflowSkipException(...)
raise AirflowException(...)
```
In this way we will be able to remove it later without worrying about breaking changes
--
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] eladkal commented on a diff in pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1304722753
##########
airflow/providers/amazon/aws/sensors/utils.py:
##########
@@ -0,0 +1,28 @@
+# 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.
+from __future__ import annotations
+
+from airflow.exceptions import AirflowException, AirflowSkipException
+
+
+def raise_failed_or_skiping_exception(
+ *, soft_fail: bool = False, failed_message: str, skipping_message: str = ""
+) -> None:
+ """Raise AirflowSkipException if self.soft_fail is set to True. Otherwise raise AirflowException."""
+ if soft_fail:
+ raise AirflowSkipException(skipping_message or failed_message)
+ raise AirflowException(failed_message)
Review Comment:
so basically what you are after is to backport a fix we have in 2.7.1
The proper way to do it is:
1. Try to import the function from base_sensor if failed on import error then use the local version.
2. The local version should be private `_raise_failed_or_skiping_exception` so we can remove it without deprecation cycle.
3. There should be a note on the function to remove it when min airflow version for provider is >=2.7.1
--
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] potiuk merged pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #33405:
URL: https://github.com/apache/airflow/pull/33405
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1295347917
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
Review Comment:
Let’s keep this then.
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
Review Comment:
Let’s keep this then.
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1295289373
##########
airflow/providers/amazon/aws/sensors/__init__.py:
##########
Review Comment:
Better put this in somewhere like `utils.py`; importing `__init__` in child modules can be problematic sometimes
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1307296401
##########
tests/providers/amazon/aws/sensors/test_batch.py:
##########
@@ -100,6 +100,12 @@ def test_execute_failure_in_deferrable_mode(self, deferrable_batch_sensor: Batch
with pytest.raises(AirflowException):
deferrable_batch_sensor.execute_complete(context={}, event={"status": "failure"})
+ def test_execute_failure_in_deferrable_mode_with_soft_fail(self, deferrable_batch_sensor: BatchSensor):
+ """Tests that an AirflowSkipException is raised in case of error event and soft_fail is set to True"""
+ deferrable_batch_sensor.soft_fail = True
+ with pytest.raises(AirflowException):
Review Comment:
We need to check if AirflowSkipException is raised not its superclass
```suggestion
with pytest.raises(AirflowSkipException):
```
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1304776191
##########
airflow/providers/amazon/aws/sensors/utils.py:
##########
@@ -0,0 +1,28 @@
+# 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.
+from __future__ import annotations
+
+from airflow.exceptions import AirflowException, AirflowSkipException
+
+
+def raise_failed_or_skiping_exception(
+ *, soft_fail: bool = False, failed_message: str, skipping_message: str = ""
+) -> None:
+ """Raise AirflowSkipException if self.soft_fail is set to True. Otherwise raise AirflowException."""
+ if soft_fail:
+ raise AirflowSkipException(skipping_message or failed_message)
+ raise AirflowException(failed_message)
Review Comment:
> Try to import the function from base_sensor if failed on import error then use the local version.
This method was removed from base sensor after merging #33424.
> The local version should be private _raise_failed_or_skiping_exception so we can remove it without deprecation cycle.
There should be a note on the function to remove it when min airflow version for provider is >=2.7.
a better solution could be removing this method and duplicating this small block in all the operators, it's the most safe solution to avoid breaking changes. 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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1304798303
##########
airflow/providers/amazon/aws/sensors/utils.py:
##########
@@ -0,0 +1,28 @@
+# 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.
+from __future__ import annotations
+
+from airflow.exceptions import AirflowException, AirflowSkipException
+
+
+def raise_failed_or_skiping_exception(
+ *, soft_fail: bool = False, failed_message: str, skipping_message: str = ""
+) -> None:
+ """Raise AirflowSkipException if self.soft_fail is set to True. Otherwise raise AirflowException."""
+ if soft_fail:
+ raise AirflowSkipException(skipping_message or failed_message)
+ raise AirflowException(failed_message)
Review Comment:
Basically we fixed the issue in a more proper way in base sensor, and all the providers will work fine with Airflow +2.7.1, but to fix this issue with older versions of Airflow, we need to implement a temporary solution in each one until bumping the min Airflow version for these providers to 2.7.1
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #33405:
URL: https://github.com/apache/airflow/pull/33405#issuecomment-1694354346
> or should we always send a failure event to execute_complete?
The problem is that in the older versions of Airflow, the failure is handled without executing the task methods.
I checked and I didn't find a simple solution. W can accept a partial fix, and if the user want a full fix, he can upgrade to 2.7.1
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #33405:
URL: https://github.com/apache/airflow/pull/33405#issuecomment-1693683180
> This solution will work if there is no errors in the trigger (and it is not canceled after a timeout). Let me check if we can implement something which always work or we should accept a partial solution as it's temporary.
or should we always send a failure event to `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] uranusjr commented on a diff in pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1295331003
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
Review Comment:
Do we know what version this will be yet? It would be easier to search if we have _Airflow X.Y_ in the comment.
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1296424064
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
+ if getattr(self, "raise_failed_or_skiping_exception"):
+ self.raise_failed_or_skiping_exception(failed_message=message)
Review Comment:
I will open a discussion on slack, related to this, could we wait before merging?
--
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 #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1306594185
##########
airflow/providers/amazon/aws/sensors/utils.py:
##########
@@ -0,0 +1,28 @@
+# 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.
+from __future__ import annotations
+
+from airflow.exceptions import AirflowException, AirflowSkipException
+
+
+def raise_failed_or_skiping_exception(
+ *, soft_fail: bool = False, failed_message: str, skipping_message: str = ""
+) -> None:
+ """Raise AirflowSkipException if self.soft_fail is set to True. Otherwise raise AirflowException."""
+ if soft_fail:
+ raise AirflowSkipException(skipping_message or failed_message)
+ raise AirflowException(failed_message)
Review Comment:
Sure! I just updated it as 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] Lee-W commented on a diff in pull request #33405: respect "soft_fail" argument when running BatchSensor in deferrable mode
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #33405:
URL: https://github.com/apache/airflow/pull/33405#discussion_r1302818677
##########
airflow/providers/amazon/aws/sensors/batch.py:
##########
@@ -115,7 +115,15 @@ def execute_complete(self, context: Context, event: dict[str, Any]) -> None:
Relies on trigger to throw an exception, otherwise it assumes execution was successful.
"""
if event["status"] != "success":
- raise AirflowException(f"Error while running job: {event}")
+ message = f"Error while running job: {event}"
+ # TODO: remove this if-else block when min_airflow_version is set to higher than the version that
+ # raise_failed_or_skiping_exception is introduced in BaseSensorOperator
+ if getattr(self, "raise_failed_or_skiping_exception"):
+ self.raise_failed_or_skiping_exception(failed_message=message)
Review Comment:
Sure. I just updated the TODO string. Could you please take a look? 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