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 2021/12/23 09:30:15 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #20482: Fix mypy errors in airflow/utils/
ephraimbuddy opened a new pull request #20482:
URL: https://github.com/apache/airflow/pull/20482
---
**^ 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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774438171
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict):
Review comment:
Not sure of this one cc: @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] potiuk commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776304364
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict, total=False): # type: ignore
Review comment:
`total=False` is Python 3.8 + feature https://docs.python.org/3/whatsnew/3.8.html
Not sure if it will be really helpful. I'd say we should specify all possible keys in Context.
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774441327
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
context["try_number"] = try_number
return render_template_to_string(self.filename_jinja_template, context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.get_dagrun().logical_date.isoformat(),
- try_number=try_number,
- )
+ elif self.filename_template:
+ return self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.get_dagrun().logical_date.isoformat(),
+ try_number=try_number,
+ )
+ else:
+ return "" # mypy
Review comment:
This is also another place I'm confused about the best approach to it and whether to work on the `parse_template_string` itself
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776309141
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
I think we need additional overload of `make_aware` with `DateTime` that should fix it. https://mypy.readthedocs.io/en/stable/more_types.html#function-overloading
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774504965
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
Review comment:
I guess "total = False" came from here. I'd say we should have a way to create "default" Context with all the fields present with default values and override the fields needed.
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774781268
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
Review comment:
But removing `total=False` _shouldn’t_ work here—it should be the other way around because this requires non-totality! We should keep `total=False` and find another solution.
https://www.python.org/dev/peps/pep-0589/#totality
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774780665
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
Review comment:
Yeah I think we should refactor `Context.__init__()` and how it interacts with `TaskInstance.get_template_context()`. Let’s have this now and I’ll do the refactoring later.
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774515035
##########
File path: airflow/utils/timezone.py
##########
@@ -133,10 +138,10 @@ def make_aware(value: Optional[dt.datetime], timezone: Optional["Timezone"] = No
value = value.replace(fold=1)
if hasattr(timezone, 'localize'):
# This method is available for pytz time zones.
- return timezone.localize(value)
+ return cast(Any, timezone).localize(value)
Review comment:
There is a discussion about MyPy supporting this case here: https://github.com/python/mypy/issues/1424
One of the nice solutions suggested in the thred as a workaround would translate to:
```
localize = getattr(timezone, 'localize', None)
if localize is not None:
return localize(value)
```
--
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] ephraimbuddy commented on pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#issuecomment-1002445739
> > > Duplication of effort here #20504 ...
> > > I wish we all mention/reference these changes here #19891 as per convention so to avoid time waste.
> >
> >
> > My bad...
>
> No worries. I will close that. Please take a look to that anyway to see if you want to amend or express anything here differently, particularly those overloadings. I am not sure why they are necessary
The overloads are because we don't want a breaking change here(_I think_).
--
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] khalidmammadov commented on pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#issuecomment-1002267626
> > Duplication of effort here #20504 ...
> > I wish we all mention/reference these changes here #19891 as per convention so to avoid time waste.
>
> My bad...
No worries. I will close that. Please take a look to that anyway to see if you want to amend or express anything here differently, particularly those overloadings. I am not sure why they are necessary
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776641028
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
I tried adding an additional overload of `make_aware` but mypy errors that it'll never be matched because of the arguments.
```
airflow/utils/timezone.py:113: error: Overloaded function signature 3 will
never be matched: signature 2's parameter type(s) are the same or broader
def make_aware(value: dt.datetime, timezone: Optional[dt.tzinfo] = N...
^
```
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774910472
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict, total=False): # type: ignore
Review comment:
Here:
```
airflow/utils/context.pyi:51: error: Unexpected keyword argument "total" for "__init_subclass__"
of "object"
class Context(TypedDict, total=False):
^
Found 1 error in 1 file (checked 60 source files)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774509120
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
context["try_number"] = try_number
return render_template_to_string(self.filename_jinja_template, context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.get_dagrun().logical_date.isoformat(),
- try_number=try_number,
- )
+ elif self.filename_template:
+ return self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.get_dagrun().logical_date.isoformat(),
+ try_number=try_number,
+ )
+ else:
+ return "" # mypy
Review comment:
The way `parse_template_string` works - it guarantess that either `filename_jinja_template` or `filename_template` is set. There is no way both will be None. The only "real" case where (currently) the if can be reached is if the "template_string" is "". But this is wrong anyway.
I'd say:
* raise RuntimeError in `parse_template_string` if `tamplate_string` is ""
* raise RuntimeError instead of `return ""'` - with "This should never happen" kind of message.
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774515035
##########
File path: airflow/utils/timezone.py
##########
@@ -133,10 +138,10 @@ def make_aware(value: Optional[dt.datetime], timezone: Optional["Timezone"] = No
value = value.replace(fold=1)
if hasattr(timezone, 'localize'):
# This method is available for pytz time zones.
- return timezone.localize(value)
+ return cast(Any, timezone).localize(value)
Review comment:
There is a discussion about MyPy supporting this case here: https://github.com/python/mypy/issues/1424
One of the nice solutions suggested in the thred as a workaround would translate to:
``
localize = getattr(timezone, 'localize', None)
if localize is not None:
return localize(value)
``
--
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] mik-laj commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776663344
##########
File path: airflow/utils/timezone.py
##########
@@ -29,7 +29,7 @@
utc = pendulum.tz.timezone('UTC')
if TYPE_CHECKING:
- from pendulum.tz.timezone import Timezone
+ pass
Review comment:
Is it still needed?
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774916244
##########
File path: airflow/utils/python_virtualenv.py
##########
@@ -118,6 +118,7 @@ def write_python_script(
to a native Python object
"""
template_loader = jinja2.FileSystemLoader(searchpath=os.path.dirname(__file__))
+ template_env: Union[jinja2.nativetypes.NativeEnvironment, jinja2.Environment]
Review comment:
```suggestion
template_env: jinja2.Environment
```
Because `NativeEnvironment` is a subclass of `Environment`
##########
File path: airflow/utils/timezone.py
##########
@@ -102,16 +102,18 @@ def convert_to_utc(value):
@overload
-def make_aware(v: None, timezone: Optional["Timezone"] = None) -> None:
+def make_aware(value: None, timezone: Optional[Union["Timezone", dt.tzinfo]] = None) -> None:
Review comment:
```suggestion
def make_aware(value: None, timezone: Optional[dt.tzinfo] = None) -> None:
```
Because `pendulum.tz.Timezone` is a subclass of `datetime.tzinfo`.
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
This shouldn’t be necessary; `coerce_datetime` always returns a `pendulum.DateTime`. Also I believe this will cause some errors to pop up elsewhere because there are functions expecting this to return a `pendulum.DateTime` specifically.
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774917257
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
This shouldn’t be necessary; `coerce_datetime` always returns None or a `pendulum.DateTime`. Also I believe this will cause some errors to pop up elsewhere because there are functions expecting this to return a `pendulum.DateTime` specifically.
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774902909
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
Review comment:
I have ignored it, for now, to wait for refactoring in another PR. Hope I understand correctly?
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774503299
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict):
Review comment:
Why would you need that ? This means that we can omit any of the fields but IMHO that would be great if we can make assumption that all fields are present in Context - so that whenever we need, we just add empty/default values for them whenever we find it necessary.
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774779237
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict):
Review comment:
This shouldn’t really matter because totality is checked on initialisation, and we don’t actually instantiate the context object directly, only force-cast a dict into one. And I chose `total=False` because it is arguably more correct semantically—the context does not always contain all the keys listed in this fake TypedDict.
What is the reason behind this 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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774914251
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict):
Review comment:
This error:
```
airflow/utils/context.pyi:51: error: Unexpected keyword argument "total" for "__init_subclass__"
of "object"
class Context(TypedDict, total=False):
^
Found 1 error in 1 file (checked 60 source files)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#issuecomment-1000723295
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774488152
##########
File path: airflow/utils/timezone.py
##########
@@ -133,10 +138,10 @@ def make_aware(value: Optional[dt.datetime], timezone: Optional["Timezone"] = No
value = value.replace(fold=1)
if hasattr(timezone, 'localize'):
# This method is available for pytz time zones.
- return timezone.localize(value)
+ return cast(Any, timezone).localize(value)
Review comment:
Use #type: ignore 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] potiuk commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774509120
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
context["try_number"] = try_number
return render_template_to_string(self.filename_jinja_template, context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.get_dagrun().logical_date.isoformat(),
- try_number=try_number,
- )
+ elif self.filename_template:
+ return self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.get_dagrun().logical_date.isoformat(),
+ try_number=try_number,
+ )
+ else:
+ return "" # mypy
Review comment:
The way `parse_template_string` works - it guarantess that either `filename_jinja_template` or `filename_template` is set. There is no way both will be None. The only "real" case where (currently) the if can be reached is if the "template_string" is "". But this is wrong anyway.
I'd say:
* raise RuntimeError in "parse_template_string" if str is ""
* raise RuntimeError instead of 'return ""' - with "This should never happen" kind of message.
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774925502
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
See the error here when changed back
```
airflow/utils/timezone.py:213: error: Incompatible return value type (got "datetime", expected
"Optional[DateTime]")
return v if v.tzinfo else make_aware(v)
^
Found 1 error in 1 file (checked 60 source files)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776309141
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
I think we need additional overload of `make_aware` with `DateTime` which should fix it. https://mypy.readthedocs.io/en/stable/more_types.html#function-overloading
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774908670
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict, total=False): # type: ignore
Review comment:
What is the error you get if you don’t ignore here? (For debugging reference later.)
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774509120
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
context["try_number"] = try_number
return render_template_to_string(self.filename_jinja_template, context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.get_dagrun().logical_date.isoformat(),
- try_number=try_number,
- )
+ elif self.filename_template:
+ return self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.get_dagrun().logical_date.isoformat(),
+ try_number=try_number,
+ )
+ else:
+ return "" # mypy
Review comment:
The way `parse_template_string` works - it guarantess that either `filename_jinja_template` or `filename_template` is set. There is no way both will be None. The only "real" case where (currently) the if can be reached is if the "template_string" is "". But this is wrong anyway.
I'd say:
* raise RuntimeError in `parse_template_string` if `tamplate_string` is ""
* raise RuntimeError instead of `return ""'` - with "This should never happen" kind of message.
* change conditions for `self.filename_template` and `self.jinja_filename_template` to `is not None` explicitly.
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774757462
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
Review comment:
I found this on mypy https://github.com/python/mypy/issues/7722 which fixes this but it seems it didn't fix it for stub file. Should we instead ignore the error and create issue in mypy? I'm thinking it's a bug considering the fix on the linked PR to the issue
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774780261
##########
File path: airflow/utils/timezone.py
##########
@@ -133,10 +138,10 @@ def make_aware(value: Optional[dt.datetime], timezone: Optional["Timezone"] = No
value = value.replace(fold=1)
if hasattr(timezone, 'localize'):
# This method is available for pytz time zones.
- return timezone.localize(value)
+ return cast(Any, timezone).localize(value)
Review comment:
+1 to `getattr`. A less-known fact: For user-defined classes (i.e. not something like `int` or `str`), `hasattr` is actually roughly equivalent to
```python
def hasattr(obj, name):
try:
getattr(obj, name)
except AttributeError:
return False
return True
```
So if you need to get the value at some point, `getattr` is always superior to `hasattr`.
--
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] khalidmammadov commented on pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#issuecomment-1001787261
Duplication of effort here https://github.com/apache/airflow/pull/20504 ...
I wish we all mention/reference these changes here https://github.com/apache/airflow/issues/19891 as per convention so to avoid time waste.
--
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] ephraimbuddy commented on pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#issuecomment-1002260764
> Duplication of effort here #20504 ...
> I wish we all mention/reference these changes here #19891 as per convention so to avoid time waste.
My bad...
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20482:
URL: https://github.com/apache/airflow/pull/20482
--
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] ephraimbuddy commented on pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#issuecomment-1000271174
> Few suggestions left :)
Thank you! Will work on them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776304364
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict, total=False): # type: ignore
Review comment:
`total=False` is Python 3.8 + feature https://docs.python.org/3/whatsnew/3.8.html
Not sure if it will be really helpful. I'd say we should specify all possible valuesi in Context.
--
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 closed pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #20482:
URL: https://github.com/apache/airflow/pull/20482
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r774509120
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,13 +82,15 @@ def _render_filename(self, ti: "TaskInstance", try_number: int) -> str:
context = Context(ti=ti, ts=ti.get_dagrun().logical_date.isoformat())
context["try_number"] = try_number
return render_template_to_string(self.filename_jinja_template, context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.get_dagrun().logical_date.isoformat(),
- try_number=try_number,
- )
+ elif self.filename_template:
+ return self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.get_dagrun().logical_date.isoformat(),
+ try_number=try_number,
+ )
+ else:
+ return "" # mypy
Review comment:
The way `parse_template_string` works - it guarantess that either `filename_jinja_template` or `filename_template` is set. There is no way both will be None. The only "real" case where (currently) the if can be reached is if the "template_string" is "". But this is wrong anyway.
I'd say:
* raise RuntimeError in `parse_template_string` if `tamplate_string` is ""
* raise RuntimeError instead of `return ""'` - with "This should never happen" kind of message.
* change conditions for `self.filename_template` and `self.filename_jinja_template` to `is not None` explicitly.
--
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] ephraimbuddy commented on a change in pull request #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r777248907
##########
File path: airflow/utils/context.pyi
##########
@@ -48,7 +48,7 @@ class VariableAccessor:
class ConnectionAccessor:
def get(self, key: str, default_conn: Any = None) -> Any: ...
-class Context(TypedDict, total=False):
+class Context(TypedDict, total=False): # type: ignore
Review comment:
Have removed it since we still support python 3.7
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776879883
##########
File path: airflow/utils/timezone.py
##########
@@ -29,7 +29,7 @@
utc = pendulum.tz.timezone('UTC')
if TYPE_CHECKING:
- from pendulum.tz.timezone import Timezone
+ pass
Review comment:
Can be removed I guess.
--
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 #20482: Fix mypy errors in airflow/utils/
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20482:
URL: https://github.com/apache/airflow/pull/20482#discussion_r776885534
##########
File path: airflow/utils/timezone.py
##########
@@ -195,11 +201,11 @@ def coerce_datetime(v: None) -> None:
@overload
-def coerce_datetime(v: dt.datetime) -> DateTime:
+def coerce_datetime(v: dt.datetime) -> dt.datetime:
...
-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:
Review comment:
OK. I figured it out. The preoblm is that DateTime derives from dt.datetime. And we have to specify DateTime overloads before datetimes ones.
What worked for me:
* make_aware overloads:
```
@overload
def make_aware(value: None, timezone: Optional[dt.tzinfo] = None) -> None:
...
@overload
def make_aware(value: DateTime, timezone: Optional[dt.tzinfo] = None) -> DateTime:
...
@overload
def make_aware(value: dt.datetime, timezone: Optional[dt.tzinfo] = None) -> dt.datetime:
...
def make_aware(value: Optional[dt.datetime], timezone: Optional[dt.tzinfo] = None) -> Optional[dt.datetime]:
```
* coerce_datetime overloads:
```
@overload
def coerce_datetime(v: None) -> None:
...
@overload
def coerce_datetime(v: DateTime) -> DateTime:
...
@overload
def coerce_datetime(v: dt.datetime) -> DateTime:
...
def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
```
--
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