Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not able to upload a compressed S3 file using close() #838

Merged
merged 2 commits into from
Oct 4, 2024
Merged

Not able to upload a compressed S3 file using close() #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f435238
Call inner close() call during outer close() call in FileLikeProxy class
jbarragan-bridge Sep 24, 2024
61df843
Return close() result in FileLikeProxy class
jbarragan-bridge Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions smart_open/tests/test_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,43 @@ def test_writebuffer(self):

assert actual == contents

def test_write_gz_using_context_manager(self):
"""Does s3 multipart upload create a compressed file using context manager?"""
contents = b'get ready for a surprise'
with smart_open.open(
f's3://{BUCKET_NAME}/{WRITE_KEY_NAME}.gz',
mode="wb",
transport_params={
"multipart_upload": True,
"min_part_size": 10,
}
) as fout:
fout.write(contents)

with smart_open.open(f's3://{BUCKET_NAME}/{WRITE_KEY_NAME}.gz', 'rb') as fin:
actual = fin.read()

assert actual == contents

def test_write_gz_not_using_context_manager(self):
"""Does s3 multipart upload create a compressed file not using context manager but close()?"""
contents = b'get ready for a surprise'
fout = smart_open.open(
f's3://{BUCKET_NAME}/{WRITE_KEY_NAME}.gz',
mode="wb",
transport_params={
"multipart_upload": True,
"min_part_size": 10,
}
)
fout.write(contents)
fout.close()

with smart_open.open(f's3://{BUCKET_NAME}/{WRITE_KEY_NAME}.gz', 'rb') as fin:
actual = fin.read()

assert actual == contents

def test_write_gz_with_error(self):
"""Does s3 multipart upload abort for a failed compressed file upload?"""
with self.assertRaises(ValueError):
Expand Down
7 changes: 7 additions & 0 deletions smart_open/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,10 @@ def __exit__(self, *args, **kwargs):

def __next__(self):
return self.__wrapped__.__next__()

def close(self):
try:
return self.__wrapped__.close()
finally:
if self.__inner != self.__wrapped__: # Don't close again if inner and wrapped are the same
Copy link
Contributor

@ddelange ddelange Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a scenario where they are the same, plus calling close a second is a no-op. I think we can do without this conditional (unless there are tests failing without this conditional?)

Copy link
Contributor Author

@jbarragan-bridge jbarragan-bridge Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking the code @ddelange . This following test fails if I remove this conditional:

https://github.com/jbarragan-bridge/smart_open/blob/f435238348ce6ab66bc2cc8a3ab13a5a4595c145/smart_open/tests/test_smart_open.py#L1409-L1445 :

assert len(responses.calls) == 4  # it returns 6 without the conditional

I don't know much about hdfs; I saw in debug mode that this scenario creates a FileLikeProxy instance where the wrapper and inner are the same; duplicating the close() call.

Should I adjust the unit test instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to know, something to look into but not related to this PR. let's leave it as is :)

self.__inner.close()
Loading