You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Taragolis (via GitHub)" <gi...@apache.org> on 2023/08/29 12:11:09 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #33881: Fix type annotation in AppflowHook
Taragolis opened a new pull request, #33881:
URL: https://github.com/apache/airflow/pull/33881
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Type hints + Static Checkers + boto3 = Pain
In latest mypy-boto3-appflow (1.28.29) version tasks have type `list[TaskTypeDef]` and `TaskOutputTypeDef` not exists anymore
Even if we change it and it pass CI static checks, it still won't pass in 1.28.16 (`breeze ci-image build --python 3.8`), for avoid fighting with mypy, just remove annotations related `mypy_boto3_appflow.type_defs`
P.S.: I'm not sure that we should have annotate something more than specific boto3 clients
<!-- Please keep an empty line above the dashes. -->
---
**^ 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 changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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 #33881: Remove tasks annotation in `AppflowHook`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #33881:
URL: https://github.com/apache/airflow/pull/33881
--
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] Taragolis commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308747664
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
Fun fact, deprecated `boto` and `types-boto` we got from [apache-airflow-providers-qubole](https://airflow.apache.org/docs/apache-airflow-providers-qubole/stable/index.html#apache-airflow-providers-qubole) and unmaintained `qds-sdk` 🤣
https://github.com/qubole/qds-sdk-py#where-are-the-maintainers-
> Where are the maintainers ?
Qubole was acquired. All the maintainers of this repo have moved on. Some of the employees founded [ClearFeed](https://clearfeed.ai/). Others are at big data teams in Microsoft, Amazon et al.
--
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] o-nikolas commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1309412539
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
> @o-nikolas Do you think is it even possible to implements https://github.com/apache/airflow/issues/11297 or maybe we should decide that in won't fixed
So far I've seen mostly grief working with these types packages, there was some breakage a week or so back as well. I'd agree with resolving as wont fix, for now, we can always re-open later.
--
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 diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308793434
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
So you think it's just safe to remove `mypy` ones? can we do it without loosing anyting ? Can You do it :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 a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308776313
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
We actually (sort of) do. The packages get upgraded automatically as the boto3 - which means that `--eager-upgrade` of ours will update them automatically when they are released. We certainly do not (and cannot) pin and update those packages manually (unless someone volunteers for updating it every few days). And to add the complexity we are also limited by aiobotocore releases - which automatically pin boto3 to specific versions.
And it ain't so easy as having the same version. Currently (due to aiobotocore) our "base CI image" has
* boto3==1.28.17 => we have not upgraded to latest 1.28.36 because aiobotocore holds us at 1.28.17 - we will upgrade automatically once aiobotocore releases newer version. Latest aiobotocore has:
```
Requires-Dist: boto3 <1.28.18,>=1.28.17 ; extra == 'boto3'
```
* mypy-boto3-appflow==1.28.16 -> simply because there is no 1.28.17 and 1.28.36 (which is next version) apparently has been released today
* the mypy-boto3-rds 1.28.36 has been released today as well and we will update to 1.28.36 likely (unless it has some requirements preventing from doing it).
same redshift-data
There is no consistency (for example some all the mypy libs had version betwen and some did not). And "eventually consistent" approach is I think the best we can do.
Of course we could try to somehow manyally manage it and put manual limits and bump them when we see we need to. But with 650 dependencies we have, that's far too much work - even if we would have a dependabot attempting to do it for us we often have 5-10 packages updated a day - so this automation we have to attempt to upgrade everything, let pypi figure out the latest "eager upgraded" set of dependencies that are not conflicting and passing the tests (only after they pass the tests they make it into latest constraints") is probably the best we can do.
At the expense of course that sometimes our main and PRs that change dependencies (which are few and far between) will get broken by unrelated dependency changes.
I have not figured a better way of doing it, but if somone has any ideas, I am all ears :).
--
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] Taragolis commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308834912
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
> So you think it's just safe to remove mypy ones? can we do it without loosing anyting ? Can You do it :D ?
I would be better keep what we have now, and maybe move `mypy-boto3-{service-name}` packages into provider extra block, I could have a look what we should do for it.
@o-nikolas Do you think is it even possible to implements https://github.com/apache/airflow/issues/11297 or maybe we should decide that in `won't 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
[GitHub] [airflow] Taragolis commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308741602
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
Just FYI for someone who want to keep type-checking for boto3/botocore expected values.
For correct work we should have exactly same version of `mypy-boto3-{service-name}` and boto3
```console
root@5e78bbc9c51e:/opt/airflow# pip list | grep boto
aiobotocore 2.6.0
boto 2.49.0
boto3 1.28.17
botocore 1.31.17
mypy-boto3-appflow 1.28.16
mypy-boto3-rds 1.28.34
mypy-boto3-redshift-data 1.28.16
mypy-boto3-s3 1.28.27
types-boto 2.49.18.9
```
--
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 diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1309685611
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
> I would be better keep what we have now, and maybe move mypy-boto3-{service-name} packages into provider extra block, I could have a look what we should do for it.
We could simply move them to `devel-only` extra. This is where we keep all things we want to have in CI image, but we do not want to make them direct dependencies of the packages. It's a bit disorganised and As I noticed yesterday we had forgottten "qubole" in there but let me just do it really quickly to split it into several dicts "per-provider" so that it will at least be organized.
--
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] Taragolis commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308801639
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
> Requires-Dist: boto3 <1.28.18,>=1.28.17 ; extra == 'boto3'
Author of `aiobotocore` [explain](https://aiobotocore.readthedocs.io/en/latest/contributing.html#background-and-implementation) about about why it required, and it wouldn't be resolved until `botocore` have native asyncio support (9 years since request created an P3 priority)
--
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 #33881: Remove tasks annotation in `AppflowHook`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33881:
URL: https://github.com/apache/airflow/pull/33881#issuecomment-1697410319
> Fun fact, deprecated boto and types-boto we got from [apache-airflow-providers-qubole](https://airflow.apache.org/docs/apache-airflow-providers-qubole/stable/index.html#apache-airflow-providers-qubole) and unmaintained qds-sdk 🤣
> https://github.com/qubole/qds-sdk-py#where-are-the-maintainers-
> Where are the maintainers ?
Qubole was acquired. All the maintainers of this repo have moved on. Some of the employees founded [ClearFeed](https://clearfeed.ai/). Others are at big data teams in Microsoft, Amazon et al.
Ok. Time to suspend Qubole then.
--
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] Taragolis commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308747664
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
Fun fact, deprecated `boto` and `types-boto` we got from [apache-airflow-providers-qubole](https://airflow.apache.org/docs/apache-airflow-providers-qubole/stable/index.html#apache-airflow-providers-qubole) and `qds-sdk` 🤣
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
Fun fact, deprecated `boto` and `types-boto` we got from [apache-airflow-providers-qubole](https://airflow.apache.org/docs/apache-airflow-providers-qubole/stable/index.html#apache-airflow-providers-qubole) and `qds-sdk` 🤣
--
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 diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1309720148
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
First step of reorganizing https://github.com/apache/airflow/pull/33907
Later we might move the provider one's to `devel` extras but this will require a bit more tool update when CI building so I wil leave it as next step. https://github.com/apache/airflow/issues/33908
--
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] Taragolis commented on pull request #33881: Fix type annotation in AppflowHook
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #33881:
URL: https://github.com/apache/airflow/pull/33881#issuecomment-1697324931
cc: @potiuk @Lee-W
--
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] Taragolis commented on a diff in pull request #33881: Remove tasks annotation in `AppflowHook`
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #33881:
URL: https://github.com/apache/airflow/pull/33881#discussion_r1308790721
##########
airflow/providers/amazon/aws/hooks/appflow.py:
##########
@@ -24,7 +24,6 @@
if TYPE_CHECKING:
from mypy_boto3_appflow.client import AppflowClient
- from mypy_boto3_appflow.type_defs import TaskOutputTypeDef, TaskTypeDef
Review Comment:
My point just keep annotations only for `boto3.client` in this case it is pretty safe to survive after changes, AFAIK `mypy-boto3-{service-name}` build in top of `botocore` JSON data definition, changes in methods are rare, rather than in methods types.
[I have an idea in the past](https://github.com/apache/airflow/discussions/28560) to make all AWS Hooks based on Generic, maybe it is even better than I decide do not implemented it. Also `mypy-boto3-{service-name}` pretty heavy, I don't remember exact numbers but I guess full size would be 100-200 MB
--
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