You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/13 12:05:59 UTC
[GitHub] [airflow] eladkal opened a new pull request, #22974: Replace usage of `DummyOperator` with `EmptyOperator`
eladkal opened a new pull request, #22974:
URL: https://github.com/apache/airflow/pull/22974
This is a followup PR to https://github.com/apache/airflow/pull/22832
1. Example dags in Providers are handled differently because Providers should be compatible with Airflow 2.1
2. In core / tests all changed to `EmptyOperator`
<!--
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 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/
-->
---
**^ 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849448708
##########
tests/dags/test_only_empty_tasks.py:
##########
@@ -41,14 +41,14 @@ def __init__(self, body, *args, **kwargs):
with dag:
- task_a = DummyOperator(task_id="test_task_a")
+ task_a = EmptyOperator(task_id="test_task_a")
- task_b = DummyOperator(task_id="test_task_b")
+ task_b = EmptyOperator(task_id="test_task_b")
task_a >> task_b
- task_c = MyDummyOperator(task_id="test_task_c", body={"hello": "world"})
+ task_c = MyEmptyOperator(task_id="test_task_c", body={"hello": "world"})
- task_d = DummyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
+ task_d = EmptyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
- task_e = DummyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
+ task_e = EmptyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
Review Comment:
It's there since the test was added.
I'm not sure too but we both checked the PR 2 years ago :)
https://github.com/apache/airflow/pull/7880/files
--
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] github-actions[bot] commented on pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22974:
URL: https://github.com/apache/airflow/pull/22974#issuecomment-1098185133
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849483424
##########
tests/dags/test_only_empty_tasks.py:
##########
@@ -41,14 +41,14 @@ def __init__(self, body, *args, **kwargs):
with dag:
- task_a = DummyOperator(task_id="test_task_a")
+ task_a = EmptyOperator(task_id="test_task_a")
- task_b = DummyOperator(task_id="test_task_b")
+ task_b = EmptyOperator(task_id="test_task_b")
task_a >> task_b
- task_c = MyDummyOperator(task_id="test_task_c", body={"hello": "world"})
+ task_c = MyEmptyOperator(task_id="test_task_c", body={"hello": "world"})
- task_d = DummyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
+ task_d = EmptyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
- task_e = DummyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
+ task_e = EmptyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
Review Comment:
I changed it to `None` https://github.com/apache/airflow/pull/22974/commits/e1492dd6c399eef3ef0fc551d922030571edd7bb
--
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 commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849462976
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
What @ashb proposed is fine. We want to promote EmptyOperator usage and import Dummy as Empty in import on "module not found" makes a nice fallback. We only need falback for compatibilty but we want that anyone looking at our providers from now on to use Empty.
The tests will fail if they don't do the fallback so we will detect it - we can also even add a e specialized pre-commit that will look for this:
and suggest changing:
```
from airflow.operators.empty import EmptyOperator
```
into
```
try:
from airflow.operators.empty import EmptyOperator
except ModuleNotFoundError:
from airflow.operators.dummy import DummyOperator as Empty Operator
```
if import is detected without try/ except around it. That's easy check.
--
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 commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849462976
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
This is fine. We want to promote EmptyOperator usage and import Dummy as Emptyi in import on "module not found" makes a nice fallback. We only need falback for compatibilty but we want that anyone looking at our providers from now on used Empty*.
The tests will fail if they don't do the fallback so we will detect it - we can also even add a e specialized pre-commit that will look for this:
and suggest changing:
```
from airflow.operators.empty import EmptyOperator
```
into
```
try:
from airflow.operators.empty import EmptyOperator
except ModuleNotFoundError:
from airflow.operators.dummy import DummyOperator as Empty Operator
```
if import is detected without try/ except around it. That's easy check.
--
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 merged pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal merged PR #22974:
URL: https://github.com/apache/airflow/pull/22974
--
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 commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849416911
##########
tests/dags/test_only_empty_tasks.py:
##########
@@ -41,14 +41,14 @@ def __init__(self, body, *args, **kwargs):
with dag:
- task_a = DummyOperator(task_id="test_task_a")
+ task_a = EmptyOperator(task_id="test_task_a")
- task_b = DummyOperator(task_id="test_task_b")
+ task_b = EmptyOperator(task_id="test_task_b")
task_a >> task_b
- task_c = MyDummyOperator(task_id="test_task_c", body={"hello": "world"})
+ task_c = MyEmptyOperator(task_id="test_task_c", body={"hello": "world"})
- task_d = DummyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
+ task_d = EmptyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
- task_e = DummyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
+ task_e = EmptyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
Review Comment:
Why do we have `**kwargs: 1` there ? I do not understand it - I believe it should be `**kwargs: 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] potiuk commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849462976
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
What @ashb proposed is fine. We want to promote EmptyOperator usage and import Dummy as Empty in import on "module not found" makes a nice fallback. We only need falback for compatibilty but we want that anyone looking at our providers from now on to use Empty.
The tests will fail if they don't do the fallback so we will detect it - we can also even add a e specialized pre-commit that will look for this:
and suggest changing (or even automatically changing):
```
from airflow.operators.empty import EmptyOperator
```
into
```
try:
from airflow.operators.empty import EmptyOperator
except ModuleNotFoundError:
from airflow.operators.dummy import DummyOperator as Empty Operator
```
if import is detected without try/ except around it. That's easy check.
--
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 commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849464563
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
But we can do it as next step.
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849534845
##########
airflow/operators/dummy_operator.py:
##########
@@ -15,12 +15,24 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""This module is deprecated. Please use :mod:`airflow.operators.dummy`."""
+"""This module is deprecated. Please use :mod:`airflow.operators.empty`."""
import warnings
-from airflow.operators.dummy import DummyOperator # noqa
+from airflow.operators.empty import EmptyOperator
warnings.warn(
- "This module is deprecated. Please use `airflow.operators.dummy`.", DeprecationWarning, stacklevel=2
+ "This module is deprecated. Please use `airflow.operators.empty`.", DeprecationWarning, stacklevel=2
)
+
+
+class DummyOperator(EmptyOperator):
Review Comment:
There was a [failure](https://github.com/apache/airflow/runs/6008279385?check_suite_focus=true#step:8:3222) in CI:
FAILED tests/always/test_deprecations.py::TestDeprecations::test_warning_on_import[airflow.operators.empty.EmptyOperator-airflow.operators.dummy_operator.DummyOperator]
```
I'm not sure why it raises now and not in the deprecation PR :\ ?
But in any case added handling for both `dummy.py` and `dummy_operator.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] eladkal commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849406923
##########
tests/dags/test_only_empty_tasks.py:
##########
@@ -41,14 +41,14 @@ def __init__(self, body, *args, **kwargs):
with dag:
- task_a = DummyOperator(task_id="test_task_a")
+ task_a = EmptyOperator(task_id="test_task_a")
- task_b = DummyOperator(task_id="test_task_b")
+ task_b = EmptyOperator(task_id="test_task_b")
task_a >> task_b
- task_c = MyDummyOperator(task_id="test_task_c", body={"hello": "world"})
+ task_c = MyEmptyOperator(task_id="test_task_c", body={"hello": "world"})
- task_d = DummyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
+ task_d = EmptyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
- task_e = DummyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
+ task_e = EmptyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
Review Comment:
This is going to fail on mypy test.
```
Run mypy.................................................................................Failed
- hook id: mypy
- exit code: 1
tests/dags/test_only_empty_tasks.py:54: error: Argument "on_execute_callback"
to "EmptyOperator" has incompatible type "Callable[[Context], int]"; expected
"Optional[Callable[[Context], None]]" [arg-type]
...k_id="test_task_on_execute", on_execute_callback=lambda *args, **kwarg...
^
tests/dags/test_only_empty_tasks.py:56: error: Argument "on_success_callback"
to "EmptyOperator" has incompatible type "Callable[[Context], int]"; expected
"Optional[Callable[[Context], None]]" [arg-type]
...k_id="test_task_on_success", on_success_callback=lambda *args, **kwarg...
^
Found 2 errors in 1 file (checked 1 source file)
ERROR: The previous step completed with error. Please take a look at output above
```
In https://github.com/apache/airflow/pull/22832/commits/2a626c73270831ccc58d8e91f035868797dba697 I removed the `__init__` of `EmptyOperator` (see https://github.com/apache/airflow/pull/22832#discussion_r848155925 ). Setting it back resolves the mypy issue but I'm not sure if this is the way to go.
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849455227
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
doesn't sound like a big change I can do this.
But users for Airflow<2.3 are not really aware of `EmptyOperator`. It might be strange to them?
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849534845
##########
airflow/operators/dummy_operator.py:
##########
@@ -15,12 +15,24 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""This module is deprecated. Please use :mod:`airflow.operators.dummy`."""
+"""This module is deprecated. Please use :mod:`airflow.operators.empty`."""
import warnings
-from airflow.operators.dummy import DummyOperator # noqa
+from airflow.operators.empty import EmptyOperator
warnings.warn(
- "This module is deprecated. Please use `airflow.operators.dummy`.", DeprecationWarning, stacklevel=2
+ "This module is deprecated. Please use `airflow.operators.empty`.", DeprecationWarning, stacklevel=2
)
+
+
+class DummyOperator(EmptyOperator):
Review Comment:
There was a [failure](https://github.com/apache/airflow/runs/6008279385?check_suite_focus=true#step:8:3222) in CI:
` FAILED tests/always/test_deprecations.py::TestDeprecations::test_warning_on_import[airflow.operators.empty.EmptyOperator-airflow.operators.dummy_operator.DummyOperator]`
I'm not sure why it raises now and not in the deprecation PR :\ ?
But in any case added handling for both `dummy.py` and `dummy_operator.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] eladkal commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849534845
##########
airflow/operators/dummy_operator.py:
##########
@@ -15,12 +15,24 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""This module is deprecated. Please use :mod:`airflow.operators.dummy`."""
+"""This module is deprecated. Please use :mod:`airflow.operators.empty`."""
import warnings
-from airflow.operators.dummy import DummyOperator # noqa
+from airflow.operators.empty import EmptyOperator
warnings.warn(
- "This module is deprecated. Please use `airflow.operators.dummy`.", DeprecationWarning, stacklevel=2
+ "This module is deprecated. Please use `airflow.operators.empty`.", DeprecationWarning, stacklevel=2
)
+
+
+class DummyOperator(EmptyOperator):
Review Comment:
There was a [failure](https://github.com/apache/airflow/runs/6008279385?check_suite_focus=true#step:8:3222):
FAILED tests/always/test_deprecations.py::TestDeprecations::test_warning_on_import[airflow.operators.empty.EmptyOperator-airflow.operators.dummy_operator.DummyOperator]
I'm not sure why it raises now and not in the deprecation PR :\ ?
But in any case added handling for both `dummy.py` and `dummy_operator.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] eladkal commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849447312
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
mypy is very picky in this case
```
Run mypy.................................................................................Failed
- hook id: mypy
- exit code: 1
airflow/providers/microsoft/azure/example_dags/example_adf_run_pipeline.py:25: error:
Incompatible import of "DummyOperator" (imported name has type
"Type[DummyOperator]", local name has type "Type[EmptyOperator]") [misc]
```
followed https://stackoverflow.com/questions/59037244/mypy-incompatible-import-error-for-conditional-imports which recommanded to add `type: ignore` in this case.
--
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] ashb commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849451006
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
I know its a slightly bigger change, but shouldn't we do this as
```suggestion
from airflow.operators.dummy import DummyOperator as EmptyOperator # type: ignore
```
and then change all the uses to `EmptyOperator()`?
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849406923
##########
tests/dags/test_only_empty_tasks.py:
##########
@@ -41,14 +41,14 @@ def __init__(self, body, *args, **kwargs):
with dag:
- task_a = DummyOperator(task_id="test_task_a")
+ task_a = EmptyOperator(task_id="test_task_a")
- task_b = DummyOperator(task_id="test_task_b")
+ task_b = EmptyOperator(task_id="test_task_b")
task_a >> task_b
- task_c = MyDummyOperator(task_id="test_task_c", body={"hello": "world"})
+ task_c = MyEmptyOperator(task_id="test_task_c", body={"hello": "world"})
- task_d = DummyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
+ task_d = EmptyOperator(task_id="test_task_on_execute", on_execute_callback=lambda *args, **kwargs: 1)
- task_e = DummyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
+ task_e = EmptyOperator(task_id="test_task_on_success", on_success_callback=lambda *args, **kwargs: 1)
Review Comment:
This is going to fail on mypy test.
```
Run mypy.................................................................................Failed
- hook id: mypy
- exit code: 1
tests/dags/test_only_empty_tasks.py:54: error: Argument "on_execute_callback"
to "EmptyOperator" has incompatible type "Callable[[Context], int]"; expected
"Optional[Callable[[Context], None]]" [arg-type]
...k_id="test_task_on_execute", on_execute_callback=lambda *args, **kwarg...
^
tests/dags/test_only_empty_tasks.py:56: error: Argument "on_success_callback"
to "EmptyOperator" has incompatible type "Callable[[Context], int]"; expected
"Optional[Callable[[Context], None]]" [arg-type]
...k_id="test_task_on_success", on_success_callback=lambda *args, **kwarg...
^
Found 2 errors in 1 file (checked 1 source file)
ERROR: The previous step completed with error. Please take a look at output above
```
In https://github.com/apache/airflow/pull/22832/commits/2a626c73270831ccc58d8e91f035868797dba697 I removed the init of EmptyOperator (see https://github.com/apache/airflow/pull/22832#discussion_r848155925 ). Setting it back resolves the mypy issue but I'm not sure if this is the way to go.
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849473935
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
it's a small change no worries :)
see https://github.com/apache/airflow/pull/22974/commits/321fc1fa02c4ca03a98d336dffc848385b6aa7ee
--
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 #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849447312
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
mypy is very picky in this cases
```
Run mypy.................................................................................Failed
- hook id: mypy
- exit code: 1
airflow/providers/microsoft/azure/example_dags/example_adf_run_pipeline.py:25: error:
Incompatible import of "DummyOperator" (imported name has type
"Type[DummyOperator]", local name has type "Type[EmptyOperator]") [misc]
```
followed https://stackoverflow.com/questions/59037244/mypy-incompatible-import-error-for-conditional-imports which recommanded to add `type: ignore` in this case.
--
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 commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849462976
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
What @ashb proposed is fine. We want to promote EmptyOperator usage and import Dummy as Emptyi in import on "module not found" makes a nice fallback. We only need falback for compatibilty but we want that anyone looking at our providers from now on to used Empty.
The tests will fail if they don't do the fallback so we will detect it - we can also even add a e specialized pre-commit that will look for this:
and suggest changing:
```
from airflow.operators.empty import EmptyOperator
```
into
```
try:
from airflow.operators.empty import EmptyOperator
except ModuleNotFoundError:
from airflow.operators.dummy import DummyOperator as Empty Operator
```
if import is detected without try/ except around it. That's easy check.
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
What @ashb proposed is fine. We want to promote EmptyOperator usage and import Dummy as Emptyi in import on "module not found" makes a nice fallback. We only need falback for compatibilty but we want that anyone looking at our providers from now on to use Empty.
The tests will fail if they don't do the fallback so we will detect it - we can also even add a e specialized pre-commit that will look for this:
and suggest changing:
```
from airflow.operators.empty import EmptyOperator
```
into
```
try:
from airflow.operators.empty import EmptyOperator
except ModuleNotFoundError:
from airflow.operators.dummy import DummyOperator as Empty Operator
```
if import is detected without try/ except around it. That's easy check.
--
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 commented on a diff in pull request #22974: Replace usage of `DummyOperator` with `EmptyOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22974:
URL: https://github.com/apache/airflow/pull/22974#discussion_r849462976
##########
airflow/providers/dbt/cloud/example_dags/example_dbt_cloud.py:
##########
@@ -22,7 +22,7 @@
try:
from airflow.operators.empty import EmptyOperator as DummyOperator
except ModuleNotFoundError:
- from airflow.operators.dummy import DummyOperator
+ from airflow.operators.dummy import DummyOperator # type: ignore
Review Comment:
What @ashb proposed is fine. We want to promote EmptyOperator usage and import Dummy as Emptyi in import on "module not found" makes a nice fallback. We only need falback for compatibilty but we want that anyone looking at our providers from now on used Empty*.
The tests will fail if they don't do the fallback so we will detect it - we can also even add a e specialized pre-commit that will look for this:
and suggest changing:
```
from airflow.operators.empty import EmptyOperator
```
into
```
try:
from airflow.operators.empty import EmptyOperator
except ModuleNotFoundError:
from airflow.operators.dummy import DummyOperator as Empty Operator
```
if import is detected without try/ except around it. That's easy check.
--
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