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/07/22 19:55:42 UTC
[GitHub] [airflow] josh-fell opened a new pull request, #25242: Update core example DAGs to use `@task.branch` decorator
josh-fell opened a new pull request, #25242:
URL: https://github.com/apache/airflow/pull/25242
Related: #9415
Now that the `@task.branch` TaskFlow decorator has been released, the core example DAGs which previously used the `BranchPythonOperator` should now use the decorator where applicable.
---
**^ 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+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 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] potiuk commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r932212169
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
I am indifferent with that one. We already have a few cases where __future__ annotation is used (in `breeze` for example) so It's already spilled here and there in our codebase. It's harmless for cherry-picking in examples, so this is not a worry for me. I guess we can slowly start with adding __future_ annotations in places where we have low probability of conflicts with cherry-picking.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r927951228
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
To make mypy happy now that an example DAG uses the `override()` method.
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r933725875
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
I don’t think we want to _enforce_ it for now. We can change each file gradually when we hit them instead, and pyupgrade performs per-file checks quite well already (it enforces modern syntax if you add the `__future__` import in a file).
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r928063360
##########
airflow/example_dags/example_datasets.py:
##########
@@ -41,12 +41,11 @@
DAG example_dataset_dag9 should fail its only task and never trigger example_dataset_dag10_req_dag9
"""
Review Comment:
Took the liberty to update this DAG to use TaskFlow API generally and conform the `start_date` declaration we've been using for example DAGs.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r931068016
##########
airflow/decorators/base.py:
##########
@@ -533,6 +533,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def expand_kwargs(self, kwargs: XComArg, *, strict: bool = True) -> XComArg:
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[FParams, FReturn, OperatorSubclass]":
Review Comment:
Excellent. Let's do 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] potiuk commented on pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25242:
URL: https://github.com/apache/airflow/pull/25242#issuecomment-1195446887
I am afraid we have circular import somewhere.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r931186044
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
Interesting idea. Since there is a _lot_ of blind copy/paste from examples by users, do we want `from __future__ import annotation` to be a pattern users should follow? This would be my only concern really.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r931186044
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
Interesting idea. Since there is a _lot_ of blind copy/paste from examples, do we want `from __future__ import annotation` to be a pattern users should follow? This would be my only concern really.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r933264822
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
OK, I have no problem adding it to these examples. Happened to find this [flake8 plugin](https://pypi.org/project/flake8-future-annotations/) for future annotations if we want to start enforcing it in CI too in, dare I say, the future.
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r928136556
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
Yeah I saw similar behaviour happening more frequently since we https://github.com/apache/airflow/pull/25088 - there are more cases when "small set of files changed" produced different result than "big set of files". I will investigate if we can make MyPy more "greedy" in terms of considering bigger set of files and searching "deeper" if only few files changed - because ultimately I think this is what's happening, that MyPy is not getting all the information neded to arrive to the same conclusion about parsing.
I will look a bit deeer on how MyPy makes it's decisions and maybe we can improve some of this "partial changes mypy" experience.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r927988882
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
Hmm mypy is, in fact, not happy which is different than the local static check runs. I'll take a look.
--
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] josh-fell commented on pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25242:
URL: https://github.com/apache/airflow/pull/25242#issuecomment-1195815747
I chose not to update `branch_python_operator.py` accordingly but to keep it as a "legacy" example similar to what we have with the ETL example DAGs. It is also used in a number of unit tests. If we're fine with deprecating this "legacy" example I'm all for it, but one could also argue it's good to keep it for the unit testing of the underlying operator. Any other opinions?
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r930595264
##########
airflow/decorators/base.py:
##########
@@ -533,6 +533,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def expand_kwargs(self, kwargs: XComArg, *, strict: bool = True) -> XComArg:
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[FParams, FReturn, OperatorSubclass]":
Review Comment:
```suggestion
def override(self, **kwargs: Any) -> "Task[FParams, FReturn]":
```
This is the user-facing type, more friendly to see in autocompletion.
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r928136504
##########
airflow/example_dags/example_datasets.py:
##########
@@ -41,12 +41,11 @@
DAG example_dataset_dag9 should fail its only task and never trigger example_dataset_dag10_req_dag9
"""
Review Comment:
Yeah I saw similar behaviour happening more frequently since we [updated to the newer mypy version](https://github.com/apache/airflow/pull/25088) - there are more cases when "small set of files changed" produced different result than "big set of files". I will investigate if we can make MyPy more "greedy" in terms of considering bigger set of files and searching "deeper" if only few files changed - because ultimately I think this is what's happening, that MyPy is not getting all the information neded to arrive to the same conclusion about parsing.
I will look a bit deeer on how MyPy makes it's decisions and maybe we can improve some of this "partial changes mypy" experience.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r931065722
##########
airflow/example_dags/example_datasets.py:
##########
@@ -70,21 +69,17 @@
with DAG(
Review Comment:
I'm not sure. We are inconsistent from example to example but not within example files. I can unify them here and update the dedent in the docs too.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r933264822
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
OK, I have no problem adding it to these examples. Happened to find this [flake8 plugin](https://pypi.org/project/flake8-future-annotations/) for `__future__` annotations if we want to start enforcing it in CI too in, dare I say, the future.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r927988882
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
Ugh mypy is, in fact, not happy.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r928129858
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
OK difference in mypy behavior is obviously from running static checks on modified files (local dev) and all files (in CI). I'll continue to investigate, but seems a little more complex to fix (although mypy is not my forté).
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25242:
URL: https://github.com/apache/airflow/pull/25242
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r928136556
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
Yeah I saw similar behaviour happening more frequently since we updated to newer mypy version https://github.com/apache/airflow/pull/25088 - there are more cases when "small set of files changed" produced different result than "big set of files". I will investigate if we can make MyPy more "greedy" in terms of considering bigger set of files and searching "deeper" if only few files changed - because ultimately I think this is what's happening, that MyPy is not getting all the information neded to arrive to the same conclusion about parsing.
I will look a bit deeer on how MyPy makes it's decisions and maybe we can improve some of this "partial changes mypy" experience.
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r930596224
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
I wonder if this is a good chance to also switch these DAGs to `from __future__ import annotation`. It would allow us to write e.g. `list[str]` instead of importing from typing.
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r930595516
##########
airflow/decorators/base.py:
##########
@@ -553,6 +556,9 @@ def __call__(
) -> Callable[[Callable[FParams, FReturn]], Task[FParams, FReturn]]:
"""For the decorator factory ``@task()`` case."""
+ def override(self, **kwargs: Any) -> "_TaskDecorator[FParams, FReturn, OperatorSubclass]":
Review Comment:
```suggestion
def override(self, **kwargs: Any) -> "Task[FParams, FReturn]":
```
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r930597225
##########
airflow/example_dags/example_datasets.py:
##########
@@ -70,21 +69,17 @@
with DAG(
Review Comment:
Just wondering, is there a specific reason why we’re a bit inconsistent in using the context manager or not in this file?
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r932004699
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
The `list[str]` syntax works out of the box on 3.9+ so the `from __future__ import annotation` line is not needed on those platforms. For 3.8 users, if they do blind copy-and-paste on partial code they get NameError due to `List` not being defined anyway, and if they know enough to copy the typing import, they probably can also notice the lack of `__future__` import.
--
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] josh-fell commented on pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25242:
URL: https://github.com/apache/airflow/pull/25242#issuecomment-1195478435
> I am afraid we have circular import somewhere.
Yeah unfortunately. Thanks for keeping an eye on this. I'm fixing this now. Fingers crossed.
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r927988882
##########
airflow/decorators/base.py:
##########
@@ -531,6 +531,9 @@ def expand(self, **kwargs: "Mappable") -> XComArg:
def partial(self, **kwargs: Any) -> "Task[Function]":
...
+ def override(self, **kwargs: Any) -> "_TaskDecorator[Function, OperatorSubclass]":
Review Comment:
Hmm mypy is, in fact, not happy for `TaskDecorator` now (unlike `Task` which this change is addressing). This is different than the local static check runs which do not have any issues.
--
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 #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r931191732
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
I think we are very close to releasing 2.4 and I think just upgrading everything to __future__ was a good idea - to make sure we have a nice cut-off point and to avoid cherry-picking problems to 2.3.2. **maybe** we are close to the cut-off point already?
--
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] josh-fell commented on a diff in pull request #25242: Update core example DAGs to use `@task.branch` decorator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25242:
URL: https://github.com/apache/airflow/pull/25242#discussion_r931186044
##########
airflow/example_dags/example_branch_operator_decorator.py:
##########
@@ -16,10 +16,12 @@
# specific language governing permissions and limitations
# under the License.
-"""Example DAG demonstrating the usage of the BranchPythonOperator."""
+"""Example DAG demonstrating the usage of the ``@task.branch`` TaskFlow API decorator."""
import random
-from datetime import datetime
+from typing import List
Review Comment:
Interesting idea. Since there is a _lot_ of blind copy/paste from examples b users, do we want `from __future__ import annotation` to be a pattern users should follow? This would be my only concern really.
--
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