Fix mypy errors in airflow/utils/ #20482

ephraimbuddy · 2021-12-23T09:29:59Z

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

ephraimbuddy · 2021-12-23T09:30:50Z

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):


Not sure of this one cc: @uranusjr

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 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 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)

ephraimbuddy · 2021-12-23T09:35:59Z

airflow/utils/log/file_task_handler.py

+                try_number=try_number,
+            )
+        else:
+            return ""  # mypy


This is also another place I'm confused about the best approach to it and whether to work on the parse_template_string itself

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.

ephraimbuddy · 2021-12-23T10:52:42Z

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)


Use #type: ignore instead?

There is a discussion about MyPy supporting this case here: python/mypy#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)

+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

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.

potiuk · 2021-12-23T11:23:47Z

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())


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.

I found this on mypy python/mypy#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

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.

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

I have ignored it, for now, to wait for refactoring in another PR. Hope I understand correctly?

potiuk

Few suggestions left :)

ephraimbuddy · 2021-12-23T12:27:22Z

Few suggestions left :)

Thank you! Will work on them.

uranusjr · 2021-12-24T07:25:28Z

airflow/utils/context.pyi

-class Context(TypedDict, total=False):
+class Context(TypedDict, total=False):  # type: ignore


What is the error you get if you don’t ignore here? (For debugging reference later.)

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)

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.

Have removed it since we still support python 3.7

airflow/utils/python_virtualenv.py

airflow/utils/timezone.py

uranusjr · 2021-12-24T07:53:59Z

airflow/utils/timezone.py

-def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
+def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:


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.

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)

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

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... ^

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]:

github-actions · 2021-12-24T08:32:54Z

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.

khalidmammadov · 2021-12-27T22:43:54Z

Duplication of effort here #20504 ...
I wish we all mention/reference these changes here #19891 as per convention so to avoid time waste.

ephraimbuddy · 2021-12-28T19:59:34Z

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...

khalidmammadov · 2021-12-28T20:20:20Z

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

ephraimbuddy · 2021-12-29T08:01:53Z

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).

mik-laj · 2021-12-30T10:23:58Z

airflow/utils/timezone.py

 if TYPE_CHECKING:
-    from pendulum.tz.timezone import Timezone
+    pass


Is it still needed?

Can be removed I guess.

boring-cyborg bot added the area:logging label Dec 23, 2021

ephraimbuddy requested review from ashb, uranusjr and potiuk and removed request for ashb and uranusjr December 23, 2021 09:30

ephraimbuddy commented Dec 23, 2021

View reviewed changes

potiuk reviewed Dec 23, 2021

View reviewed changes

ephraimbuddy force-pushed the fix-mypy-for-utils branch from 7ab58ee to 7de29e2 Compare December 23, 2021 23:17

uranusjr reviewed Dec 24, 2021

View reviewed changes

uranusjr approved these changes Dec 24, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 24, 2021

khalidmammadov mentioned this pull request Dec 28, 2021

Fixing various issues in utils folder reported by MyPy #20504

Closed

potiuk closed this Dec 29, 2021

potiuk reopened this Dec 29, 2021

ephraimbuddy force-pushed the fix-mypy-for-utils branch from 7670a41 to 520e81b Compare December 30, 2021 05:51

mik-laj reviewed Dec 30, 2021

View reviewed changes

potiuk added the mypy Fixing MyPy problems after bumpin MyPy to 0.990 label Dec 31, 2021

Fix mypy errors in airflow/utils/

bba317f

ephraimbuddy added 3 commits January 2, 2022 20:55

apply reviews from code review

9aa0632

fixup! apply reviews from code review

7e8d50a

apply reviews from code review

4f4ef54

ephraimbuddy force-pushed the fix-mypy-for-utils branch from 520e81b to 4f4ef54 Compare January 2, 2022 19:55

Remove total=false in context

9106c9f

potiuk approved these changes Jan 2, 2022

View reviewed changes

uranusjr approved these changes Jan 3, 2022

View reviewed changes

uranusjr merged commit 138f38f into apache:main Jan 3, 2022

uranusjr deleted the fix-mypy-for-utils branch January 3, 2022 03:24

kaxil added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mypy errors in airflow/utils/ #20482

Fix mypy errors in airflow/utils/ #20482

ephraimbuddy commented Dec 23, 2021

ephraimbuddy Dec 23, 2021

potiuk Dec 23, 2021

uranusjr Dec 23, 2021

ephraimbuddy Dec 24, 2021

ephraimbuddy Dec 23, 2021

potiuk Dec 23, 2021 •

edited

Loading

ephraimbuddy Dec 23, 2021

potiuk Dec 23, 2021 •

edited

Loading

uranusjr Dec 23, 2021

potiuk Dec 23, 2021

ephraimbuddy Dec 23, 2021

uranusjr Dec 23, 2021

uranusjr Dec 23, 2021

ephraimbuddy Dec 24, 2021

potiuk left a comment

ephraimbuddy commented Dec 23, 2021

uranusjr Dec 24, 2021

ephraimbuddy Dec 24, 2021

potiuk Dec 29, 2021 •

edited

Loading

ephraimbuddy Jan 2, 2022

uranusjr Dec 24, 2021 •

edited

Loading

ephraimbuddy Dec 24, 2021

potiuk Dec 29, 2021 •

edited

Loading

ephraimbuddy Dec 30, 2021

potiuk Dec 30, 2021

github-actions bot commented Dec 24, 2021

khalidmammadov commented Dec 27, 2021

ephraimbuddy commented Dec 28, 2021

khalidmammadov commented Dec 28, 2021

ephraimbuddy commented Dec 29, 2021

mik-laj Dec 30, 2021

potiuk Dec 30, 2021

		@@ -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())

		class Context(TypedDict, total=False):
		class Context(TypedDict, total=False): # type: ignore

		def coerce_datetime(v: Optional[dt.datetime]) -> Optional[DateTime]:
		def coerce_datetime(v: Optional[dt.datetime]) -> Optional[dt.datetime]:

Fix mypy errors in airflow/utils/ #20482

Fix mypy errors in airflow/utils/ #20482

Conversation

ephraimbuddy commented Dec 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

ephraimbuddy commented Dec 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uranusjr Dec 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 24, 2021

khalidmammadov commented Dec 27, 2021

ephraimbuddy commented Dec 28, 2021

khalidmammadov commented Dec 28, 2021

ephraimbuddy commented Dec 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Dec 23, 2021 •

edited

Loading

potiuk Dec 23, 2021 •

edited

Loading

potiuk Dec 29, 2021 •

edited

Loading

uranusjr Dec 24, 2021 •

edited

Loading

potiuk Dec 29, 2021 •

edited

Loading