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/08/30 20:13:57 UTC
[GitHub] [airflow] Jorricks opened a new pull request, #26077: FIX Broken typing information
Jorricks opened a new pull request, #26077:
URL: https://github.com/apache/airflow/pull/26077
When I was debugging the API and figuring out when which fields of a TaskInstance could be None for #26068, I stumbled across this code for which the typing information is incorrect.
It confused me completely and I even though this was some sort of magic function from SQLAlchemy at some point.
I figured this should be fixed so it hopefully doesn't confuse anyone else :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959193277
##########
airflow/models/dagrun.py:
##########
@@ -980,11 +980,11 @@ def create_ti(task: "Operator", indexes: Tuple[int, ...]) -> Generator:
def _create_tasks(
self,
dag: "DAG",
- task_creator: Callable,
- task_filter: Callable,
+ task_creator: Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]],
+ task_filter: Callable[[Operator], bool],
*,
session: Session,
- ) -> Iterable["Operator"]:
+ ) -> Union[Iterator[Dict[str, Any]], Iterator[TI]]:
Review Comment:
And this can be parametrized...
```python
CreatedTasksType = TypeVar("CreatedTasksType")
def _create_tasks(
self,
dag: "DAG",
task_creator: Callable,
task_filter: Callable,
task_creator: Callable[[Operator, Tuple[int, ...]], CreatedTasksType,
task_filter: Callable[[Operator], bool],
*,
session: Session,
) -> CreatedTasksType:
```
to avoid the `type: ignore` comments (I believe)
--
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 #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959432250
##########
airflow/models/dagrun.py:
##########
@@ -56,6 +57,7 @@
from sqlalchemy.orm import joinedload, relationship, synonym
from sqlalchemy.orm.session import Session
from sqlalchemy.sql.expression import false, select, true
+from typing_extensions import Literal, overload
Review Comment:
Use `airflow.typing_compat` instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #26077:
URL: https://github.com/apache/airflow/pull/26077
--
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] Jorricks commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
Jorricks commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959554610
##########
airflow/models/dagrun.py:
##########
@@ -839,7 +841,7 @@ def verify_integrity(
from airflow.settings import task_instance_mutation_hook
# Set for the empty default in airflow.settings -- if it's not set this means it has been changed
- hook_is_noop = getattr(task_instance_mutation_hook, 'is_noop', False)
+ hook_is_noop: Literal[True, False] = getattr(task_instance_mutation_hook, 'is_noop', False)
Review Comment:
Changing it back returns:
```
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
airflow/models/dagrun.py:867: error: No overload variant of "_get_task_creator"
of "DagRun" matches argument types "Dict[str, int]", "Callable[[Any], Any]",
"bool" [call-overload]
task_creator = self._get_task_creator(created_counts, task_ins...
^
airflow/models/dagrun.py:867: note: Possible overload variants:
airflow/models/dagrun.py:867: note: def _get_task_creator(self, created_counts: Dict[str, int], ti_mutation_hook: Callable[..., Any], hook_is_noop: Literal[True]) -> Callable[[Union[BaseOperator, MappedOperator], Tuple[int, ...]], Iterator[Dict[str, Any]]]
airflow/models/dagrun.py:867: note: def _get_task_creator(self, created_counts: Dict[str, int], ti_mutation_hook: Callable[..., Any], hook_is_noop: Literal[False]) -> Callable[[Union[BaseOperator, MappedOperator], Tuple[int, ...]], Iterator[TaskInstance]]
Found 1 error in 1 file (checked 1 source 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] Jorricks commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
Jorricks commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959245310
##########
airflow/models/dagrun.py:
##########
@@ -980,11 +980,11 @@ def create_ti(task: "Operator", indexes: Tuple[int, ...]) -> Generator:
def _create_tasks(
self,
dag: "DAG",
- task_creator: Callable,
- task_filter: Callable,
+ task_creator: Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]],
+ task_filter: Callable[[Operator], bool],
*,
session: Session,
- ) -> Iterable["Operator"]:
+ ) -> Union[Iterator[Dict[str, Any]], Iterator[TI]]:
Review Comment:
Alright makes sense. But the arguments should occur only once right?
--
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] Jorricks commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
Jorricks commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959549213
##########
airflow/models/dagrun.py:
##########
@@ -839,7 +841,7 @@ def verify_integrity(
from airflow.settings import task_instance_mutation_hook
# Set for the empty default in airflow.settings -- if it's not set this means it has been changed
- hook_is_noop = getattr(task_instance_mutation_hook, 'is_noop', False)
+ hook_is_noop: Literal[True, False] = getattr(task_instance_mutation_hook, 'is_noop', False)
Review Comment:
I tried, it didn't unfortunately.
It was not able to match the bool to Literal[True] or Literal[False] for the overloads.
##########
airflow/models/dagrun.py:
##########
@@ -56,6 +57,7 @@
from sqlalchemy.orm import joinedload, relationship, synonym
from sqlalchemy.orm.session import Session
from sqlalchemy.sql.expression import false, select, true
+from typing_extensions import Literal, overload
Review Comment:
Let me change that.
--
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] Jorricks commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
Jorricks commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959549213
##########
airflow/models/dagrun.py:
##########
@@ -839,7 +841,7 @@ def verify_integrity(
from airflow.settings import task_instance_mutation_hook
# Set for the empty default in airflow.settings -- if it's not set this means it has been changed
- hook_is_noop = getattr(task_instance_mutation_hook, 'is_noop', False)
+ hook_is_noop: Literal[True, False] = getattr(task_instance_mutation_hook, 'is_noop', False)
Review Comment:
I tried, it didn't unfortunately.
--
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 #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959248393
##########
airflow/models/dagrun.py:
##########
@@ -980,11 +980,11 @@ def create_ti(task: "Operator", indexes: Tuple[int, ...]) -> Generator:
def _create_tasks(
self,
dag: "DAG",
- task_creator: Callable,
- task_filter: Callable,
+ task_creator: Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]],
+ task_filter: Callable[[Operator], bool],
*,
session: Session,
- ) -> Iterable["Operator"]:
+ ) -> Union[Iterator[Dict[str, Any]], Iterator[TI]]:
Review Comment:
Oh yes, I copy-pasted them from the diff incorrectly 😓
--
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] Jorricks commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
Jorricks commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959244886
##########
airflow/models/dagrun.py:
##########
@@ -945,7 +945,7 @@ def _check_for_removed_or_restored_tasks(
def _get_task_creator(
self, created_counts: Dict[str, int], ti_mutation_hook: Callable, hook_is_noop: bool
- ) -> Callable:
+ ) -> Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]]:
Review Comment:
Nice one! I never really got around to using `overloads`, but this is a very nice use case for it indeed.
Will change it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959433090
##########
airflow/models/dagrun.py:
##########
@@ -839,7 +841,7 @@ def verify_integrity(
from airflow.settings import task_instance_mutation_hook
# Set for the empty default in airflow.settings -- if it's not set this means it has been changed
- hook_is_noop = getattr(task_instance_mutation_hook, 'is_noop', False)
+ hook_is_noop: Literal[True, False] = getattr(task_instance_mutation_hook, 'is_noop', False)
Review Comment:
I believe `bool` would work, Mypy should be smart enough to automatically coerce.
--
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] Jorricks commented on a diff in pull request #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
Jorricks commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959560016
##########
airflow/models/dagrun.py:
##########
@@ -839,7 +841,7 @@ def verify_integrity(
from airflow.settings import task_instance_mutation_hook
# Set for the empty default in airflow.settings -- if it's not set this means it has been changed
- hook_is_noop = getattr(task_instance_mutation_hook, 'is_noop', False)
+ hook_is_noop: Literal[True, False] = getattr(task_instance_mutation_hook, 'is_noop', False)
Review Comment:
Added a note to be more explicit why it has to be `Literal[True, False]`
--
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 #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959191405
##########
airflow/models/dagrun.py:
##########
@@ -945,7 +945,7 @@ def _check_for_removed_or_restored_tasks(
def _get_task_creator(
self, created_counts: Dict[str, int], ti_mutation_hook: Callable, hook_is_noop: bool
- ) -> Callable:
+ ) -> Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]]:
Review Comment:
It’s probably a good idea to make this an overload
```python
@overload
def _get_task_creator(
self,
created_counts: Dict[str, int],
ti_mutation_hook: Callable,
hook_is_noop: Literal[True],
) -> Callable[[Operator, Tuple[int, ...]], Iterator[Dict[str, Any]]]:
...
@overload
def _get_task_creator(
self,
created_counts: Dict[str, int],
ti_mutation_hook: Callable,
hook_is_noop: Literal[False],
) -> Callable[[Operator, Tuple[int, ...]], Iterator[TI]]:
...
def _get_task_creator(
self,
created_counts: Dict[str, int],
ti_mutation_hook: Callable,
hook_is_noop: bool,
) -> Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]]:
# Actual implementation...
```
--
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 #26077: FIX Incorrect typing information
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26077:
URL: https://github.com/apache/airflow/pull/26077#discussion_r959193277
##########
airflow/models/dagrun.py:
##########
@@ -980,11 +980,11 @@ def create_ti(task: "Operator", indexes: Tuple[int, ...]) -> Generator:
def _create_tasks(
self,
dag: "DAG",
- task_creator: Callable,
- task_filter: Callable,
+ task_creator: Callable[[Operator, Tuple[int, ...]], Union[Iterator[Dict[str, Any]], Iterator[TI]]],
+ task_filter: Callable[[Operator], bool],
*,
session: Session,
- ) -> Iterable["Operator"]:
+ ) -> Union[Iterator[Dict[str, Any]], Iterator[TI]]:
Review Comment:
And this can be parametrized...
```python
CreatedTasksType = TypeVar("CreatedTasksType")
def _create_tasks(
self,
dag: "DAG",
task_creator: Callable,
task_filter: Callable,
task_creator: Callable[[Operator, Tuple[int, ...]], CreatedTasksType],
task_filter: Callable[[Operator], bool],
*,
session: Session,
) -> CreatedTasksType:
```
to avoid the `type: ignore` comments (I believe)
--
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