Fix mypy errors in airflow/utils/

[ephraimbuddy]

---

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

Not sure of this one cc: @uranusjr

[potiuk]

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.

[uranusjr]

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?

[ephraimbuddy]

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]

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

[potiuk]

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]

Use #type: ignore instead?

[potiuk]

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)

[uranusjr]

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

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.

[ephraimbuddy]

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

[uranusjr]

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.

[uranusjr]

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

[ephraimbuddy]

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

[potiuk]

Few suggestions left :)

[ephraimbuddy]

Few suggestions left :)

Thank you! Will work on them.

[uranusjr]

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

[ephraimbuddy]

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)

[potiuk]

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.

[ephraimbuddy]

Have removed it since we still support python 3.7

[uranusjr]

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.

[ephraimbuddy]

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)

[potiuk]

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

[ephraimbuddy]

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

[potiuk]

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]

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]

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

[ephraimbuddy]

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]

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]

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]

Is it still needed?

[potiuk]

Can be removed I guess.