Default to_* methods to compression='infer'
State: closed

- [x] closes https://github.com/pandas-dev/pandas/issues/22004
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This PR does the following:

- Update default compression for `to_csv`, `to_json`, and `to_pickle` methods to infer.
- Adds `test_compression_defaults_to_infer` to test that compression='infer' is default for the relevant to_* methods.
- Fixes a bug in CSVFormatter where setting `compression='infer'` with a file object would produce a RuntimeWarning.
- Adds documentation to `test_compression_warning` which can fail due to a pytest bug.
- Cleans up how the encoding argument in CSVFormatter is processed.
- Moves compression tests from `pandas/tests/test_common.py` to `pandas/tests/io/test_common.py`

Labels:
name: IO Data
description: IO issues that don't fit into a more specific label
name: IO CSV
description: read_csv, to_csv
name: IO JSON
description: read_json, to_json, json_normalize

Comments:
The following test is failing in Python 2.7 (see [travis log](https://travis-ci.org/pandas-dev/pandas/jobs/406613281#L3856)):

https://github.com/pandas-dev/pandas/blob/1033e8b1195d4071253889ada60523832285354c/pandas/tests/test_common.py#L254-L264

Let me look into https://github.com/pandas-dev/pandas/issues/21227 as to what this test is for.

**Update**: this test was added in https://github.com/pandas-dev/pandas/commit/91451cb7dbaaf6fb3f9bdfca73fe6adc2ee68cce / https://github.com/pandas-dev/pandas/pull/21478 (not https://github.com/pandas-dev/pandas/issues/21227). @minggli can you explain the purpose of `test_compression_warning`? I'm not sure why switching the compression default is causing this to fail. It seems like in Python 2, the test expects a `RuntimeWarning` that is not occurring.

Here is the relevant pytest fixture:

https://github.com/pandas-dev/pandas/blob/55cbd7d9291b2d3b9e20410a008042bab889d870/pandas/conftest.py#L131-L138
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=h1) Report
> Merging [#22011](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d30c4a0696d5fbdc3c7ce36a9b9b19224a557e09?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22011/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22011      +/-   ##
==========================================
- Coverage   92.07%   92.06%   -0.01%     
==========================================
  Files         170      170              
  Lines       50690    50704      +14     
==========================================
+ Hits        46672    46680       +8     
- Misses       4018     4024       +6
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.47% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.3% <100%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.11% <ø> (ø)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.26% <ø> (ø)` | :arrow_up: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `92.47% <ø> (ø)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.47% <ø> (ø)` | :arrow_up: |
| [pandas/io/formats/csvs.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvY3N2cy5weQ==) | `98.21% <100%> (+0.55%)` | :arrow_up: |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `94.02% <0%> (-1.04%)` | :arrow_down: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `94.87% <0%> (-0.34%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `85.69% <0%> (-0.21%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.54% <0%> (-0.14%)` | :arrow_down: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `93.79% <0%> (-0.03%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/pandas-dev/pandas/pull/22011/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=footer). Last update [d30c4a0...cf5b62e](https://codecov.io/gh/pandas-dev/pandas/pull/22011?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> Needs tests

@WillAyd I believe the different values for `compression` are already being tested, such that we don't need to test that `compression='infer'` works. Are you saying that I should be testing that changing the default actually changes the default? It seems like that could create an excessive amount of tests were we to test every default argument, but am happy to proceed as you see fit.
@dhimmel saw your comment. thanks for contributing! this `test_compression_warning` expects a RuntimeWarning when a file handle is passed to `to_csv` method with compression kwarg is supplied.

This is because it's not supported as stated in:
https://github.com/pandas-dev/pandas/blob/1033e8b1195d4071253889ada60523832285354c/pandas/io/formats/csvs.py#L134

I don't see why it would fail your work on `to_json`. 

Having checked out your PR, I saw the issue but can't replicate it on master. Are you working off the latest master branch?
> Are you saying that I should be testing that changing the default actually changes the default

Yep. Don't need to overthink it with all the various parameter combinations but at least need a test to ensure this now defaults to infer (since that is what you are changing)
> Are you working off the latest master branch?

@minggli, this PR currently branches off at 322dbf4f812f47d4eaee5d8afa10e17acf6dbc67, which is now two commits behind. I can rebase, but don't think that's the issue.

> I don't see why it would fail your work on to_json.

I was thinking the change of the to_csv compression default may have caused this issue, but it doesn't make sense to me. The test explicitly specifies compression in `df.to_csv(f, compression=compression_only)`, so I don't see how my PR would effect `test_compression_warning`.
@dhimmel,

I think the change of default in to_csv did change things. there is no infer_compression procedure in to_csv. I think that's what caused the failing of test. 

add _infer_compression in to_csv should solve this problem.

by the way, it appears that https://github.com/pandas-dev/pandas/pull/17900 has removed zip from docstrings in to_csv and to_json based on Oct 2017 discussion but zip compression for writing has been added in 0.23 Jun 2018:
https://github.com/pandas-dev/pandas/blob/67b627717db2b0d6c1b3699d9384cc433c0b9af4/doc/source/whatsnew/v0.23.0.txt#L543

could you add 'zip' back in the docstrings of to_csv and to_json?
> there is no infer_compression procedure in to_csv

Hmm. I thought to_csv now supports `compression='infer'` as of https://github.com/pandas-dev/pandas/pull/17900 by @Dobatymo and @gfyoung. Basically, compression should be inferred by `_get_handle` but it appears the zip logic operates outside of this delegation.

> could you add 'zip' back in the docstrings of to_csv and to_json?

Is there consensus on this change. IIRC from reading past issues, it was considered poor practice to save a single file to a zip archive. Thus pandas would be lenient on reading, but stricter on writing. I don't have a strong opinion, but don't want to hold up this PR for another controversial issue. So let's diagnose the failing test and see if it's somehow related to the zip issue.
#17900 added infer in _get_handle but to_csv uses compression argument before calling _get_handle; therefore raises RuntimeWarning at https://github.com/dhimmel/pandas/blob/648bf4d1810a2c2b9cbff1d4b941ab7cb7bc0b35/pandas/io/formats/csvs.py#L133 because compression='infer' instead of None as it was before. It on itself shouldn't be a problem but https://github.com/pytest-dev/pytest/issues/2917 exists so that test_compression_warning fails because the warning has been raised in earlier test before this case. simply changing the order of the test is not thread-safe so I think the best way is:

inside to_csv:
compression = _infer_compression(path_or_buf, compression)
or inside csvs
self.compression = _infer_compression(path_or_buf, compression)

In regards to zip in docstring, it's what production is showing right and supported already.
```
import os
import pandas as pd

a = pd.DataFrame(10000 * [[123, 234, 435]], columns=['A', 'B', 'C'])
a.to_csv('test_compressed', index=False, compression='zip')

b = pd.read_csv('test_compressed', compression='zip')

assert a.equals(b)
os.remove('test_compressed')
```
So I'm a bit concerned with what has happened since I last worked on compression & file IO. My impression was that we should start to use a unified API for inferring compression and opening files: see https://github.com/pandas-dev/pandas/issues/15008. `_get_handle` now delegates to `_infer_compression` . It seems however that `CSVFormatter.save` has now gotten further away from using the unified API. In https://github.com/pandas-dev/pandas/pull/22011/commits/be724fa7edcb80056cf0bdbc94351ca39d8f3d23 I replace `CSVFormatter.save` with the simplified workflow. If writing to zip is a feature that we want to support, then shouldn't this go in _get_handle?

Happy to revert https://github.com/pandas-dev/pandas/pull/22011/commits/be724fa7edcb80056cf0bdbc94351ca39d8f3d23 or put it in another PR, just wanted to see the CI test output to know what this would break.
> _get_handle now delegates to _infer_compression . 

why is this a problem? this is clear separation of concerns. I don't think the goal has drifted from when you last worked on this. A number of bugs / consolidations have happened in the interim. Happy to take further cleanup.
> why is this a problem?

Having `_get_handle` call `_infer_compression` is good. It simplifies the number of functions that individual `to_*` methods have to call. However, the issue with `CSVFormatter.save` is that performs a custom workflow for handling zip files, which has made the code quite complicated with several lines calling `_get_handle`. The issue came up because the failing test is part of this custom compression workflow CSVFormatter now contains. However, I apologize for getting a bit sidetracked. I should keep this PR focused on changing the default for compression to infer.
Hello @dhimmel! Thanks for updating the PR.

















Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on August 01, 2018 at 14:41 Hours UTC
# Why is `test_compression_warning` failing?

There is a [testing failure](https://travis-ci.org/pandas-dev/pandas/jobs/407342195#L3855) that has completely stumped me. It is only occurring in some Python 2 builds. The failing test is:

https://github.com/pandas-dev/pandas/blob/1033e8b1195d4071253889ada60523832285354c/pandas/tests/test_common.py#L254-L264

The error is:

```
E               AssertionError: Did not see expected warning of class 'RuntimeWarning'.
```

It doesn't make sense to me how the changes in this PR would effect whether the `RuntimeWarning`. This PR only changes the default compression value. The test specifies `compression` so the default should not matter.

In https://github.com/pandas-dev/pandas/pull/22011/commits/cebc0d98bd4ec7f68d3521f2814c0f41de818312 I added some debugging statements to help diagnose the testing failure

In CSVFormatter.save:

> ![warning-debug](https://user-images.githubusercontent.com/1117703/43151674-7b62f83e-8f3a-11e8-9e33-0ac41baeaa64.png)

In `test_compression_warning`:

> ![test-debug-changes](https://user-images.githubusercontent.com/1117703/43151695-8690fbf2-8f3a-11e8-9978-5a303b863501.png)

Here's the output:

```
------------------------------ Captured log call -------------------------------
test_common.py             261 WARNING  debug_1: gzip
test_common.py             266 WARNING  debug_2: gzip
csvs.py                    138 WARNING  debug_3: gzip
csvs.py                    139 WARNING  debug_4: <gzip open file '/tmp/tmpQYAr5L', mode 'wb' at 0x7f7c02e56db0 0x7f7c04698b10>
csvs.py                    141 WARNING  debug_5: True
csvs.py                    143 WARNING  debug_6: in loop, should RuntimeWarn
```

So the logging shows in the failing test that the code enters the if statement that triggers the RuntimeWarning. So I don't understand how the warning could not be raised or where this issue could be coming from.

@WillAyd other than this issue how do things look? I added a test for `to_csv` defaulting to inference.
@minggli any thoughts on the issue @dhimmel is describing above?
@dhimmel 

> #17900 added infer in _get_handle but to_csv uses compression argument before calling _get_handle; therefore raises RuntimeWarning at https://github.com/dhimmel/pandas/blob/648bf4d1810a2c2b9cbff1d4b941ab7cb7bc0b35/pandas/io/formats/csvs.py#L133 because compression='infer' instead of None as it was before. It on itself shouldn't be a problem but pytest-dev/pytest#2917 exists so that test_compression_warning fails because the warning has been raised in earlier test before this case. simply changing the order of the test is not thread-safe so I think the best way is:
> 
> inside to_csv:
> compression = _infer_compression(path_or_buf, compression)
> or inside csvs
> self.compression = _infer_compression(path_or_buf, compression)
Thanks @minggli for tracking down the [pytest issue](https://github.com/pytest-dev/pytest/issues/2917) titled "asserting repeated warning does not work in Python 2.7". I think it's very likely this is issue, however, I'm still not sure where the first RuntimeWarning is occurring that is causing pytest to malfunction when detecting the second. An easy solution would be to skip `test_compression_warning` on Python 2. However, I'd like to know where in the tests changing the default for compression to infer is causing a RuntimeWarning, in case we should just fix that test.
@dhimmel ,

to_csv should not raise RuntimeWarning when user didn't explicitly specify a compression and pass a file-like object, so the tests work fine. In other words, now that there is 'infer' as default, the RuntimeWarning raises all the time, which is undesirable, and should only raise when user passes explicitly and a file-handle is given.

So, I feel fixing the test is wrong way about it (the test affected by default 'infer' is test_compression_size_fh) and we should instead pass inferred compression keyword into CSV formatter or during construction of it. 
@shangyian I saw you're the author of https://github.com/pandas-dev/pandas/pull/20341. Was there a specific reason why the encoding processing is performed in `CSVFormatter.save` rather than `CSVFormatter.__init__`? Does the movement of encoding logic as of https://github.com/pandas-dev/pandas/pull/22011/commits/eb7f9b5491defe035231e87f7902f1ea8786e86d raise any concerns?
> to_csv should not raise RuntimeWarning when user didn't explicitly specify a compression and pass a file-like object

@minggli got it! I've added `_infer_compression` to `CSVFormatter.__init__`. This fixes the bug where `CSVFormatter.save` raises a RuntimeWarning when a file object is passed and `compression='infer'`. By preventing undesired RuntimeWarnings in other tests, we no longer hit the pytest bug in `test_compression_warning` in Python 2. I've also added a docstring to `test_compression_warning`, so if other users hit the pytest bug, they will hopefully not have to repeat our laborious debugging.

In the longterm, I still think it makes sense for any non-CSV-specific file/compression handling to be moved to the common module. However, that can be done in the future.
@WillAyd [Travis jobs](https://travis-ci.org/pandas-dev/pandas/builds/408516636) have all passed, if you want to take another look. No outstanding items from my end.
> I'm thinking that one fully parametrized test for compression inference by default is enough. This could either be the current modifications to test_compression_size or it could be a rewritten test_compression_defaults_to_infer

Your point is understood but we typically like to have small, very directed unit tests to keep a logical separation of features tested. As such, there should (or already is in fact) one test that ensures that files actually get compressed as part of the process. A separate test entity for "ensuring that the default compression is set to 'infer'" is what you are introducing with this change. 

The structure I was hinting at would keep those two "features" distinct in tests and give a better logical separation. Again the smaller and more directed tests can be, the better. There are plenty of older tests in our suite that try to do a lot of things at once - if you don't believe me take a look at them :-)

lmk if that's not clear
> A separate test entity for "ensuring that the default compression is set to 'infer'" is what you are introducing with this change.

Okay, the only test that this now changes (besides comments) is a new test `test_compression_defaults_to_infer`. It uses a round-trip approach to testing compression. This test is parametrized as follows:

```python
@pytest.mark.parametrize('input', [
    DataFrame([[1.0, 0, -4.4],
               [3.4, 5, 2.4]], columns=['X', 'Y', 'Z']),
    Series([0, 1, 2, 4], name='X'),
])
@pytest.mark.parametrize('methods', [
    ('to_csv', pandas.read_csv),
    ('to_json', pandas.read_json),
    ('to_pickle', pandas.read_pickle),
])
def test_compression_defaults_to_infer(input, methods, compression_only):
```

@WillAyd do you agree that this is the minimal test to exhaustively test that supported `to_*` methods default to infer? I think it's important we test Series AND DataFrame as well as to_json, to_csv, AND to_pickle. Hence, I put the test in `test_common.py` rather than `test_to_csv.py`.
Had to split test into two in https://github.com/pandas-dev/pandas/pull/22011/commits/fadb943851e395cfd6e7dbe05ba86d15ea2ee6f2: one for series and one for dataframes. Basically, the DataFrame & Series roundtrips require different args. Still a WIP, but the filesize test is considerably simpler given the intricacies of roundtripping.
@WillAyd round-tripping a series to a CSV with to_csv and read_csv is not straightforward. There is the deprecated pandas.Series.from_csv, but it doesn't support compression. Take a look at the current PR... do you still think round-tripping is preferable to file size comparing, given the complexity of round-tripping? Is there an easier way to roundtrip the CSV?
@dhimmel - there wasn't a specific reason, so yeah, go ahead and move it if it makes more sense!
@WillAyd ready for re-review. Tests passing as of https://github.com/pandas-dev/pandas/pull/22011/commits/14c3945410288a8edd3ee989d5582b1e5df0e992 and the following commit just modifies comments.

> you might also want to take a look at #19715 to make sure there aren't any conflicts.

There may be some minor git conflicts since it looks like @toobaz's commits move where the `Series.to_csv` code is located: see https://github.com/toobaz/pandas/commit/191d2ab56249f0c60786cdb24b35dc7b7c92890f. However, all we touch in that code is the compression default and docstring, so it shouldn't be too bad for @toobaz to rebase on top of these changes (I didn't see a WIP PR with his/her changes).
> There may be some minor git conflicts since it looks like @toobaz's commits move where the Series.to_csv code is located: see toobaz@191d2ab. However, all we touch in that code is the compression default and docstring, so it shouldn't be too bad for @toobaz to rebase on top of these changes

Sure, no problem




> (I didn't see a WIP PR with his/her changes)

#21896
can you rebase
> can you rebase

I merged in the latest master (https://github.com/pandas-dev/pandas/pull/22011/commits/25bdb4cecdb15e70e4ef217c883def2eb4137274) and took care of the conflict. Usually I rebase, but I wanted to preserve the commit history in the PR since we reference commit hashes and CI builds frequently in the discussion. Hope that is OKAY.

CC @WillAyd, I think we should be ready for your re-review.