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

[jbarragan-bridge]

Title: Not able to upload a compressed S3 file using close()

Motivation

We are using smart_open to compress and upload files to s3 without a context manager but since version 7.0.0 the following scenario is NOT longer working because we are using close() call without a context manager:
e.g.

hook = S3Hook()
s3_params = {
    'client': hook.get_session().client('s3'),
}
f = smart_open.open('s3://bucket/file.gz', 'w', transport_params=s3_params)
f.write('Some content')
f.close()

The problem is that the close() call is only hitting the outer stream and NOT the inner stream. More information can be found in the issue information.

- Fixes #837

Tests

2 unit tests were added to shows how this change address the compressed file upload during the close() call.

Checklist

Before you create the PR, please make sure you have:

• [ x ] Picked a concise, informative and complete title
• [ x ] Clearly explained the motivation behind the PR
• [ x ] Linked to any existing issues that your PR will be solving
• [ x ] Included tests for any new functionality
• [ x ] Checked that all unit tests pass

[ddelange]

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?)

[jbarragan-bridge]

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?

[ddelange]

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

[ddelange]

Suggested change

	self.__wrapped__.close()
	return super().close()

analogous to __exit__ above (maybe some wrapped classes might return a boolean or something)

[jbarragan-bridge]

Hi @ddelange ,

I am trying the super().close() call but the unit test returns an error:

    def close(self):
        try:
            # return self.__wrapped__.close()
>           return super().close()
E           AttributeError: 'super' object has no attribute 'close'

../utils.py:230: AttributeError

I noticed that you have __wrapper__.next() in the class method above. Probably because wrapt.ObjectProxy does not implement it; similar scenario with the class method close()).

I am implementing the return part in case close() return something in different implementation. Thanks!!

[ddelange]

alright, thanks for checking!

[ddelange]

@mpenkov an important fix, can you cut a bugfix release with this one included?

[jbarragan-bridge]

@mpenkov an important fix, can you cut a bugfix release with this one included?

Thanks @mpenkov . I was out for a couple of days. Any updates on this change? Do you need me to take a different approach? Thank you!!

[mpenkov]

Thank you @jbarragan-bridge . I will make a release in the next day or so.

[mpenkov]

https://pypi.org/project/smart-open/7.0.5/

[jbarragan-bridge]

https://pypi.org/project/smart-open/7.0.5/

Thank you so much @mpenkov and @ddelange!! We are moving to version 7.0.5 this week. I will keep you posted with our results.