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/01/10 21:47:17 UTC
[GitHub] [airflow] ashb opened a new pull request #20795: Fix remaining mypy issues in "core" Airflow
ashb opened a new pull request #20795:
URL: https://github.com/apache/airflow/pull/20795
This fixes the remaining Mypy errors for "core" Airflow, i.e. excluding anything in a provider, or its tests
Related #19891
Before: Found 144 errors in 48 files (checked 2707 source files); Found 11 errors in 5 files
After: Found 102 errors in 29 files (checked 2707 source files); Found 2 errors in 1 file (checked 384 source files)
<!--
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] ashb commented on pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1009410162
Looks like I somehow triggered: `airflow.utils.context.AirflowContextDeprecationWarning: Accessing 'execution_date' from the template is deprecated and will be removed in a future version. Please use 'data_interval_start' or 'logical_date' instead.` all over the shop
I'll look at what I did to cause that tomorrow.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782100284
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,7 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
+ if self._wildcard and is_context_instance(self._kwargs):
return lazy_mapping_from_context(self._kwargs)
Review comment:
So either we need a type ignore on this line, or a cast.
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20795:
URL: https://github.com/apache/airflow/pull/20795
--
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] kaxil commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781606972
##########
File path: airflow/executors/base_executor.py
##########
@@ -55,7 +55,7 @@ class BaseExecutor(LoggingMixin):
``0`` for infinity
"""
- job_id: Optional[str] = None
+ job_id: Optional[int] = None
Review comment:
We pass this as a `str` (from Backfill and Manual triggers) and `int` from Scheduler
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/jobs/backfill_job.py#L807
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/jobs/scheduler_job.py#L662
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/jobs/base_job.py#L56
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782987340
##########
File path: airflow/models/baseoperator.py
##########
@@ -1600,6 +1601,10 @@ def get_serialized_fields(cls):
return cls.__serialized_fields
+ def serialize_for_task_group(self) -> Tuple[DagAttributeTypes, Any]:
Review comment:
nice
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781953855
##########
File path: airflow/decorators/task_group.py
##########
@@ -61,7 +61,7 @@ def __attrs_post_init__(self):
def _make_task_group(self, **kwargs) -> TaskGroup:
return TaskGroup(**kwargs)
- def __call__(self, *args, **kwargs) -> R:
+ def __call__(self, *args, **kwargs) -> Union[R, TaskGroup]:
Review comment:
I’m wondering maybe `TaskGroupDecorator` does not need to be a generic and `R` can all be replaced with `DependencyMixin`.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782713312
##########
File path: airflow/executors/base_executor.py
##########
@@ -55,7 +55,7 @@ class BaseExecutor(LoggingMixin):
``0`` for infinity
"""
- job_id: Optional[str] = None
+ job_id: Optional[int] = None
Review comment:
Changing this to `int | str | None` affects quite a few things to make the changes non-trivial, so I created a separate PR for easier review: astronomer/airflow#1481.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782007848
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
We can always ask for reassumption of the vote BTW if we have strong 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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782993963
##########
File path: airflow/utils/log/logging_mixin.py
##########
@@ -73,11 +73,23 @@ def supports_external_link(self) -> bool:
"""Return whether handler is able to support external links."""
-class StreamLogWriter(IOBase):
+class StreamLogWriter(IO[str], IOBase):
"""Allows to redirect stdout and stderr to logger"""
encoding: None = None
+ def fileno(self) -> int:
Review comment:
Hmmm. I had the impression this has been here 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] potiuk commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783059967
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,10 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- # Context is a TypedDict at lint time, and Mypy would complain it cannot
- # be used in isinstance. But the call works at runtime since the type is
- # actually implemented as a custom mapping, so we ignore the Mypy error.
- if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
+ if self._wildcard and isinstance(self._kwargs, Context):
Review comment:
```
pre-commit run mypy --all-files 2>&1 | sed -r "s/\x1B\[([0-9]{1,2}(;[0-9]{1,2})?)?[mGK]//g" | grep "error:" | wc
105 292 8934
```
BTW. The numbers might not add up. It's quite possible that fixing a mypy error in "selective" check will generate more errors elsewhere (in providers for example)
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783023128
##########
File path: airflow/version.py
##########
@@ -22,7 +22,7 @@
try:
import importlib_metadata as metadata
except ImportError:
- from importlib import metadata
+ from importlib import metadata # type: ignore
Review comment:
You mean `no-redef` :)
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782999465
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,7 +164,10 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
+ # Context is a TypedDict at lint time, and Mypy would complain it cannot
+ # be used in isinstance. But the call works at runtime since the type is
+ # actually implemented as a custom mapping, so we ignore the Mypy error.
+ if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
Review comment:
Might be good also to review other type-ignores to see if we can silence a specific error (watch-out for `misc`)
--
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] kaxil commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781606972
##########
File path: airflow/executors/base_executor.py
##########
@@ -55,7 +55,7 @@ class BaseExecutor(LoggingMixin):
``0`` for infinity
"""
- job_id: Optional[str] = None
+ job_id: Optional[int] = None
Review comment:
We pass this as a `str` (from Backfill and Manual triggers) and `int` from Scheduler
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/jobs/backfill_job.py#L807
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/cli/commands/task_command.py#L172
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/jobs/scheduler_job.py#L662
https://github.com/apache/airflow/blob/5980d2b05eee484256c634d5efae9410265c65e9/airflow/jobs/base_job.py#L56
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782007028
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
Well. What could I say :) . I was outvoted with that one :). So now there are the two of us.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781955838
##########
File path: airflow/jobs/scheduler_job.py
##########
@@ -1140,9 +1140,9 @@ def _send_sla_callbacks_to_processor(self, dag: DAG):
def _emit_pool_metrics(self, session: Session = None) -> None:
pools = models.Pool.slots_stats(session=session)
for pool_name, slot_stats in pools.items():
- Stats.gauge(f'pool.open_slots.{pool_name}', slot_stats[PoolSlotState.OPEN.value])
- Stats.gauge(f'pool.queued_slots.{pool_name}', slot_stats[PoolSlotState.QUEUED.value])
- Stats.gauge(f'pool.running_slots.{pool_name}', slot_stats[PoolSlotState.RUNNING.value])
+ Stats.gauge(f'pool.open_slots.{pool_name}', slot_stats["open"])
Review comment:
But it’s too stupid to know enum values are constants anyway LOL
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781961074
##########
File path: airflow/models/taskinstance.py
##########
@@ -2095,8 +2096,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
html_content_err = jinja_env.from_string(default_html_content_err).render(**jinja_context)
else:
- jinja_context = self.get_template_context()
- jinja_context.update(additional_context)
+ jinja_context = {**cast(dict, self.get_template_context()), **additional_context}
Review comment:
(I've got another case of this inside utils.helpers too0
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783028253
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,7 +164,10 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
+ # Context is a TypedDict at lint time, and Mypy would complain it cannot
+ # be used in isinstance. But the call works at runtime since the type is
+ # actually implemented as a custom mapping, so we ignore the Mypy error.
+ if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
Review comment:
Have gone through all the ones added in this PR and converted them to specific codes where possible.
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011843784
Flay test fixed in https://github.com/apache/airflow/pull/20843
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782015577
##########
File path: airflow/jobs/scheduler_job.py
##########
@@ -1140,9 +1140,9 @@ def _send_sla_callbacks_to_processor(self, dag: DAG):
def _emit_pool_metrics(self, session: Session = None) -> None:
pools = models.Pool.slots_stats(session=session)
for pool_name, slot_stats in pools.items():
- Stats.gauge(f'pool.open_slots.{pool_name}', slot_stats[PoolSlotState.OPEN.value])
- Stats.gauge(f'pool.queued_slots.{pool_name}', slot_stats[PoolSlotState.QUEUED.value])
- Stats.gauge(f'pool.running_slots.{pool_name}', slot_stats[PoolSlotState.RUNNING.value])
+ Stats.gauge(f'pool.open_slots.{pool_name}', slot_stats["open"])
Review comment:
https://github.com/python/mypy/issues/8722
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781992436
##########
File path: airflow/providers_manager.py
##########
@@ -571,10 +571,10 @@ def _add_taskflow_decorator(self, name, decorator_class_name: str, provider_pack
if name in self._taskflow_decorators:
try:
existing = self._taskflow_decorators[name]
- other_name = f'{existing.__module__}.{existing.__name}'
+ other_name = f'{existing.__module__}.{existing.__name__}'
Review comment:
wat
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781938763
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
The most generally accepted way to work around this is to actually use `assert` _without_ the if-guard; this is strictly for the linters, so `assert` being optimised in `-O` is totally a good thing. This is why Airflow’s “never use assert ever” rule has not sit well with me; assert exists for a reason, and _this is exactly 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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782104131
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,7 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
+ if self._wildcard and is_context_instance(self._kwargs):
return lazy_mapping_from_context(self._kwargs)
Review comment:
Ah right because if you wrap `isinstance()` inside a function Mypy can no longer recognise 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] kaxil commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781896046
##########
File path: airflow/executors/base_executor.py
##########
@@ -55,7 +55,7 @@ class BaseExecutor(LoggingMixin):
``0`` for infinity
"""
- job_id: Optional[str] = None
+ job_id: Optional[int] = None
Review comment:
Yeah
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011064541
Still few nits, but it's good enough :)
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783792398
##########
File path: airflow/models/taskmixin.py
##########
@@ -114,6 +115,14 @@ class DAGNode(DependencyMixin, metaclass=ABCMeta):
def node_id(self) -> str:
raise NotImplementedError()
+ @property
+ def label(self) -> Optional[str]:
+ tg: Optional["TaskGroup"] = getattr(self, 'task_group', None)
+ if tg and tg.node_id and tg.prefix_group_id:
+ # "task_group_id.task_id" -> "task_id"
+ return self.node_id[len(tg.node_id) + 1 :]
Review comment:
Makes 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 commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781621630
##########
File path: airflow/models/taskinstance.py
##########
@@ -1286,7 +1287,7 @@ def check_and_change_state_before_execution(
return True
def _date_or_empty(self, attr: str):
- result = getattr(self, attr, None) # type: datetime
+ result = getattr(self, attr, None) # type: Optional[datetime]
Review comment:
Yes, that works.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782015800
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -975,7 +975,7 @@ def serialize_task_group(cls, task_group: TaskGroup) -> Optional[Dict[str, Any]]
"children": {
label: (DAT.OP, child.task_id)
if isinstance(child, BaseOperator)
- else (DAT.TASK_GROUP, SerializedTaskGroup.serialize_task_group(child))
+ else (DAT.TASK_GROUP, SerializedTaskGroup.serialize_task_group(cast("TaskGroup", child)))
for label, child in task_group.children.items()
},
Review comment:
This feels like a good candidate for poylmorphism. If we have add an abstract method `serialize_for_task_group` on `DAGNode`, we can do
```python
"children": {
label: child.serialize_for_task_group()
for label, child in task_group.children.items()
}
```
to both avoid the `cast`, and better prevent bugs if we add more `DAGNode` subclasses in 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] uranusjr commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782012495
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,8 +164,10 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
- return lazy_mapping_from_context(self._kwargs)
+ # We have a typestub that says Context is a TypedDict, but at runtime it's actually a custom
+ # MutableMapping class.
+ if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
+ return lazy_mapping_from_context(self._kwargs) # type: ignore
Review comment:
What’s the error here? I was able to remove both `type: ignore` comments locally and have pre-commit pass.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782995769
##########
File path: airflow/version.py
##########
@@ -22,7 +22,7 @@
try:
import importlib_metadata as metadata
except ImportError:
- from importlib import metadata
+ from importlib import metadata # type: ignore
Review comment:
```suggestion
from importlib import metadata # type: ignore[attr-defined]
```
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783021187
##########
File path: airflow/providers_manager.py
##########
@@ -571,10 +571,10 @@ def _add_taskflow_decorator(self, name, decorator_class_name: str, provider_pack
if name in self._taskflow_decorators:
try:
existing = self._taskflow_decorators[name]
- other_name = f'{existing.__module__}.{existing.__name}'
+ other_name = f'{existing.__module__}.{existing.__name__}'
except Exception:
# If problem importing, then get the value from the functools.partial
- other_name = self._taskflow_decorators._raw_dict[name].args[0]
+ other_name = self._taskflow_decorators._raw_dict[name].args[0] # type: ignore
Review comment:
I'll add `show_error_codes=True` in to the config in this PR 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 a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783014315
##########
File path: airflow/utils/log/logging_mixin.py
##########
@@ -73,11 +73,23 @@ def supports_external_link(self) -> bool:
"""Return whether handler is able to support external links."""
-class StreamLogWriter(IOBase):
+class StreamLogWriter(IO[str], IOBase):
"""Allows to redirect stdout and stderr to logger"""
encoding: None = None
+ def fileno(self) -> int:
Review comment:
I thought so too but it complained without it -- this was mostly around the use in StdoutRedirector which wants this to be `IO[str]`
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781581643
##########
File path: airflow/jobs/scheduler_job.py
##########
@@ -1140,9 +1140,9 @@ def _send_sla_callbacks_to_processor(self, dag: DAG):
def _emit_pool_metrics(self, session: Session = None) -> None:
pools = models.Pool.slots_stats(session=session)
for pool_name, slot_stats in pools.items():
- Stats.gauge(f'pool.open_slots.{pool_name}', slot_stats[PoolSlotState.OPEN.value])
- Stats.gauge(f'pool.queued_slots.{pool_name}', slot_stats[PoolSlotState.QUEUED.value])
- Stats.gauge(f'pool.running_slots.{pool_name}', slot_stats[PoolSlotState.RUNNING.value])
+ Stats.gauge(f'pool.open_slots.{pool_name}', slot_stats["open"])
Review comment:
Mypy complained about this as `slot_stats` is a TypedDict, so I removed this and deleted the PoolSlotState, as this is the _only_ place this class is used.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783088432
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,10 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- # Context is a TypedDict at lint time, and Mypy would complain it cannot
- # be used in isinstance. But the call works at runtime since the type is
- # actually implemented as a custom mapping, so we ignore the Mypy error.
- if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
+ if self._wildcard and isinstance(self._kwargs, Context):
Review comment:
Possibly other fix fixed it. Also while MyPy is stable and repeatable, it is somewhat "vulnerable" to small changes - it generates more or less loosely related errors after small changes that should not (at least obviously) trigger them - at least that's the impression I had sometimes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782041271
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,8 +164,10 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
- return lazy_mapping_from_context(self._kwargs)
+ # We have a typestub that says Context is a TypedDict, but at runtime it's actually a custom
+ # MutableMapping class.
+ if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
+ return lazy_mapping_from_context(self._kwargs) # type: ignore
Review comment:
I got an error saying something to the effect of "isistance doesn't work on TypedDict".
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781619815
##########
File path: airflow/models/taskinstance.py
##########
@@ -1286,7 +1287,7 @@ def check_and_change_state_before_execution(
return True
def _date_or_empty(self, attr: str):
- result = getattr(self, attr, None) # type: datetime
+ result = getattr(self, attr, None) # type: Optional[datetime]
Review comment:
I'll see if that works -- I just added the `Optional[]` without really thinking/examining too much.
--
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] kaxil commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781609000
##########
File path: airflow/models/taskinstance.py
##########
@@ -1286,7 +1287,7 @@ def check_and_change_state_before_execution(
return True
def _date_or_empty(self, attr: str):
- result = getattr(self, attr, None) # type: datetime
+ result = getattr(self, attr, None) # type: Optional[datetime]
Review comment:
```suggestion
result: Optional[datetime] = getattr(self, attr, None)
```
nit: Feels more cleaner
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1010959908
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] potiuk commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783022283
##########
File path: airflow/providers_manager.py
##########
@@ -571,10 +571,10 @@ def _add_taskflow_decorator(self, name, decorator_class_name: str, provider_pack
if name in self._taskflow_decorators:
try:
existing = self._taskflow_decorators[name]
- other_name = f'{existing.__module__}.{existing.__name}'
+ other_name = f'{existing.__module__}.{existing.__name__}'
except Exception:
# If problem importing, then get the value from the functools.partial
- other_name = self._taskflow_decorators._raw_dict[name].args[0]
+ other_name = self._taskflow_decorators._raw_dict[name].args[0] # type: ignore
Review comment:
Ahhhhh YEAH!
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781619342
##########
File path: airflow/executors/base_executor.py
##########
@@ -55,7 +55,7 @@ class BaseExecutor(LoggingMixin):
``0`` for infinity
"""
- job_id: Optional[str] = None
+ job_id: Optional[int] = None
Review comment:
I did wonder if this might cause a problem. I guess it should be `int | str | None` then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781959287
##########
File path: airflow/models/taskinstance.py
##########
@@ -2095,8 +2096,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
html_content_err = jinja_env.from_string(default_html_content_err).render(**jinja_context)
else:
- jinja_context = self.get_template_context()
- jinja_context.update(additional_context)
+ jinja_context = {**cast(dict, self.get_template_context()), **additional_context}
Review comment:
This is where the deprecations are from, I’ll fix 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] ashb commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781960498
##########
File path: airflow/models/taskinstance.py
##########
@@ -2095,8 +2096,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
html_content_err = jinja_env.from_string(default_html_content_err).render(**jinja_context)
else:
- jinja_context = self.get_template_context()
- jinja_context.update(additional_context)
+ jinja_context = {**cast(dict, self.get_template_context()), **additional_context}
Review comment:
Yeah, I'm on it already @uranusjr
--
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] kaxil commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781902068
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
We could also probably do this _I think_ (not tested) -- as we have marked `SubDagOperator.subdag` of type `DAG`.
```python
current_task = cast(SubDagOperator, current_task)
```
or
```python
current_task.subdag = cast(DAG, current_task.subdag)
```
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782997629
##########
File path: airflow/providers_manager.py
##########
@@ -571,10 +571,10 @@ def _add_taskflow_decorator(self, name, decorator_class_name: str, provider_pack
if name in self._taskflow_decorators:
try:
existing = self._taskflow_decorators[name]
- other_name = f'{existing.__module__}.{existing.__name}'
+ other_name = f'{existing.__module__}.{existing.__name__}'
except Exception:
# If problem importing, then get the value from the functools.partial
- other_name = self._taskflow_decorators._raw_dict[name].args[0]
+ other_name = self._taskflow_decorators._raw_dict[name].args[0] # type: ignore
Review comment:
Not sure which error is silenced here but we started to silence specific error codes recently: https://mypy.readthedocs.io/en/stable/error_code_list.html#error-code-list
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011076700
Do we want to use this as the fix for `airflow/dag_processing` rather than #20470?
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783029207
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,10 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- # Context is a TypedDict at lint time, and Mypy would complain it cannot
- # be used in isinstance. But the call works at runtime since the type is
- # actually implemented as a custom mapping, so we ignore the Mypy error.
- if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
+ if self._wildcard and isinstance(self._kwargs, Context):
Review comment:
Despite mypy complaining about this before _in this PR_ it's now not, at least not for me when I run `pre-commit run -a mypy` locally.
I want to see what number CI gets -- it should be 102 errors left for the first mypy job.
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011482747
> > @josh-fell Oh I like https://github.com/apache/airflow/pull/20470/files#diff-af25dd96233a97ace811c614fa7d5bd059cbbc1571e421f6675e16f6290814c5 more than adding all those "not implemeneted errors" I did.
>
> Ah yeah... So I remembered well then it was already dealt with - I helped @josh-fell then. The style of the comment clearly indicate it was my fixup to @josh-fell 's change :)
Oh definitely. I can't take credit for that at all 🙂 . Shall I close #20470 then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781987743
##########
File path: airflow/decorators/task_group.py
##########
@@ -61,7 +61,7 @@ def __attrs_post_init__(self):
def _make_task_group(self, **kwargs) -> TaskGroup:
return TaskGroup(**kwargs)
- def __call__(self, *args, **kwargs) -> R:
+ def __call__(self, *args, **kwargs) -> Union[R, TaskGroup]:
Review comment:
```
@task_group
def foo() -> BashOperator
return BashOperator(task_id='a')
bash_op = foo()
```
--
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 edited a comment on pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
josh-fell edited a comment on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011482747
> > @josh-fell Oh I like https://github.com/apache/airflow/pull/20470/files#diff-af25dd96233a97ace811c614fa7d5bd059cbbc1571e421f6675e16f6290814c5 more than adding all those "not implemeneted errors" I did.
>
> Ah yeah... So I remembered well then it was already dealt with - I helped @josh-fell then. The style of the comment clearly indicate it was my fixup to @josh-fell 's change :)
Oh definitely. I can't take credit for that at all 🙂. Shall I close #20470 then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783085692
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,10 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- # Context is a TypedDict at lint time, and Mypy would complain it cannot
- # be used in isinstance. But the call works at runtime since the type is
- # actually implemented as a custom mapping, so we ignore the Mypy error.
- if self._wildcard and isinstance(self._kwargs, Context): # type: ignore
+ if self._wildcard and isinstance(self._kwargs, Context):
Review comment:
Confirmed it's fine now. I have _no_ idea what was going on before but this was def a problem.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782982377
##########
File path: airflow/executors/base_executor.py
##########
@@ -55,7 +55,7 @@ class BaseExecutor(LoggingMixin):
``0`` for infinity
"""
- job_id: Optional[str] = None
+ job_id: Union[None, int, str] = None
Review comment:
Maybe:
```suggestion
job_id: Optional[Union[int, str]] = None
```
A bit longer, but I think conceptually more consistent with other optionals.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782996345
##########
File path: airflow/plugins_manager.py
##########
@@ -24,12 +24,12 @@
import os
import sys
import types
-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Type
+from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Type
try:
import importlib_metadata
except ImportError:
- from importlib import metadata as importlib_metadata
+ from importlib import metadata as importlib_metadata # type: ignore
Review comment:
```suggestion
from importlib import metadata as importlib_metadata # type: ignore[attr-defined]
```
--
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] kaxil commented on a change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781602618
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
Why this?
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781618480
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
Because mypy doesn't/can't know that `isinstance(current_task, SubDagOperator)` means `current_task.subdag` will be non-None as a result, so that's what this assert gives us.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r781989049
##########
File path: airflow/decorators/task_group.py
##########
@@ -61,7 +61,7 @@ def __attrs_post_init__(self):
def _make_task_group(self, **kwargs) -> TaskGroup:
return TaskGroup(**kwargs)
- def __call__(self, *args, **kwargs) -> R:
+ def __call__(self, *args, **kwargs) -> Union[R, TaskGroup]:
Review comment:
It maybe doesn't _need_ to be, but it feels like it should be.
The "Union" is because I couldn't find a way of doing an overload on a class-level generic.
THe ideal is: If R is None return, then the return type of call is TaskGroup, else it's R
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782097858
##########
File path: airflow/api/common/experimental/mark_tasks.py
##########
@@ -194,13 +196,15 @@ def get_subdag_runs(
current_task = current_dag.get_task(task_id)
if isinstance(current_task, SubDagOperator) or current_task.task_type == "SubDagOperator":
+ if TYPE_CHECKING:
+ assert current_task.subdag
Review comment:
I'm more than happy to have the assert without the `if` -- I think the main point of avoiding asserts at runtime is that the code must continue to function if the assert was never run (and so for most people the easiest thing is to avoid them entirely)
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782099478
##########
File path: airflow/utils/operator_helpers.py
##########
@@ -164,7 +164,7 @@ def determine(
def unpacking(self) -> Mapping[str, Any]:
"""Dump the kwargs mapping to unpack with ``**`` in a function call."""
- if self._wildcard and isinstance(self._kwargs, Context):
+ if self._wildcard and is_context_instance(self._kwargs):
return lazy_mapping_from_context(self._kwargs)
Review comment:
@uranusjr
```
airflow/utils/operator_helpers.py:168: error: Argument 1 to
"lazy_mapping_from_context" has incompatible type "Mapping[str, Any]"; expected
"Context"
return lazy_mapping_from_context(self._kwargs)
```
That is why I had this as a type: ignore
--
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 #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011376175
> @josh-fell Oh I like https://github.com/apache/airflow/pull/20470/files#diff-af25dd96233a97ace811c614fa7d5bd059cbbc1571e421f6675e16f6290814c5 more than adding all those "not implemeneted errors" I did.
Ah yeah... So I remembered well then it was already dealt with - I helped @josh-fell then. The style of the comment clearly indicate it was my fixup to @josh-fell 's change :)
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783786932
##########
File path: airflow/models/taskmixin.py
##########
@@ -114,6 +115,14 @@ class DAGNode(DependencyMixin, metaclass=ABCMeta):
def node_id(self) -> str:
raise NotImplementedError()
+ @property
+ def label(self) -> Optional[str]:
+ tg: Optional["TaskGroup"] = getattr(self, 'task_group', None)
+ if tg and tg.node_id and tg.prefix_group_id:
+ # "task_group_id.task_id" -> "task_id"
+ return self.node_id[len(tg.node_id) + 1 :]
Review comment:
This doesn't feel safe -- there are cases where there can be a dot in the task_id. There _shouldn't_ be, but I cant guarantee 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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r783675997
##########
File path: airflow/models/taskmixin.py
##########
@@ -114,6 +115,14 @@ class DAGNode(DependencyMixin, metaclass=ABCMeta):
def node_id(self) -> str:
raise NotImplementedError()
+ @property
+ def label(self) -> Optional[str]:
+ tg: Optional["TaskGroup"] = getattr(self, 'task_group', None)
+ if tg and tg.node_id and tg.prefix_group_id:
+ # "task_group_id.task_id" -> "task_id"
+ return self.node_id[len(tg.node_id) + 1 :]
Review comment:
```suggestion
return self.node_id.split(".", 1)[-1]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#issuecomment-1011119023
@josh-fell Oh I like https://github.com/apache/airflow/pull/20470/files#diff-af25dd96233a97ace811c614fa7d5bd059cbbc1571e421f6675e16f6290814c5 more than adding all those "not implemeneted errors" I did.
--
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 change in pull request #20795: Fix remaining mypy issues in "core" Airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20795:
URL: https://github.com/apache/airflow/pull/20795#discussion_r782988971
##########
File path: airflow/providers_manager.py
##########
@@ -571,10 +571,10 @@ def _add_taskflow_decorator(self, name, decorator_class_name: str, provider_pack
if name in self._taskflow_decorators:
try:
existing = self._taskflow_decorators[name]
- other_name = f'{existing.__module__}.{existing.__name}'
+ other_name = f'{existing.__module__}.{existing.__name__}'
Review comment:
ups.
--
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