Update download_file method in s3_hook to accept files and directories by kalyani-subbiah · Pull Request #23654 · apache/airflow

kalyani-subbiah · 2022-05-11T20:51:49Z

Change the download_file method in s3_hook (aws provider) to use the download_file method from boto3 instead of download_fileobj. This allows a local filename to be passed as an argument, making it easier to track files in a bucket.

boring-cyborg · 2022-05-11T20:51:52Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

tirkarthi · 2022-05-12T06:28:42Z

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

Filename is made optional here with None as default value. It seems filename cannot be None as per Boto S3 docs. What happens when filename is not passed?

This is also a change in the documented interface and might need changelog update : https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/_api/airflow/providers/amazon/aws/hooks/s3/index.html?highlight=download_file#airflow.providers.amazon.aws.hooks.s3.S3Hook.download_file

https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file

Thanks for the review.

Good catch on the filename - I can change its type from Optional[str] to str.

Not sure where to edit the docs for the s3_hook, the link at the bottom of the page: https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/_api/airflow/providers/amazon/aws/hooks/s3/index.html that reads: "Suggest a change on this page" gives me a 404 error: https://github.com/apache/airflow/edit/main/docs/apache-airflow-providers-amazon/_api/airflow/providers/amazon/aws/hooks/s3/index.rst

Please also don't rename the parameter like that.
renaming it is a breaking change and we don't want to break user code if we can avoid it.
Users may use this function directly in thier code.

If you decide that it's best to rename (and can explain why this rename is needed) then you must support both local_path and filename.
for local_path you need to raise deprecation warning.

Good point. In that case we can keep the argument local_path.
If the local_path argument is a path to a directory, then we run the old download_fileobj method from boto3. If its a path to a file, then we run download_file from boto3. I think this workflow would be non-breaking.
We would still have to update the docs to reflect this change though.

don't get me wrong.. if you think it's best to use the new name we can do the deprecation..
it's not that complex it just add 3-4 code lines :)
See for example:

airflow/airflow/providers/trino/hooks/trino.py

Lines 165 to 173 in ccb5ce9

def get_records(self, sql: str = "", parameters: Optional[dict] = None, hql: str = ""):

""":sphinx-autoapi-skip:"""

if hql:

warnings.warn(

"The hql parameter has been deprecated. You should pass the sql parameter.",

DeprecationWarning,

stacklevel=2,

)

sql = hql

Yeah. Deprecations should be when we have some external circumstances - such as changing naming conventions from libraries etc. But if we are in full control of the API and can choose, then it's better to keep it backwards compatible.

Hi! I have updated this pull request based on the feedback above. It's now non-breaking. Thanks so much everyone.
I've added a conditional for using local_path as path to a directory or a file.
Also added an additional unit-test.

…oto3 instead of download_fileobj in order to keep track of the file name & update unit tests

potiuk · 2022-05-17T17:07:11Z

Needsrebase/fixing static checks.

ferruzzi · 2022-05-17T17:59:38Z

tests/providers/amazon/aws/hooks/test_s3.py

        s3_hook.get_key.assert_called_once_with(key, bucket)
        s3_obj.download_fileobj.assert_called_once_with(mock_temp_file)

+    def test_download_file2(self, s3_bucket):


Please use a more descriptive test/method name. What is this testing that test_download_file doesn't? These are test methods, so you can rename them to something like test_download_file_with_filename and test_download_file_with_directory or something like that, which is much more useful.

Hi, thanks for pointing it out - will do once I get some time.

…d in s3_hook.py

potiuk · 2022-06-04T21:30:00Z

How about fixing static checks and doc failures @kalyani-subbiah - we won't even look at merging it before they are fixed really.

potiuk · 2022-06-04T21:30:21Z

Rebase and use pre-commit/breeze to build docs.

tests/providers/amazon/aws/hooks/test_s3.py

potiuk · 2022-06-19T21:32:22Z

Docs build failing.

eladkal · 2022-07-22T02:33:32Z

@kalyani-subbiah are you still working on this PR?

github-actions · 2022-09-06T00:13:56Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

o-nikolas · 2022-09-28T20:58:05Z

Checking in again here, @kalyani-subbiah are you still working on this PR? It looks like this one was close to the finish line but needed some rebasing and docs fixes.

eladkal · 2022-10-05T16:14:19Z

Followup: #26886

boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels May 11, 2022

tirkarthi reviewed May 12, 2022

View reviewed changes

kalyani-subbiah added 2 commits May 13, 2022 11:58

Change method download_file in s3_hook.py to use download_file from b…

d223893

…oto3 instead of download_fileobj in order to keep track of the file name & update unit tests

local_path in s3.py can be either filename or directory

db91e31

kalyani-subbiah force-pushed the main branch from cf90984 to db91e31 Compare May 13, 2022 15:58

kalyani-subbiah changed the title ~~Update download_file method in s3_hook to accept filename argument~~ Update download_file method in s3_hook to accept files and directories May 13, 2022

adding suggested changes by breeze checks

dfd0571

vincbeck approved these changes May 17, 2022

View reviewed changes

ferruzzi reviewed May 17, 2022

View reviewed changes

kalyani-subbiah added 2 commits May 31, 2022 10:39

Merge branch 'apache:main' into main

de0a3eb

change unit test names to be more descriptive for download_file metho…

30e7d2f

…d in s3_hook.py

eladkal reviewed Jun 15, 2022

View reviewed changes

tests/providers/amazon/aws/hooks/test_s3.py Outdated Show resolved Hide resolved

Update tests/providers/amazon/aws/hooks/test_s3.py

b918a3f

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 6, 2022

github-actions bot closed this Sep 11, 2022

alexkruc mentioned this pull request Oct 5, 2022

Adding preserve_file_name param to S3Hook.download_file method #26886

Merged

	def get_records(self, sql: str = "", parameters: Optional[dict] = None, hql: str = ""):
	""":sphinx-autoapi-skip:"""
	if hql:
	warnings.warn(
	"The hql parameter has been deprecated. You should pass the sql parameter.",
	DeprecationWarning,
	stacklevel=2,
	)
	sql = hql

Comments

Conversation

kalyani-subbiah commented May 11, 2022 • edited by eladkal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented May 11, 2022

Uh oh!

tirkarthi May 12, 2022

Choose a reason for hiding this comment

Uh oh!

kalyani-subbiah May 12, 2022 • 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!

eladkal May 12, 2022

Choose a reason for hiding this comment

Uh oh!

kalyani-subbiah May 12, 2022 • 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!

eladkal May 12, 2022

Choose a reason for hiding this comment

Uh oh!

potiuk May 12, 2022

Choose a reason for hiding this comment

Uh oh!

kalyani-subbiah May 13, 2022 • 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!

potiuk commented May 17, 2022

Uh oh!

ferruzzi May 17, 2022

Choose a reason for hiding this comment

Uh oh!

kalyani-subbiah May 18, 2022

Choose a reason for hiding this comment

Uh oh!

potiuk commented Jun 4, 2022

Uh oh!

potiuk commented Jun 4, 2022

Uh oh!

Uh oh!

potiuk commented Jun 19, 2022

Uh oh!

eladkal commented Jul 22, 2022

Uh oh!

github-actions bot commented Sep 6, 2022

Uh oh!

o-nikolas commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eladkal commented Oct 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kalyani-subbiah commented May 11, 2022 •

edited by eladkal

Loading

kalyani-subbiah May 12, 2022 •

edited

Loading

kalyani-subbiah May 12, 2022 •

edited

Loading

kalyani-subbiah May 13, 2022 •

edited

Loading

o-nikolas commented Sep 28, 2022 •

edited

Loading