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/19 20:25:58 UTC
[GitHub] [airflow] khalidmammadov opened a new pull request #20409: Fixing MyPy issues inside providers/microsoft
khalidmammadov opened a new pull request #20409:
URL: https://github.com/apache/airflow/pull/20409
Part of https://github.com/apache/airflow/issues/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](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] khalidmammadov commented on pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000439968
> > 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!
--
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] josh-fell edited a comment on pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
josh-fell edited a comment on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997495631
@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".
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000415080
> 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.
--
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 merged pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20409:
URL: https://github.com/apache/airflow/pull/20409
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997815698
> 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!
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997852138
> Context is a TypedDict, which naturally downcasts to `Dict[str, Any]`.
I will try as you suggest
--
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 edited a comment on pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000403160
> > 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
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997817599
> @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: https://github.com/apache/airflow/blob/f9591042285e8acefebdab0f789357049e136f64/docs/exts/exampleinclude.py#L52
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997934311
> > 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 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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997945918
Example on how we can treat Context here: #20422
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#discussion_r774655967
##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -107,7 +107,7 @@ def close(self) -> None:
# 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]]:
Review comment:
hmmmmmm :)
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000403160
> > Some static checks :(
>
> Yes, pre-commit was disabled... re-enabled now and fixed
BTW. You now you can disable just selected pre-commits @khalidmammadov :
```
export SKIP="mypy"
```
Also `git commit --no-verify` will skip pre-commit just for this one commut without disabling it for the repo.
:D
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997475696
@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+
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000407283
> > > 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!
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997475362
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.
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997485746
> @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
--
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 edited a comment on pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000403160
> > Some static checks :(
>
> Yes, pre-commit was disabled... re-enabled now and fixed
BTW. You now 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
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000393174
Some static checks :(
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-998127489
> > > 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
--
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] josh-fell commented on pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997495631
@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 build 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".
--
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 pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-997498550
Context is a TypedDict, which naturally downcasts to `Dict[str, Any]`.
--
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 a change in pull request #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#discussion_r774661821
##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -107,7 +107,7 @@ def close(self) -> None:
# 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]]:
Review comment:
Is there anything wrong here? didn't get :)
--
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 #20409: Fixing MyPy issues inside providers/microsoft
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #20409:
URL: https://github.com/apache/airflow/pull/20409#issuecomment-1000398482
> Some static checks :(
Yes, pre-commit was disabled... re-enabled now and fixed
--
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