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.

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

Support writing any bytes-like object to S3. #361

Merged
merged 2 commits into from
Oct 13, 2019

Conversation

gilbsgilbs
Copy link
Contributor

Fixes #360

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 9, 2019

All the tests pass, so I guess that check was not necessary. Good catch!

Could you please update the docstring of that method and mention the Buffer interface?

Is it possible to write a unit test for this without including PyArrow?

@piskvorky
Copy link
Owner

piskvorky commented Oct 9, 2019

I don't remember that "isinstance checking code" that was removed here. Can you please track down the commit / PR that added it, to see if it gives its rationale?

Just to make sure we're not regressing somewhere.

@gilbsgilbs gilbsgilbs force-pushed the s3-support-any-bytelike-object branch from 97d4fc5 to 423d7db Compare October 9, 2019 08:44
@gilbsgilbs
Copy link
Contributor Author

I don't remember that "isinstance checking code" that was removed here. Can you please track down the commit / PR that added it, to see if it gives its rationale?

It comes from this commit which I don't think is relevant: 5f3b6fa8#diff-6d8b12a1a0fab36149bf68fc0ef770faR315

It was then improved in #293 to support memoryviews.

Is it possible to write a unit test for this without including PyArrow?

Not sure how 🤔 . I think I'd have to implement the buffer interface in a C extension somehow which sounds overkill. With pyarrow, it'd be quite easy to reproduce, still you'd have to download loads of wheels or even compile it yourself which also sounds overkill for the tests.

import pyarrow as pa
import smart_open

buf = pa.py_buffer(b'foobar')

with open('raw', 'wb') as f:
  f.write(buf)  # Works

with smart_open.open('s3://…', 'wb') as f:
  f.write(buf)  # Crashes

Could you please update the docstring of that method and mention the Buffer interface?

✔️

@gilbsgilbs
Copy link
Contributor Author

@piskvorky @mpenkov anything else I can do?

@@ -539,22 +533,19 @@ def detach(self):
raise io.UnsupportedOperation("detach() not supported")

def write(self, b):
"""Write the given bytes (binary string) to the S3 file.
"""Write the given buffer (bytes, bytearray, memoryview or any buffer
interface implementation) to the S3 file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please include a link to the appopriate standard library documentation?

Is it this one? https://docs.python.org/3/c-api/buffer.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the right one.

@mpenkov mpenkov merged commit b921446 into piskvorky:master Oct 13, 2019
@mpenkov
Copy link
Collaborator

mpenkov commented Oct 13, 2019

Congrats on your first contribution to smart_open @gilbsgilbs 🥇 !!!

@gilbsgilbs gilbsgilbs deleted the s3-support-any-bytelike-object branch October 13, 2019 18:30
@gilbsgilbs
Copy link
Contributor Author

Thanks for the prompt review and merge! May I ask when you expect to release it? This is quite blocking for me actually.

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 14, 2019

We will try to release before the end of October.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't write any bytes-like object to S3
3 participants