Remove compression from xopen
State: closed
Labels: description: Generic discussion on the library
name: generic discussion

We implemented support for streaming with 2 requirements:
- transparent use for the end user: just needs to pass the parameter `streaming=True`
- no additional work for the contributors: previous loading scripts should also work in streaming mode with no (or minor) changes; and new loading scripts should not involve additional code to support streaming

In order to fulfill these requirements, streaming implementation patched some Python functions:
- the `open(urlpath)` function was patched with `fsspec.open(urlpath)`
- the `os.path.join(urlpath, *others)` function was patched in order to add to `urlpath` hops (`::`) and extractor protocols (`zip://`), which are required by `fsspec.open`

Recently, we implemented support for streaming all archive+compression formats: zip, tar, gz, bz2, lz4, xz, zst; tar.gz, tar.bz2,...
Under the hood, the implementation:
- passes an additional parameter `compression` to `fsspec.open`, so that it performs the decompression on the fly: `fsspec.open(urlpath, compression=...)`

Some concerns have been raised about passing the parameter `compression` to `fsspec.open`:
- https://github.com/huggingface/datasets/pull/2786#discussion_r689550254
- #2811 

The main argument is that if `open` decompresses the file and afterwards we call `gzip.open` on it, that will raise an error in `oscar` dataset:
```python
gzip.open(open(urlpath
```
While this is true:
- it is not natural/usual to call `open` inside `gzip.open` (never seen this before)
- indeed, this was recently (2 months ago) coded that way in `datasets` in order to allow streaming support (with previous implementation of streaming)

In this particular case, there is a natural fix solution: #2811:
- Revert the `open` inside the `gzip.open` (change done 2 months ago): `gzip.open(open(urlpath` => `gzip.open(urlpath`
- Patch `gzip.open(urlpath` with `fsspec.open(urlpath, compression="gzip"` 

Are there other issues apart from this?

Note that there is an issue just because the open inside of the gzip.open. There is no issue in the other cases where datasets loading scripts use just
- `gzip.open` 
- `open` (after having called dl_manager.download_and_extract)

TODO:
- [ ] Is this really an issue? Please enumerate the `datasets` loading scripts where this is problematic.
  - For the moment, there are only 3 datasets where we have an `open` inside a `gzip.open`:
    - oscar (since 23 June), mc4 (since 2 July) and c4 (since 2 July)
  - In the 3 datasets, the only reason to put an open inside a gzip.open was indeed to force supporting streaming
- [ ] If this is indeed an issue, which are the possible alternatives? Pros/cons?

Comments:
After discussing with @lhoestq, a reasonable alternative:
- `download_manager.extract(urlpath)` adds prefixes to `urlpath` in the same way as `fsspec` does for protocols, but we implement custom prefixes for all compression formats: 
  `bz2::http://domain.org/filename.bz2`
- `xopen` parses the `urlpath` and extracts the `compression` parameter and passes it to `fsspec.open`:
  `fsspec.open("http://domain.org/filename.bz2", compression="bz2")`

Pros:
- clean solution that continues giving support to all compression formats
- no breaking change when opening non-decompressed files: if no compression-protocol-like is passed, fsspec.open does not uncompress (passes compression=None)

Cons:
- we create a "private" convention for the format of `urlpath`: although similar to `fsspec` protocols, we add custom prefixes for the `compression` argument