You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ashb (via GitHub)" <gi...@apache.org> on 2023/02/09 18:17:35 UTC
[GitHub] [airflow] ashb opened a new pull request, #29445: Better handle forward type definitions in `@task.python` for multiple output
ashb opened a new pull request, #29445:
URL: https://github.com/apache/airflow/pull/29445
This does two things:
1) it only looks at the return type annotation, not everything; and
2) it catches and tests for an invalid/TYPE_CHECKING-only type import
Closes #29435
--
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 #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "josh-fell (via GitHub)" <gi...@apache.org>.
josh-fell commented on code in PR #29445:
URL: https://github.com/apache/airflow/pull/29445#discussion_r1102809860
##########
airflow/decorators/base.py:
##########
@@ -296,7 +297,20 @@ class _TaskDecorator(ExpandableFactory, Generic[FParams, FReturn, OperatorSubcla
@multiple_outputs.default
def _infer_multiple_outputs(self):
try:
- return_type = typing_extensions.get_type_hints(self.function).get("return", Any)
+ # We only care about the return annotation, not anything about the parameters
+ def fake():
+ ...
+
+ fake.__annotations__ = {"return": self.function.__annotations__["return"]}
+
+ return_type = typing_extensions.get_type_hints(fake, self.function.__globals__).get("return", Any)
+ except NameError as e:
+ warnings.warn(
+ "Cannot infer multiple_outputs with forward type references that are not imported. "
Review Comment:
The warning is nice. I think it would be helpful for the message to include the function name too. WDYT?
Something like:
```suggestion
f"Cannot infer multiple_outputs for TaskFlow function {self.function.__name__!r} with forward type references that are not imported. "
```
##########
airflow/decorators/base.py:
##########
@@ -296,7 +297,20 @@ class _TaskDecorator(ExpandableFactory, Generic[FParams, FReturn, OperatorSubcla
@multiple_outputs.default
def _infer_multiple_outputs(self):
try:
- return_type = typing_extensions.get_type_hints(self.function).get("return", Any)
+ # We only care about the return annotation, not anything about the parameters
+ def fake():
+ ...
+
+ fake.__annotations__ = {"return": self.function.__annotations__["return"]}
Review Comment:
```suggestion
fake.__annotations__ = {"return": self.function.__annotations__.get("return")}
```
Just in case there isn't a return annotation.
--
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 pull request #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1427067951
Oh weird just on 3.8 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] ashb commented on pull request #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1427074832
Oh 3.8+, not just 3.8. That makes more sense
--
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 merged pull request #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb merged PR #29445:
URL: https://github.com/apache/airflow/pull/29445
--
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 #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29445:
URL: https://github.com/apache/airflow/pull/29445#discussion_r1102828840
##########
airflow/decorators/base.py:
##########
@@ -296,7 +297,20 @@ class _TaskDecorator(ExpandableFactory, Generic[FParams, FReturn, OperatorSubcla
@multiple_outputs.default
def _infer_multiple_outputs(self):
try:
- return_type = typing_extensions.get_type_hints(self.function).get("return", Any)
+ # We only care about the return annotation, not anything about the parameters
+ def fake():
+ ...
+
+ fake.__annotations__ = {"return": self.function.__annotations__["return"]}
+
+ return_type = typing_extensions.get_type_hints(fake, self.function.__globals__).get("return", Any)
+ except NameError as e:
+ warnings.warn(
+ "Cannot infer multiple_outputs with forward type references that are not imported. "
Review Comment:
Oh yes good call.
--
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 #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1427071957
Stack traces are slightly different on 3.8+ that's why.
--
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 #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "josh-fell (via GitHub)" <gi...@apache.org>.
josh-fell commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1425881254
Looks cool to me!
--
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 #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "josh-fell (via GitHub)" <gi...@apache.org>.
josh-fell commented on code in PR #29445:
URL: https://github.com/apache/airflow/pull/29445#discussion_r1103234842
##########
airflow/decorators/base.py:
##########
@@ -295,8 +296,25 @@ class _TaskDecorator(ExpandableFactory, Generic[FParams, FReturn, OperatorSubcla
@multiple_outputs.default
def _infer_multiple_outputs(self):
+ if "return" not in self.function.__annotations__:
Review Comment:
Yep this is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1424624253
What do you think @josh-fell ?
--
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 #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1427086605
Yeah. Our PY38 is slightly misleading as it really means PY38_PLUS
--
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 pull request #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1427017838
I think this PR is causing failures in main:
`tests/decorators/test_python.py::TestAirflowTaskDecorator::test_infer_multiple_outputs_forward_annotation - assert 115 == 118`
example:
https://github.com/apache/airflow/actions/runs/4156131743/jobs/7189733639#step:6:6938
--
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 pull request #29445: Better handle forward type definitions in `@task.python` for multiple output
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29445:
URL: https://github.com/apache/airflow/pull/29445#issuecomment-1427018005
A fix is here https://github.com/apache/airflow/pull/29489
sorry missed 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