Add type annotations to S3 hook module #10164

coopergillan · 2020-08-05T03:50:57Z

What

Add type annotations to the S3Hook module.

Import S3Transfer object from boto3 as well as BytesIO to add type annotations.

Why

Related: #9708

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

coopergillan · 2020-08-05T04:11:44Z

When I ran the mypy report for this module using the command from #9708 I kept getting 25 out of 26 checks. I ~~can't~~ couldn't figure out which check ~is` was missing:

$ MP_DIR=$(mktemp -d); mypy --linecount-report ${MP_DIR} airflow/providers/amazon/aws/hooks; \
cat "${MP_DIR}/linecount.txt" | grep providers.amazon.aws.hooks.s3 | grep -v example_dags | \
awk '$4 != 0 {print 100.00 * ($3/$4), $3, $4, $5}'| sort -g

However, it looks like when I add annotations to __init__:

def __init__(self, *args: str, **kwargs: str):
    super().__init__(client_type='s3', *args, **kwargs)

I can get to 26/26, but get this error:

airflow/providers/amazon/aws/hooks/s3.py:110: error: "__init__" of "AwsBaseHook" gets multiple
values for keyword argument "client_type"
            super().__init__(client_type='s3', *args, **kwargs)
            ^

I'm guessing it doesn't like something about how client_type is not specified in the args, but that seems like it should be fine due to the super.

Either way, I just pushed up 573d9a6d73cf565967598b806d411a4a84b6e567 to add these and ignore the type check. That gets us to 26/26. That said, any ideas what might be going on here?

I would be happy to revert and change this if we can explain better what's going on or if we are perfectly content to have 25/26 methods covered, which I'm guessing we are =)

coopergillan · 2020-08-05T12:26:53Z

Can't tell if the CI failure is related or not.

turbaszek · 2020-08-05T12:53:38Z

airflow/providers/amazon/aws/hooks/s3.py

@@ -104,11 +106,11 @@ class S3Hook(AwsBaseHook):
        :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
    """

-    def __init__(self, *args, **kwargs):
-        super().__init__(client_type='s3', *args, **kwargs)
+    def __init__(self, *args: str, **kwargs: str):


Suggested change

def __init__(self, *args: str, **kwargs: str):

def __init__(self, *args, **kwargs):

We do not add type hints to those (usually it is tuple and dictionary)

Got it, that makes sense. Any ideas on how to get the type hint to work here? ~~Okay to ignore like I did in 573d9a6?~~ Or are we content to just leave it be?

Looking at some other examples, it seems like you can only get the type hinting to work if and when there are other arguments. Also kinda makes me wonder if we need the def __init__ at all or if we could just call the super right away.

Going to just revert 573d9a6.

turbaszek · 2020-08-05T12:55:40Z

airflow/providers/amazon/aws/hooks/s3.py

+                      prefix: str = '',
+                      delimiter: str = '',
+                      page_size: Optional[int] = None,
+                      max_items: Optional[int] = None) -> Optional[list]:


Suggested change

prefix: str = '',

delimiter: str = '',

page_size: Optional[int] = None,

max_items: Optional[int] = None) -> Optional[list]:

prefix: Optional[str] = None,

delimiter: Optional[str] = None,

page_size: Optional[int] = None,

max_items: Optional[int] = None) -> Optional[list]:

prefix = prefix or ""

delimiter = delimiter or ""

WDYT?

I think we should avoid strings as default values

I think it makes more sense if the idea behind Optional[str] is that it can be a str or None, yep.

Import S3Transfer object from boto3 as well as BytesIO to add type annotations.

This reverts commit 573d9a6d73cf565967598b806d411a4a84b6e567.

coopergillan · 2020-08-06T14:56:36Z

@turbaszek - just made a few changes and rebased on apache/master. Let me know if anything else is needed here. I'll keep an eye on the builds also.

turbaszek

Looks good to me! 🚀

ccage-simp · 2021-06-17T15:37:12Z

airflow/providers/amazon/aws/hooks/s3.py

@@ -304,7 +323,7 @@ def check_for_key(self, key, bucket_name=None):

    @provide_bucket_name
    @unify_bucket_name_and_key
-    def get_key(self, key, bucket_name=None):
+    def get_key(self, key: str, bucket_name: Optional[str] = None) -> S3Transfer:


This seems incorrect to have annotated with return type S3Transfer though actual return type is boto3.s3.Object. This is giving pycharm fits due to the mismatched interface. Hopefully no impact at runtime.

Interesting. Could you share the output you are seeing from pycharm? I cannot figure out where the boto3.s3.Object is defined. I have been through the boto3, botocore and airflow code as well as some docs but I have not been able to.

I think we need the boto3-type-annotations to solve this issue. Check out this Stack Overflow question also.

This is something @mik-laj once brought up in #11297 it looks like. I don't have time to tackle it right now, but I could in the near future.

Just dropped a comment over in #11297 as well: #11297 (comment)

Thanks, Cooper. Saying it's giving pycharm fits might have been hyperbole by me. It's showing an error message due to the confusion over types:

Unresolved attribute reference 'download_fileobj' for class 'S3Transfer'

When I step into the get_key function it's showing rtype as Object:

:rtype: boto3.s3.Object

https://pypi.org/project/boto3-stubs/ might be a helpful project here.

boring-cyborg bot added the provider:amazon AWS/Amazon - related issues label Aug 5, 2020

turbaszek reviewed Aug 5, 2020

View reviewed changes

coopergillan added 4 commits August 6, 2020 14:55

Add type annotations to S3 hook

6507c68

Import S3Transfer object from boto3 as well as BytesIO to add type annotations.

Add annotations for __init__, ignore check

e8e7386

Revert "Add annotations for __init__, ignore check"

672d5fd

This reverts commit 573d9a6d73cf565967598b806d411a4a84b6e567.

PR feedback - set default values after function call

138b73c

coopergillan force-pushed the add-type-annotations-to-s3-hook branch from 06ea37b to 138b73c Compare August 6, 2020 14:55

turbaszek approved these changes Aug 6, 2020

View reviewed changes

turbaszek merged commit 0c77ea8 into apache:master Aug 6, 2020

coopergillan deleted the add-type-annotations-to-s3-hook branch August 6, 2020 16:22

coopergillan mentioned this pull request Aug 11, 2020

Add type annotations to AwsGlueJobHook, RedshiftHook modules #10286

Merged

ccage-simp reviewed Jun 17, 2021

View reviewed changes

	def __init__(self, args: str, *kwargs: str):
	def __init__(self, args, *kwargs):

Add type annotations to S3 hook module #10164

Add type annotations to S3 hook module #10164

Uh oh!

Conversation

coopergillan commented Aug 5, 2020

What

Why

Uh oh!

coopergillan commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coopergillan commented Aug 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coopergillan Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coopergillan commented Aug 6, 2020

Uh oh!

turbaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coopergillan commented Aug 5, 2020 •

edited

Loading

coopergillan Aug 5, 2020 •

edited

Loading