Refactor compression code to expand URL support
State: closed

Work in progress. Picks up where #13340 left off.
 
closes #12688 
closes #13340 
closes #14570


Labels:
name: IO Data
description: IO issues that don't fit into a more specific label

Comments:
> Can you add tests for the new features you added? (the compressed sources from urls)

@jorisvandenbossche, do you want me to add xz/zip/bz2 compressed versions of `salary.table.csv` to [`pandas/io/tests/parser/data`](https://github.com/pandas-dev/pandas/tree/e1cdc4b83d7e970e9683056be722d5fa2a00fa70/pandas/io/tests/parser/data)? Also should I add my tests to [`pandas/io/tests/parser/test_network.py#L17-L35`](https://github.com/pandas-dev/pandas/blob/e1cdc4b83d7e970e9683056be722d5fa2a00fa70/pandas/io/tests/parser/test_network.py#L17-L35)?

Are we also going to have to do something to [`common.py#L239-L244`](https://github.com/pandas-dev/pandas/blob/e1cdc4b83d7e970e9683056be722d5fa2a00fa70/pandas/io/common.py#L239-L244)?

@dhimmel yes that should be more general

## [Current coverage](https://codecov.io/gh/pandas-dev/pandas/pull/14576?src=pr) is 85.29% (diff: 88.40%)
> Merging [#14576](https://codecov.io/gh/pandas-dev/pandas/pull/14576?src=pr) into [master](https://codecov.io/gh/pandas-dev/pandas/branch/master?src=pr) will increase coverage by **0.02%**

```diff
@@             master     #14576   diff @@
==========================================
  Files           144        144          
  Lines         50980      50943    -37   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          43470      43453    -17   
+ Misses         7510       7490    -20   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [14e4815...b33a500](https://codecov.io/gh/pandas-dev/pandas/compare/14e4815391dcd8c9fe91479fed629410bf63ca33...b33a500120b63836d64f50c5f4e4504099160585?src=pr)
ideally if you can use some of this code in the parser routines would be great (may not be completely possible, but want to consolidate some of that duplication)

Regarding [`common.py#L239-L244`](https://github.com/pandas-dev/pandas/blob/e1cdc4b83d7e970e9683056be722d5fa2a00fa70/pandas/io/common.py#L239-L244), I think there's a problem. Currently, when `compression="infer"` and the `path_or_buff` is a URL, `get_filepath_or_buffer` is using the [Content-Encoding](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding) header of the request to infer the compression type. However, I don't actually think compressed files on the network will provide a Content-Encoding header of their compression type. Instead I think we want to infer the compression of a URL from it's name, just as we do with paths.

For example, `https://raw.githubusercontent.com/pandas-dev/pandas/master/pandas/io/tests/parser/data/salaries.csv.bz2` would infer `bz2` compression because it ends with `.bz2`. @jreback and @jorisvandenbossche, agree?
i think inferring based on the file name is usually enough - but u can also use the Content if it's available
I'm getting [failing builds in AppVeyor](https://ci.appveyor.com/project/jreback/pandas-465/build/1.0.2307/job/1f7g05x811genspf) in Python 2 due to `ImportError: No module named backports`. This occurs in `compat/__init__.py` at:

```python
    def import_lzma():
        """ import the backported lzma library
        or raise ImportError if not available """
        from backports import lzma
        return lzma
```

Any idea why `backports` is not available? How should I proceed?

**Update**: I think I resolved this in https://github.com/pandas-dev/pandas/pull/14576/commits/af3c2bc051d7c7178c617c211bf1fe0e14c5b5bd.
@jorisvandenbossche, I added the `c` engine tests in https://github.com/pandas-dev/pandas/pull/14576/commits/e78d022949cd610276d83a820c45768840c25d0a. But we're gettings errors in the following [tests](https://travis-ci.org/pandas-dev/pandas/jobs/181193644):

```
ERROR: Test reading bz2-compressed tables from URL: compression="bz2", engine="c"
ERROR: Test reading bz2-compressed tables from URL: compression="infer", engine="c"
```

due to `ValueError: Python 2 cannot read bz2 from open file handle`. This error is coming from [`parser.pyx#L615-L621`](https://github.com/pandas-dev/pandas/blob/b3dd9bafbfcbac1a2b6ab756e588d6e167f90439/pandas/parser.pyx#L615-L621). This code looks to duplicate functionality of `_get_handle`, but is maybe Cython code? Anyways, I've never worked with `.pyx` code. Suggestions? Should we save `c` engine for a future PR?

Yes, let's leave issues with the C engine for another PR (also the pathlib stuff, there is an issue for it), and lets try to finish this PR. 
Still haven't migrated the handle tracking logic to `_infer_compression` as @jorisvandenbossche suggested. The main thing holding me up is that I don't know exactly what should be considered a handle. Is this a prerequisite to merging this PR, or should we address later? 

> Yes, let's leave issues with the C engine for another PR

Okay c engine tests removed in https://github.com/pandas-dev/pandas/pull/14576/commits/bc9304389996496d92a72789c86b95b328e7925b. Prior to removing, the c-engine [tests](https://travis-ci.org/pandas-dev/pandas/builds/181502054) were all passing in 3. In 2.7 with the c-engine, the `bz2` reading from URL fails, but the other compression methods work.

I'd consider future work to add more S3 tests for all compression types. Also consolidating the [c-engine code](https://github.com/pandas-dev/pandas/blob/b3dd9bafbfcbac1a2b6ab756e588d6e167f90439/pandas/parser.pyx#L615-L621) that's making bz2 fail in 2.

When this PR is merged, it's important to me to have my authorship preserved in the commit history. I bring this up, since the first commit in this PR is from @lababidi -- I'm assuming he/she also cares that his/her authorship is preserved. So just let me know if you'd like me to squash this thing in a certain way / how we should proceed.

Thanks for seeing this through. Can't wait to use the feature going forward and happy to keep working on these issues as needed.
> The main thing holding me up is that I don't know exactly what should be considered a handle.

The handles we need to track, are all the handles (open files or file like objects) that pandas opens *itself* (not file handles provided by the user). You can check from the current `add_handle` boolean expression which ones it should be (those checks correspond to parts in the code of `_get_handle`. Tracking those opened handles inside `_get_handle` will make it a lot clearer what is going on.

I would like to see this fixed before merging

> I'd consider future work to add more S3 tests for all compression types. Also consolidating the c-engine code that's making bz2 fail in 2.

Can you open issues for those to keep track of them.

> When this PR is merged, it's important to me to have my authorship preserved in the commit history.

That's not problem, for sure! We can just squash your additional commits into one commit, and then commit those two commits of two authors to master.
Will get around to finishing this early next week. Currently traveling.
can you rebase, problem with appeveyor which i just fixed
Assuming the tests pass for https://github.com/pandas-dev/pandas/pull/14576/commits/640dc1b0ed88bfdb9164083077d17af6a87ba826, I don't think there are any more unaddressed comments.

> can you rebase, problem with appeveyor which i just fixed

@jreback rebased with appveyor now passing.

> Can you open issues for those to keep track of them.

@jorisvandenbossche: I created #14874 and #14875.

> We can just squash your additional commits into one commit, and then commit those two commits of two authors to master.

@jorisvandenbossche do you want me to squash all of my commits? If so, what should go in the commit message? Should I add the [issues we're closing](https://github.com/pandas-dev/pandas/pull/14576#issue-187096549) to the commit message?
@dhimmel you can squash or it will get squashed on the merge. in this case if you'd leave the original commit, and squash all of our subsequent ones to a single (or possibly) multiple, logically oriented commits would be great. We could in this case cherry-pick them (rather than completely squashing), to preserver authoriship (as I think when we squash ALL then authoriship is the last one). if we only have 2 commits a merge is easy.
> in this case if you'd leave the original commit, and squash all of our subsequent ones to a single (or possibly) multiple, logically oriented commits would be great. 

I'm going to squash all of my commits. Grouping them will be pretty burdensome and the intermediate commits were often not functional. I [created a branch](https://github.com/dhimmel/pandas/commits/compression-unsquashed) on `dhimmel/pandas` to preserve the individual commits for reference. 

Just to list things here, `git log master.. --oneline` gives the following:

640dc1b Simplify TextIOWrapper usage
4774b75 Use list to store new handles
94aa0cb Track new handles in _get_handle https://github.com/pandas-dev/pandas/pull/14576#issuecomment-265283737 https://github.com/pandas-dev/pandas/pull/14576#discussion_r91988117
376dce3 Close bz2 file object in Python 2
b88d1b8 Remove compressed URL c-engine tests
b34c2f6 Move _infer_compression to io/common.py
d27e57d Fix "ImportError: No module named backports"
97a1c25 Test compressed URLs using c engine
ecee74f Revert to old error msg. Remove unused line
7c9fe8b Skip xz test if no lzma
492e615 Remove unused imports
bf9fe97 Add invalid compression test
7940ab7 Fix PEP8 errors
05332fd Update S3 get_filepath_or_buffer compression
24ea605 Fix get_filepath_or_buffer and _get_handle
2dc1f2f Clean up docstring
10652a0 Move compression inference to io/parsers
8d24bcf Delete unused function _wrap_compressed
83b2bc5 Infer compression from URL extension
c2e6e5b Improve error message, fix partial scope
9977da2 Fix nose test generator bug. Better error messages
7ff1247 Inheret from object for TestCompressedUrl
30e1caf Fix local salaries.csv path
6230dfb Update salaries.csv URL post #14587
6530f46 Enable nose execution of test generators
a441448 Broken: use method generators to DRY tests
25114c3 Tests for bz2, xz & zip URLs
89e45f4 Revert changes to zip error messages
29b7f74 Change error message in test_zip
3aba6cb Replicate original handles appending
101a344 Impove _get_handle docstring
58ffbce Rename source to path_or_buf and flake8
f0901fb Remove ZipFile support for Python 2.6
2c78178 Improve readability
592cf92 Fix ZipFile bug in Python 3.6+
74caf72 combine compression code into one

36 commits total in this PR. Wow!
> if we only have 2 commits a merge is easy.

Great we're now at 2 commits. I think merge makes sense, because it is nice to see the diff of both commits combined. Alone the changesets will be incomplete.

Let me know if there's anything more for me to do.
@jreback You can use the github interface as well to "rebase and merge" instead of squashing, which will just apply the commits in this PR to master. And so the two commits now in the PR are perfect!
@jorisvandenbossche right, but this is the exception to the rule.  I will have a look in a bit. 

@dhimmel going to merge, and can followup on comments / issues in another PR.
thanks @dhimmel very nice!

in your xref PR, pls add whatsnew that cover all of the issues that were closed (for 0.20.0)
@dhimmel thanks a lot!