Repository navigation
Closed
Labels
area:providersgood first issuekind:bugThis is a clearly a bugThis is a clearly a bugprovider:amazonAWS/Amazon - related issuesAWS/Amazon - related issues
Description
Apache Airflow Provider(s)
amazon
Versions of Apache Airflow Providers
No response
Apache Airflow version
2.3.0 (latest released)
Operating System
Mac OS Mojave 10.14.6
Deployment
Official Apache Airflow Helm Chart
Deployment details
No response
What happened
When I download a json file from S3 using the S3Hook:
filename=s3_hook.download_file(bucket_name=self.source_s3_bucket, key=key, local_path="./data")
The file is being downloaded as a text file starting with airflow_temp_.
What you think should happen instead
It would be nice to have them download as a json file or keep the same filename as in S3. Since it requires additional code to go back and read the file as a dictionary (ast.literal_eval) and there is no guarantee that the json structure is maintained.
How to reproduce
Where s3_conn_id is the Airflow connection and s3_bucket is a bucket on AWS S3.
This is the custom operator class:
from airflow.models.baseoperator import BaseOperator
from airflow.utils.decorators import apply_defaults
from airflow.hooks.S3_hook import S3Hook
import logging
class S3SearchFilingsOperator(BaseOperator):
"""
Queries the Datastore API and uploads the processed info as a csv to the S3 bucket.
:param source_s3_bucket: Choose source s3 bucket
:param source_s3_directory: Source s3 directory
:param s3_conn_id: S3 Connection ID
:param destination_s3_bucket: S3 Bucket Destination
"""
@apply_defaults
def __init__(
self,
source_s3_bucket=None,
source_s3_directory=True,
s3_conn_id=True,
destination_s3_bucket=None,
destination_s3_directory=None,
search_terms=[],
*args,
**kwargs) -> None:
super().__init__(*args, **kwargs)
self.source_s3_bucket = source_s3_bucket
self.source_s3_directory = source_s3_directory
self.s3_conn_id = s3_conn_id
self.destination_s3_bucket = destination_s3_bucket
self.destination_s3_directory = destination_s3_directory
def execute(self, context):
"""
Executes the operator.
"""
s3_hook = S3Hook(self.s3_conn_id)
keys = s3_hook.list_keys(bucket_name=self.source_s3_bucket)
for key in keys:
# download file
filename=s3_hook.download_file(bucket_name=self.source_s3_bucket, key=key, local_path="./data")
logging.info(filename)
with open(filename, 'rb') as handle:
filing = handle.read()
filing = pickle.loads(filing)
logging.info(filing.keys())
And this is the dag file:
from keywordSearch.operators.s3_search_filings_operator import S3SearchFilingsOperator
from airflow import DAG
from airflow.utils.dates import days_ago
from datetime import timedelta
# from aws_pull import aws_pull
default_args = {
"owner" : "airflow",
"depends_on_past" : False,
"start_date": days_ago(2),
"email" : ["airflow@example.com"],
"email_on_failure" : False,
"email_on_retry" : False,
"retries" : 1,
"retry_delay": timedelta(seconds=30)
}
with DAG("keyword-search-full-load",
default_args=default_args,
description="Syntax Keyword Search",
max_active_runs=1,
schedule_interval=None) as dag:
op3 = S3SearchFilingsOperator(
task_id="s3_search_filings",
source_s3_bucket="processed-filings",
source_s3_directory="citations",
s3_conn_id="Syntax_S3",
destination_s3_bucket="keywordsearch",
destination_s3_directory="results",
dag=dag
)
op3
Anything else
No response
Are you willing to submit PR?
- Yes I am willing to submit a PR!
Code of Conduct
- I agree to follow this project's Code of Conduct
Metadata
Metadata
Assignees
Labels
area:providersgood first issuekind:bugThis is a clearly a bugThis is a clearly a bugprovider:amazonAWS/Amazon - related issuesAWS/Amazon - related issues
Activity
boring-cyborg commentedon May 5, 2022
Thanks for opening your first issue here! Be sure to follow the issue template!
tirkarthi commentedon May 6, 2022
The logic is here where
airflow_tmp_is hardcoded. Since the name is returned I guess youshutil.moveto rename it https://docs.python.org/3/library/shutil.html#shutil.moveairflow/airflow/providers/amazon/aws/hooks/s3.py
Lines 858 to 859 in 83784d9
Change introduced in #6577 . cc: @feluelle
kalyani-subbiah commentedon May 6, 2022
I see. In my project since I'm downloading a lot of files, the filenames are important to keep track of what the contents are. When it gets downloaded as a temp file with a different name, this content is lost. I will have to delete all the contents of the temp directory for each iteration which is time-consuming.
One thing that would be nice is if we could specify the file name for the download. Any reason why we're using .download_fileobj over .download_file from boto3? The .download_file function would be much better since a local filename can be specified:
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file
feluelle commentedon May 7, 2022
Hi @kalyani-subbiah,
I would say we can change this behavior to use open and write to a real file instead of a temp file as like you said you can properly set the file name incl. ending.
I can't remember why I initially did it like this. Maybe the use case was only to dump the data and then forget about it, as by default it writes to the temp dir.
Feel free to open a PR with how you think it should be. But first we should check where this method is used in the code base and then also very importantly consider backwards compatibility.
Thanks 🙏😊
kalyani-subbiah commentedon May 7, 2022
Hi @feluelle . Thanks, will look into creating a PR now.
kalyani-subbiah commentedon May 11, 2022
Hi I just created a PR for this issue!