Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Dec 20, 2021

Part of #19891

The .py additions are to handle "default_args" passed in
examples. Currently some of the obligatory parameters are
(correctly) passed as default_args. We have no good
mechanism yet to handle it properly for MyPy (it would
require to add a custom MyPy plugin to handle it)

We have no better way to handle it for now.


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

@potiuk
Copy link
Member Author

potiuk commented Dec 20, 2021

cc: @khalidmammadov

@potiuk
Copy link
Member Author

potiuk commented Dec 20, 2021

If we agree this is the right way of treating context we could easily automate and apply it to all operators/sensors that have currnetly:

execute(self, context)

or

poke(self, context)

as a single big change (after we complete all other MyPy changes. WDYT?

@potiuk
Copy link
Member Author

potiuk commented Dec 20, 2021

[jarek:~/code/airflow] [airflow-master-3.7] fix-mypy-google-cloud-build+ ± grep -r "execute(self, context)" airflow/providers | wc
    362    1700   32196
[jarek:~/code/airflow] [airflow-master-3.7] fix-mypy-google-cloud-build+ ± grep -r "poke(self, context)" airflow/providers | wc
     21      86    1644
[jarek:~/code/airflow] [airflow-master-3.7] fix-mypy-google-cloud-build+ ± 

@potiuk
Copy link
Member Author

potiuk commented Dec 20, 2021

And including airlfow built-ins (there we could directly import airflow's context):

[jarek:~/code/airflow] [airflow-master-3.7] fix-mypy-google-cloud-build+ 3s ± grep -r "execute(self, context)" airflow | wc
    373    1744   32907
[jarek:~/code/airflow] [airflow-master-3.7] fix-mypy-google-cloud-build+ ± grep -r "poke(self, context)" airflow | wc          
     28     114    2040

@potiuk potiuk force-pushed the fix-mypy-apache-providers branch from 723a882 to 7c1c7bf Compare December 20, 2021 16:58
@potiuk
Copy link
Member Author

potiuk commented Dec 20, 2021

I am afraid I have no better idea for the "Context" imports from providers though :(

@potiuk
Copy link
Member Author

potiuk commented Dec 21, 2021

Need some review here - anyone has better idea what to do instead of:

if TYPE_CHECKING:
    from airflow.utils.context import Context
else:
    Context = MutableMapping[str, Any]

I would like to automatically add it later to all the provider's executes/pokes to really benefit from the latest TypeDict Context improvements :D, but maybe we can find a better way ? The problem with it is we can't rely on the common code in airflow :(

@khalidmammadov
Copy link
Contributor

khalidmammadov commented Dec 21, 2021 via email

@khalidmammadov
Copy link
Contributor

khalidmammadov commented Dec 21, 2021 via email

@potiuk
Copy link
Member Author

potiuk commented Dec 21, 2021

Ignore me, we still need that part

Nice try though :)

@potiuk potiuk force-pushed the fix-mypy-apache-providers branch from 7c1c7bf to 0f90cde Compare December 21, 2021 22:41
@uranusjr
Copy link
Member

uranusjr commented Dec 22, 2021

I don’t think we need the else part. This should work:

if TYPE_CHECKING:
    from airflow.utils.context import Context

def execute(self, context: "Context") -> None:
    ...

@josh-fell
Copy link
Contributor

I don’t think we need the else part. This should work:

if TYPE_CHECKING:
    from airflow.utils.context import Context

def execute(self, context: "Context") -> None:
    ...

I can confirm this works in #20034 for the Docker decorator.

@potiuk
Copy link
Member Author

potiuk commented Dec 22, 2021

Ah yeah! good one @uranusjr . I totally forgot that we can quote context :)

@potiuk
Copy link
Member Author

potiuk commented Dec 22, 2021

That's why I was waiting with applying that. Now we can actually even make it truly "one-liner - cc: @khalidmammadov :

if TYPE_CHECKING: from airflow.utils.context import Context

(not sure if black will be happy with it though)

@potiuk
Copy link
Member Author

potiuk commented Dec 22, 2021

Black is not happy with one-liner :(

@potiuk potiuk force-pushed the fix-mypy-apache-providers branch from 0f90cde to 38df9d1 Compare December 22, 2021 16:12
@potiuk
Copy link
Member Author

potiuk commented Dec 22, 2021

All should be nicely updaated. @uranusjr - good stuff with the "Context"

@potiuk potiuk force-pushed the fix-mypy-apache-providers branch from 38df9d1 to 7984d61 Compare December 22, 2021 16:13
@potiuk
Copy link
Member Author

potiuk commented Dec 22, 2021

Should be good to go and I'd love t refresh stats after that :)

@uranusjr
Copy link
Member

It seems I’m missing context on the pyi additions.

@subkanthi subkanthi mentioned this pull request Dec 24, 2021
10 tasks
@khalidmammadov
Copy link
Contributor

Looking at airflow/utils. Let me know if you as well

@potiuk potiuk force-pushed the fix-mypy-apache-providers branch 3 times, most recently from afcbefd to 8a6e6bc Compare December 27, 2021 08:26
@potiuk
Copy link
Member Author

potiuk commented Dec 27, 2021

It seems I’m missing context on the pyi additions.

Context added.

1 similar comment
@potiuk
Copy link
Member Author

potiuk commented Dec 27, 2021

It seems I’m missing context on the pyi additions.

Context added.

Part of apache#19891

The .pyi additions are to handle "default_args" passed in
examples. Currently some of the obligatory parameters are
(correctly) passed as default_args. We have no good
mechanism yet to handle it properly for MyPy (it would
require to add a custom MyPy plugin to handle it)

We have no better way to handle it for now.
@potiuk potiuk force-pushed the fix-mypy-apache-providers branch from 8a6e6bc to 2e90189 Compare December 27, 2021 12:07
@potiuk
Copy link
Member Author

potiuk commented Dec 29, 2021

Can I get the approval :) ? MyPy is calling.

@potiuk potiuk requested a review from eladkal December 29, 2021 12:55
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 29, 2021
@potiuk potiuk merged commit 485ff6c into apache:main Dec 29, 2021
@potiuk potiuk deleted the fix-mypy-apache-providers branch December 29, 2021 20:33
@potiuk potiuk restored the fix-mypy-apache-providers branch April 26, 2022 20:52
@potiuk potiuk deleted the fix-mypy-apache-providers branch July 29, 2022 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants