Openpyxl engine for reading excel files
State: closed

- [x] closes #11499
- [x] tests added 
- [x] tests passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Builds upon #26233, so that should be merged first.

This PR adds an almost fully compatible OpenPyXL based engine to excel_read, next to the xlrd engine.

Using OpenPyXL will also allow reading and writing in excel tables referenced by name which I intend to put in a future PR, see also discussion of #24862.

As the current xlrd implementation is pretty tightly coupled to the the text based parsing options, there are many supported keywords in excel_read that I think eventually do not belong there. With this PR I left all of them in and support them as much as possible even though this:

 - Make the code much more complicated
 - Makes the code slower
  
The way this is implemented is that the excel file is read into a dataframe early, and all the keywords like `convert`, `dtype`, `convert_float` etc are dealt with by modifying the dataframe. Because of this some of the parser code needed to be re-implemented, so this is not ideal. An alternative would be to just not support these keywords at all for the openpyxl engine.

Labels:
name: Enhancement
name: IO Excel
description: read_excel, to_excel

Comments:
Hello @tdamsma! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:











There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-06-28 14:48:52 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=h1) Report
> Merging [#25092](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/bb43726e1f52a0ddee45fcf485690719f262870d?src=pr&el=desc) will **decrease** coverage by `49.49%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #25092      +/-   ##
==========================================
- Coverage   92.37%   42.88%   -49.5%     
==========================================
  Files         166      166              
  Lines       52420    52420              
==========================================
- Hits        48423    22479   -25944     
- Misses       3997    29941   +25944
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.88% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| [pandas/core/tools/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvbnVtZXJpYy5weQ==) | `10.44% <0%> (-89.56%)` | :arrow_down: |
| ... and [123 more](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25092?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/25092?src=pr&el=footer). Last update [bb43726...821fa4d](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=h1) Report
> Merging [#25092](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d47fc0cba3cf94ebd289ad3776bf7ff3fe60dfb8?src=pr&el=desc) will **decrease** coverage by `50.58%`.
> The diff coverage is `14.81%`.

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

```diff
@@             Coverage Diff             @@
##           master   #25092       +/-   ##
===========================================
- Coverage   91.71%   41.13%   -50.59%     
===========================================
  Files         178      178               
  Lines       50771    50931      +160     
===========================================
- Hits        46567    20949    -25618     
- Misses       4204    29982    +25778
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.13% <14.81%> (-0.15%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/excel/\_openpyxl.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19vcGVucHl4bC5weQ==) | `12.97% <14.37%> (-71.74%)` | :arrow_down: |
| [pandas/io/excel/\_base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19iYXNlLnB5) | `28.5% <50%> (-63.32%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/plotting/\_matplotlib/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19tYXRwbG90bGliL19faW5pdF9fLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.37%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.16%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.1%)` | :arrow_down: |
| ... and [134 more](https://codecov.io/gh/pandas-dev/pandas/pull/25092/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25092?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/25092?src=pr&el=footer). Last update [d47fc0c...614d972](https://codecov.io/gh/pandas-dev/pandas/pull/25092?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@WillAyd This is still a work in progress, but feedback would be welcome. I had to make a few adjustments to the tests to be able to run them for a different engine. 
As openpyxl is pretty smart in parsing the excel files to data I decided to not use the TextParser of PythonParser, but (re-implement) some if the api specifically for the OpenPyXL reader. Not sure if this is really desirable, but as the parsers all seem to be meant to deal with text files this felt like the best approach. On my local machine 70 of the 78 tests for openpyxl reader are passing 
I think i mentioned this before
but would like to split up the excel.py first to make a subdir then each reader / writer can go in slimmer files

would take a precursor PR to do this
oops, think I missed that. That would certainly help as the file is huge.
@WillAyd I somehow can't comment inline on 

> This should be reverted and just parametrize on `openpyxl` as well

but I really see how this should work with how the tests are set-up now. As i see it the `TestXlrdReader` is responsible for running the `ReadingTestsBase` tests with the 'xlrd' engine. So I added a `TestOpenpyxlReader` class for running the `openpyxl` tests. But as the subclasses are responsible for defining the engine, there is no need for the baseclass to parametrize the engine. In fact this would cause the xlrd specific tests to be run with the openpyxl engine? 

Of course the writer tests follow a different pattern This leads to a pretty complicated decorator like 
```
@pytest.mark.parametrize("engine,ext", [
    pytest.param('openpyxl', '.xlsx', marks=pytest.mark.skipif(
        not td.safe_import('openpyxl'), reason='No openpyxl')),
    pytest.param('openpyxl', '.xlsm', marks=pytest.mark.skipif(
        not td.safe_import('openpyxl'), reason='No openpyxl')),
    pytest.param('xlwt', '.xls', marks=pytest.mark.skipif(
        not td.safe_import('xlwt'), reason='No xlwt')),
    pytest.param('xlsxwriter', '.xlsx', marks=pytest.mark.skipif(
        not td.safe_import('xlsxwriter'), reason='No xlsxwriter'))
])
```

and then every set of engine specific tests need redefine which extension and engine combinations it should run for. I guess judging from the naming (`ReadingTestsBase` vs `_WriterBase`) the sets of read and write tests follow different patterns anyway


@jreback, @WillAyd I completly agree that there are lots of refinements to be made to this code, but I deliberatly did not put too much effort into that because I would like some kind of approval on the general approach before I put a lot of effort into something that may never be merged anyway.


The way this is implemented is that the excel file is read into a dataframe early, and all the keywords like `convert`, `dtype`, `convert_float` etc are dealt with by modifying the dataframe. Because of this some of the parser code needed to be re-implemented, so this is not ideal. An alternative would be to just not support these keywords at all for the openpyxl engine.
The general structure I chose if convert the excel file to a dataframe with openpyxl as soon as possible, and then lots of "silly" code to support keywords like `convert`, `dtype`, `convert_float` etc that make only limited sense here purely for compatibilty.

If you agree with that approach than I will happily work on the code to bring it up to standards. An alternative approach could be to drop support for some keywords in favor of simplicity of the code (but then the set of supported keywords would be engine dependent). 

Finally, I am struggling a bit with some of the tests. Currently 7 are failing, 4 are passing but I can't figure out why e.g. Windows py36_np14 is not working. Any assistance would be much appreciated
I think general approach is fine just want to be cautious of code duplication and complexity, so it's something that can be addressed iteratively during review.

You should be able to click through the logs to see the failure. In this case it's `test_reader_special_dtypes` which you'll see below

https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=8762&view=logs&jobId=60e7d5ab-17b4-5716-5939-f0e150f5044f&taskId=e747a90c-d7dd-5b19-0967-6bf1e91fb8b8&lineStart=692&lineEnd=693&colStart=1&colEnd=1
@tdamsma can you merge master here?
> @tdamsma can you merge master here?

Done. Unfortunately I haven't had much time/energy work on this lately, but now the python 2.x support is fully dropped I'll look into it again
@tdamsma looks like errors in CI as a result of latest commit - can you merge master and check errors?
can you merge master
Just merged master (and now all tests fail..), I'll have a go at refactoring and fixing the tests tomorrow
@WillAyd and @jreback , I wanted to get all the tests passing before I refactor the code. I almost got there except the linux_37_np_dev build. It fails in pandas/tests/series/test_missing.py, something I have not touched as far as I'm aware.
Try one more merge of master - just pushed through a fix for the doc failures that are there now. 

Sent from my iPhone

> On Apr 22, 2019, at 11:56 PM, Thijs Damsma <notifications@github.com> wrote:
> 
> @WillAyd and @jreback , I wanted to get all the tests passing before I refactor the code. I almost got there except the linux_37_np_dev build. It fails in pandas/tests/series/test_missing.py, something I have not touched as far as I'm aware.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

> Try one more merge of master - just pushed through a fix for the doc failures that are there now.
> […](#)
> Sent from my iPhone
> On Apr 22, 2019, at 11:56 PM, Thijs Damsma ***@***.***> wrote: @WillAyd and @jreback , I wanted to get all the tests passing before I refactor the code. I almost got there except the linux_37_np_dev build. It fails in pandas/tests/series/test_missing.py, something I have not touched as far as I'm aware. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Thanks, finally all the tests pass! I hope to do the refactoring in the next few days
@WillAyd and @jreback , I refactored most of the code to something hopefully a bot more comprehensible. Could you re-review?
can you merge master
@WillAyd and @jreback, awaiting your feedback. There are a few things I would like to point out:

- The way I added the Openpyxl tests. I followed the structure of the existing code, however in the existing code there is a difference in setup between the excel reading tests and the writing tests. The writing tests follow the model of one set of tests parametrized to run for all engines, and then engine dependent classes for the special cases. The read tests follow the model of subclassing a base test for each engine. So this is a bit inconsistency here. For the record, I find the latter model cleaner, as it reduces the complexity of the parametrization statements and IMHO better encapsulates the code for a single engine.
- I sprinkled a few `pytest.skip` cases in the tests, where I felt the test was testing something very specific to xlrd. If that is agreed, these tests can be moved to the xlrd testclass.
- There are two tests where I don't understand what is going on. Specifically `assert local_table.equals(url_table)` passes, but `tm.assert_frame_equal(url_table, local_table)` doesn't. I haven't been able to get to the bottom of this yet.
@WillAyd and @jreback, thanks for the reviews! I haven't been able to make time lately to address all the issues, might be a week or two till I can back at this. I'll probably start with a precursor to get the read tests consistent with the write tests, and then take it from there. Sorry it is taking so long, just a lot of other things going on. 
@tdamsma the excel tests have been overhauled so if you can merge master and give this another look should be easier to integrate on the testing side of things. That may also resolve some of the errors or at the very least make them easier to debug
For the sake of explicitness you should be able to add openpyxl as an engine here:

https://github.com/pandas-dev/pandas/blob/5574a9fe2d1bcb3378cce08b5557d4384e5c36dd/pandas/tests/io/excel/test_readers.py#L39

Within that function I think you'll also have to `pytest.skip()` if the `engine` is openpyxl and the `ext` is .xls

cc @simonjayhawkins something to consider as we look to support .odf files as well is how we map engine and ext support in these tests (orthogonal to issue here but providing for visibility)
@WillAyd, nice! Have merged, really nice improvement overall. Some remarks: 
 - How do I skip a test for a certain engine? I hacked something in, but doesn't look right. 
 - I don't really like the code I wrote for skipping .xls files in combination with openpyxl, is there a better way?
Yea great question. I would try to differentiate between "this test really only makes sense for xlrd" versus "this could work with openpyxl but haven't implemented yet". Items in the former should be moved to `test_xlrd` whereas the latter are OK to use the `pytest.skip()` imperatively
@tdamsma this is a great change hoping to get in soon. We are sprinting on pandas so made some changes - would welcome any input you've got!
Great, thanks for pitching in/taking over as I was not getting to it myself (feel kind of bad not being available for this a bit more). Looks like you found a way to get rid of most of the code, great! I'll make some time to see if I have anything to add. 
I made a few minor changes and all tests are passing now. There is just an issue with the type checking. I haven't used type checking yet so not sure how to solve the import error, or if we should just not bother checking the openpyxl types

Great thanks for the help! I think mypy was getting tripped up by optional dependencies. I'm not sure of best resolution but for now removed annotations from openpyxl and added elsewhere to get things green
@tdamsma I don't think GH liked that ha! Can you reopen the PR on your end? Grayed out for me