Fixing MyPy issues inside providers/microsoft #20409

khalidmammadov · 2021-12-19T20:25:46Z

Part of #19891

Issue fixed:

Ignoring issue when mandatory arguments are not provided but provided from default_args instead
Fixing type signature for _read method of WasbTaskHandler
Fixing type of execute method's context parameter as it currently violates (type wise) Liskov principle from SOLID, due to different signatures from base class

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

github-actions · 2021-12-19T22:46:37Z

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.

potiuk · 2021-12-19T22:48:25Z

@khalidmamadov : There is one problem. Context does not exist in Airlfow 2.1 so you will have to change it to Any - until we decide to make providers depends on 2.3+

khalidmammadov · 2021-12-19T23:48:11Z

@khalidmamadov : There is one problem. Context does not exist in Airlfow 2.1 so you will have to change it to Any - until we decide to make providers depends on 2.3+

I see... let me fix

josh-fell · 2021-12-20T00:34:28Z

@potiuk Since some of the example DAGs (in this provider and several others) that have Mypy complaining about default_args, WDYT about adding to the logic when we render docs that ignores the # type: ignore... comments similar to what we do with the # START/END [...] markers so they don't show up in documentation? In this provider for example, we hide the Mypy ignore comment in "example_azure_blob_to_gcs.py".

uranusjr · 2021-12-20T00:45:13Z

Context is a TypedDict, which naturally downcasts to Dict[str, Any].

potiuk · 2021-12-20T10:56:32Z

Context is a TypedDict, which naturally downcasts to Dict[str, Any].

Oh yeah. that would be the best soluition - using Dict[str,Any] instead of Context should work also for 2.1 compatibility!

potiuk · 2021-12-20T10:59:25Z

@potiuk Since some of the example DAGs (in this provider and several others) that have Mypy complaining about default_args, WDYT about adding to the logic when we render docs that ignores the # type: ignore... comments similar to what we do with the # START/END [...] markers so they don't show up in documentation? In this provider for example, we hide the Mypy ignore comment in "example_azure_blob_to_gcs.py".

Should be possible. I think for that we would have to write a little extra logic here:

airflow/docs/exts/exampleinclude.py

Line 52 in f959104

class ExampleInclude(SphinxDirective):

khalidmammadov · 2021-12-20T11:46:04Z

Context is a TypedDict, which naturally downcasts to Dict[str, Any].

I will try as you suggest

potiuk · 2021-12-20T13:40:56Z

Context is a TypedDict, which naturally downcasts to Dict[str, Any].

I will try as you suggest

It will not work as exepected because of Liskov substitution principle.

airflow/providers/apache/hive/sensors/metastore_partition.py:70: error: Argument 1 of "poke" is incompatible with supertype "BaseSensorOperator"; supertype defines the argument type as "Context"
        def poke(self, context: MutableMapping[str, Any]) -> Any:
        ^
airflow/providers/apache/hive/sensors/metastore_partition.py:70: note: This violates the Liskov substitution principle
airflow/providers/apache/hive/sensors/metastore_partition.py:70: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

But I think I found a nice "workaround":

In imports section of all operators/sensors:

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

potiuk · 2021-12-20T13:56:48Z

Example on how we can treat Context here: #20422

khalidmammadov · 2021-12-20T17:24:21Z

Context is a TypedDict, which naturally downcasts to Dict[str, Any].

I will try as you suggest

It will not work as exepected because of Liskov substitution principle.

airflow/providers/apache/hive/sensors/metastore_partition.py:70: error: Argument 1 of "poke" is incompatible with supertype "BaseSensorOperator"; supertype defines the argument type as "Context"
        def poke(self, context: MutableMapping[str, Any]) -> Any:
        ^
airflow/providers/apache/hive/sensors/metastore_partition.py:70: note: This violates the Liskov substitution principle
airflow/providers/apache/hive/sensors/metastore_partition.py:70: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

But I think I found a nice "workaround":

In imports section of all operators/sensors:

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

This look good, will try this as well

…versions of airflow

potiuk · 2021-12-23T15:56:00Z

airflow/providers/microsoft/azure/log/wasb_task_handler.py

        # Mark closed so we don't double write if close is called twice
        self.closed = True

-    def _read(self, ti, try_number: str, metadata: Optional[str] = None) -> Tuple[str, Dict[str, bool]]:


Is there anything wrong here? didn't get :)

potiuk

I wish we had better solution for the "default_args" but it must do for now :(

potiuk · 2021-12-23T15:57:43Z

Some static checks :(

khalidmammadov · 2021-12-23T16:07:32Z

Some static checks :(

Yes, pre-commit was disabled... re-enabled now and fixed

potiuk · 2021-12-23T16:15:43Z

Some static checks :(

Yes, pre-commit was disabled... re-enabled now and fixed

BTW. You know you can disable just selected pre-commits @khalidmammadov ?

export SKIP="mypy"

Also git commit --no-verify will skip pre-commit just for this one commit without disabling it for the repo.

:D

khalidmammadov · 2021-12-23T16:23:19Z

Some static checks :(

Yes, pre-commit was disabled... re-enabled now and fixed

BTW. You know you can disable just selected pre-commits @khalidmammadov ?
export SKIP="mypy"
Also git commit --no-verify will skip pre-commit just for this one commit without disabling it for the repo.

:D

Ah, didnt know that (looked up for something like that but didn't find..) thank for that!

potiuk · 2021-12-23T16:38:18Z

Ah, didnt know that (looked up for something like that but didn't find..) thank for that!

You are most welcome. Pre-commit has a number of useful things built in - for example you can easily run pre-commit on the last commit only:

pre-commit run --from-ref=HEAD^ --to-ref=HEAD

Also see here: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#id1 "using pre-commit" for some other stuff.

khalidmammadov · 2021-12-23T17:29:26Z

Ah, didnt know that (looked up for something like that but didn't find..) thank for that!

You are most welcome. Pre-commit has a number of useful things built in - for example you can easily run pre-commit on the last commit only:
pre-commit run --from-ref=HEAD^ --to-ref=HEAD
Also see here: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#id1 "using pre-commit" for some other stuff.

Thanks for these, I will definitely will make use of these!

boring-cyborg bot added area:logging area:providers provider:microsoft-azure Azure-related issues labels Dec 19, 2021

potiuk approved these changes Dec 19, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 19, 2021

khalidmammadov force-pushed the mypy_providers_microsoft branch from 5e34032 to abec6c1 Compare December 19, 2021 23:45

potiuk approved these changes Dec 20, 2021

View reviewed changes

khalidmammadov mentioned this pull request Dec 21, 2021

Fix MyPy errors in Apache Providers #20422

Merged

khalidmammadov added 5 commits December 23, 2021 09:32

Fixing MyPy issues inside providers/microsoft

dd27942

Change type to Dict instead of Context as it only available in later …

81a3861

…versions of airflow

Add import for type checking only

e99943c

Add else clause

7efb4c4

Added stubs and Context type check is one liner

1e5940b

khalidmammadov force-pushed the mypy_providers_microsoft branch from 733071d to 1e5940b Compare December 23, 2021 14:26

khalidmammadov mentioned this pull request Dec 23, 2021

Re-enable MyPy #19891

Closed

10 tasks

khalidmammadov added 2 commits December 23, 2021 15:20

Relocated .pyi files to relevant packages

2fedd02

Fix formatting

11bf753

potiuk reviewed Dec 23, 2021

View reviewed changes

potiuk approved these changes Dec 23, 2021

View reviewed changes

Fix formatting

bd22d07

potiuk approved these changes Dec 23, 2021

View reviewed changes

potiuk merged commit e63e23c into apache:main Dec 23, 2021

Fixing MyPy issues inside providers/microsoft #20409

Fixing MyPy issues inside providers/microsoft #20409

Uh oh!

Conversation

khalidmammadov commented Dec 19, 2021

Uh oh!

github-actions bot commented Dec 19, 2021

Uh oh!

potiuk commented Dec 19, 2021

Uh oh!

khalidmammadov commented Dec 19, 2021

Uh oh!

josh-fell commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uranusjr commented Dec 20, 2021

Uh oh!

potiuk commented Dec 20, 2021

Uh oh!

potiuk commented Dec 20, 2021

Uh oh!

khalidmammadov commented Dec 20, 2021

Uh oh!

potiuk commented Dec 20, 2021

Uh oh!

potiuk commented Dec 20, 2021

Uh oh!

khalidmammadov commented Dec 20, 2021

Uh oh!

potiuk Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

khalidmammadov Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Dec 23, 2021

Uh oh!

khalidmammadov commented Dec 23, 2021

Uh oh!

potiuk commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khalidmammadov commented Dec 23, 2021

Uh oh!

potiuk commented Dec 23, 2021

Uh oh!

khalidmammadov commented Dec 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

josh-fell commented Dec 20, 2021 •

edited

Loading

potiuk commented Dec 23, 2021 •

edited

Loading