Smart_open: Can no longer write gzipped files.

[balihoo-dengstrom]

This new check has removed the ability to write gzipped files to S3.

It looks like native gzipping is being added to smart_open and that's why this check was put in place. However, until the new write functionality is added this check should be removed in order to allow users to write their own compressed stream.

[mpenkov]

@balihoo-dengstrom I'm the short-sighted developer that put that line in, so let me first apologize for inconveniencing you and anyone else affected by this.

@piskvorky @tmylk We _could_ remove the check, but that would break the symmetry of the read/write operation for gzipped S3 files. More specifically, reading would perform native decompression, while writing would leave the compression up to the user. In my opinion, that's unexpected behavior.

An alternative for the check would be to allow the user to specify whether or not smart_open should use native decompression when reading/writing gzipped S3 files. This would be a keyword argument to smart_open, e.g.

smart_open("s3://bucket/key.gz", "r", native_gzip=True)   # perform native decompression
smart_open("s3://bucket/key.gz", "r", native_gzip=False)   # leave decompression up to user
smart_open("s3://bucket/key.gz", "w", native_gzip=True)   # NotImplementedError
smart_open("s3://bucket/key.gz", "w", native_gzip=False)   # leave compression up to user

What do you think?

[tmylk]

@mpenkov The alternative looks very clear. A PR on it would be very welcome.

[piskvorky]

I don't think I understand this.

smart_open does compression/decompression of text files transparently, based on file extension (.gz, .bz2). Where it cannot do this, it should explicitly (and loudly) fail.

We definitely don't want to sometimes (de)compress, sometimes not. I don't see the need for any such native_gzip switch/option -- what is its use-case?

[tmylk]

@balihoo-dengstrom What is the use case for this? I am asking because using smart_open for binary data is unexpected. It streams files line by line - how do you work around that?

Also, for ease of understanding should the parameter be called gzip_decompression instead of native_gzip?

[mpenkov]

@piskvorky @tmylk What if their file is already gzip-compressed and they just want to store
it to S3?
On Fri, Aug 5, 2016 at 14:03 Radim Řehůřek [email protected] wrote:

I don't think I understand this.

smart_open does compression/decompression of text files transparently,
based on file extension (.gz, .bz2).

We definitely don't want to sometimes uncompress, sometimes not. I don't
see the need for any such native_gzip switch/options -- what is its
use-case?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/RaRe-Technologies/smart_open/issues/82#issuecomment-237833453,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDOVP7eq5PvzKhCc1KkYNG28lkW3Trjks5qcyaPgaJpZM4JUm6y
.

[mpenkov]

+1 for gzip_decompression
On Fri, Aug 5, 2016 at 14:12 Misha Penkov misha.[email protected] wrote:

What if their file is already gzip-compressed and they just want to store
it to S3?
On Fri, Aug 5, 2016 at 14:03 Radim Řehůřek [email protected]
wrote:

I don't think I understand this.

smart_open does compression/decompression of text files transparently,
based on file extension (.gz, .bz2).

We definitely don't want to sometimes uncompress, sometimes not. I don't
see the need for any such native_gzip switch/options -- what is its
use-case?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/RaRe-Technologies/smart_open/issues/82#issuecomment-237833453,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDOVP7eq5PvzKhCc1KkYNG28lkW3Trjks5qcyaPgaJpZM4JUm6y
.

[balihoo-dengstrom]

@piskvorky & @tmylk: Maybe I've misunderstood the purpose of smart_open. Here's an example of how I was using it before the gzip check was implemented.

csv_data = io.BytesIO()
writer = csv.writer(csv_data)
writer.writerows(my_data)

gz_stream = io.BytesIO()
with gzip.GzipFile(fileobj=gz_stream, mode="w") as gz:
    gz.write(csv_data.getvalue())

with smart_open(s3_path, "w") as output:
    output.write(gz_stream.getvalue())

[tmylk]

@balihoo-dengstrom Thanks for posting your use case. The smart_open for binary data use case is clear. We should support it by having r/w and gzip_decompression=True/False cases.

[mpenkov]

@tmylk @piskvorky I'll make a PR in the next day or two

[tmylk]

To be safe it might be good to throw an exception when iterating line by line over a binary file in smart_open("s3://bucket/key.gz", "r", native_gzip=False) mode

[piskvorky]

We should support the use case, but I'm still unsure about the API.

If I understand correctly, you want to bypass the .gz filename extension detection and send binary data instead, doing your own compression/decompression manually:

with smart_open("s3://some/object.gz", "wb", ignore_extension=True) as output:  # note the 'b' flag!
    output.write(gz_stream.getvalue())

By default, the contract is, smart_open looks at your file extension and does the compression/decompression transparently. So if you omit the ignore_extension=True, the expected result would be your data being compressed twice (once by gz_stream.getvalue(), then again by smart_open).

File mode / storage combinations that are not supported (like writing .bz2 to s3, etc) should throw an exception. We don't want any silent fallbacks or magic. We want to be explicit.

For iterating over "lines" in binary files, we should do the same thing that Python does in built-in open, for compatibility. I think that's treating the file as text (giving newlines special meaning).

[val314159]

I'm just going to throw this out there:

what about something like open('myfile.txt.gz','w+z') ?

On Tue, Aug 9, 2016 at 12:38 AM, Radim Řehůřek [email protected]
wrote:

We should support the use case, but I'm still unsure about the API.

If I understand correctly, you want to bypass the .gz filename extension
detection and send binary data instead, doing your own
compression/decompression manually:

with smart_open("s3://some/object.gz", "wb", ignore_extension=True) as output: # note the 'b' flag!
output.write(gz_stream.getvalue())

By default, the contract is, smart_open looks at your file extension and
does the compression/decompression transparently. So if you omit the
ignore_extension=True, the expected result would be your data being
compressed _twice_ (once in gz_stream.getvalue(), then again by smart_open
).

File mode / storage combinations that are not supported (like writing .bz2
to s3, etc) should throw an exception. We don't want any silent fallbacks
or magic. We want to be explicit.

For iterating over "lines" in binary files, we should do the same thing
that Python does in built-in open, for compatibility. I think that's
treating the file as text (giving newlines special meaning).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/RaRe-Technologies/smart_open/issues/82#issuecomment-238477080,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD-Qu3CdU9RdImUVcjNo-LkjRuWyP-IHks5qeC5-gaJpZM4JUm6y
.

[mpenkov]

@val31459 I think it's a bit too cryptic. Does anyone else do it that way?

[val314159]

That was mostly my lazy-typing self thinking. Now that I've said it out
loud, it seems like going down the road of encoding things in the second
option probably oft leads to madness.
(don't do it that way)

On Tue, Aug 9, 2016 at 10:15 AM, Michael Penkov [email protected]
wrote:

@val31459 I think it's a bit too cryptic. Does anyone else do it that way?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/RaRe-Technologies/smart_open/issues/82#issuecomment-238623454,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD-Qu72X9VH3c4Ki317qoB59MKqPcYgiks5qeLW5gaJpZM4JUm6y
.

[balihoo-dengstrom]

@val314159's suggestion isn't too far off from native reading and writing in python. Maybe something this where the binary indicator is a flag for whether or not to compress.

smart_open("s3://bucket/key.gz", "w")
smart_open("s3://bucket/key.gz", "wb")

[tmylk]

The most explicit way is still ignore_extension=True. The other ways are too prone to confusion.

[mpenkov]

What should the logic be for extensions we are not aware of (e.g. .7z)
given ignore_extension=False? Do we raise an exception, fall back to text
mode, or something else?
On Mon, Aug 15, 2016 at 09:28 Lev Konstantinovskiy [email protected]
wrote:

The most explicit way is still ignore_extension=True. The other ways are
too prone to confusion.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/RaRe-Technologies/smart_open/issues/82#issuecomment-239744022,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDOVJiOFSUShQtylQRWiowJ3SAY_Ao2ks5qgBUMgaJpZM4JUm6y
.

[tmylk]

IMHO the most explicit way is raising an exception 7z streaming not implemented. What do you think?

[balihoo-dengstrom]

Just to close the loop here, I found a workaround by using boto's new upload_fileobj method instead:

csv_data = io.BytesIO()
writer = csv.writer(csv_data)
writer.writerows(my_data)

gz_stream = io.BytesIO()
with gzip.GzipFile(fileobj=gz_stream, mode="w") as gz:
    gz.write(csv_data.getvalue())
gz_stream.seek(0)

s3 = boto3.client('s3')
s3.upload_fileobj(gz_stream, bucket_name, key)

[piskvorky]

Oh, nice! Can we make use of that for streaming directly in smart_open?

And can we use this for supporting all other modes, too? Like bzip etc.

[balihoo-dengstrom]

@piskvorky: Yeah, I think so. The new upload_fileobj method accepts any file-like object in binary mode that implements a read method.

[piskvorky]

I'm still unsure how that supports streaming (blocking until read returns?), but if it does, that's great!

@mpenkov @gojomo what do you think? Any idea how to make use of upload_fileobj in smart_open to stream-write to S3?

Btw this function seems to be boto3-only.

[mpenkov]

@piskvorky @tmylk I think before we move on with this implementation, we should confirm the problem we are trying to solve. I'm new to the library, but I think these problems are:

1. Read files from a variety of protocols (e.g. S3, HFS, local file system, etc), and
2. Read files compressed with a variety of algorithms (.gz, .tar.gz, .bz2, etc).

I think they are two separate problems, and we should solve them separately and individually (divide and conquer). The solution to 1) is to implement a file-like object for each protocol. To an extent, we have already achieved this with classes like S3OpenRead and S3OpenWrite. However, looking at it now, I think it was a mistake to integrate gzip support into S3OpenRead and S3OpenWrite in the fist place.

Why? Because it's tight coupling - something that, in principle, is best to avoid unless completely necessary. For example, if we have N protocols (S3, HFS, local file system) and M compression algorithms (.gz, .tar.gz, .bz2, 7z), then we have to implement, test and support N*M use cases: S3/gz, S3/tar.gz, S3/bz2, S3/7z, HFS/gz, and so on.

What would be a better idea? I think it'd be better to move the compression algorithm-specific parts out of S3OpenRead and S3OpenWrite.

The solution to 2) would then be to wrap the file-like object returned by 1) in a compression algorithm-specific class that would transparently read or write.

Why is this better? There are two obvious wins:

• It keeps the implementation of the protocol-specific parts simple
• It would allow us to reuse the compression-specific parts for other protocols, like HFS and the local file system.

How would this look? Inside the smart_open function, we would wrap the returned object in the necessary decompressor:

        decompressor_class = get_decompressor(uri, ignore_extension)
        bucket = s3_connection.get_bucket(parsed_uri.bucket_id)
        if mode in ('r', 'rb'):
            key = bucket.get_key(parsed_uri.key_id)
            if key is None:
                raise KeyError(parsed_uri.key_id)
            return decompressor_class(S3OpenRead(key, **kw))
        elif mode in ('w', 'wb'):
            key = bucket.get_key(parsed_uri.key_id, validate=False)
            if key is None:
                raise KeyError(parsed_uri.key_id)
            return decompressor_class(S3OpenWrite(key, **kw))

Please let me know what you think.

[tmylk]

Closing the issue as it is really a new feature request, plus a workaround was found.
New feature discussion in #91