DEPR: Deprecate using `xlrd` engine for read_excel
- [x] closes #28547
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This is MR #29375 but rebased to master
Yea should warn first can actually change in 2.0

Sent from my iPhone

> On Jul 3, 2020, at 1:25 PM, Robert de Vries <notifications@github.com> wrote:
> 
> ﻿
> @roberthdevries commented on this pull request.
> 
> In pandas/io/excel/_base.py:
> 
> > @@ -844,14 +844,24 @@ class ExcelFile:
>  
>      def __init__(self, path_or_buffer, engine=None):
>          if engine is None:
> -            engine = "xlrd"
> +            engine = "openpyxl"
> @WillAyd Should I revert the change to make openpyxl the default engine?
> And only warn when using xlrd in combination with .xlsx or .xlsm files?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
 @roberthdevries canyou update to comments and merge master @roberthdevries can you address comments? Not until I am back from vacation in three weeks. I have addressed the remaining comment from @WillAyd to only warn about the pending deprecation. just because I didn't see it mentioned above, I thought I'd mention that `openpyxl` is 10x slower. So it's a good thing that the default is not being changed, because otherwise a lot of user would see an impact after upgrading. > just because I didn't see it mentioned above, I thought I'd mention that `openpyxl` is 10x slower. So it's a good thing that the default is not being changed, because otherwise a lot of user would see an impact after upgrading.

Interesting, what did you base this on? > Interesting, what did you base this on?

Testing based on files I'm working on. After seeing the deprecation notice, we switched our code, and found out our CI was taking one hour longer (!). We traced it back to `openpyxl`. Fortunately we were aware we had done that change. If pandas had changed the default silently, it would have taken us a long time to figure out what was going on. @roberthdevries This branch has conflicts that must be resolved. can you also address @WillAyd comments. https://github.com/pandas-dev/pandas/pull/35029#pullrequestreview-475475584 This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. Closing for now. @roberthdevries ping us whenever you'd like to continue and we'll reopen! @pandas-dev/pandas-core this would be good to do for 1.2, anyone interested in starting from here and putting up a PR? I can finish this off, should be able to get a PR up by tomorrow morning. A few notes that I wasn't 100% sure on:
 - I don't believe a doc page for ExcelFile gets created, but added the deprecation to the docstring there anyways.
 - For the future warning, stacklevel should be 2 for ExcelFile and 4 for pd.read_excel. I think 2 (and in general, the minimum) is correct here because that way there is an actual trace in all cases, but wasn't certain.
 - Added a deprecation note to the read_excel page:

![image](https://user-images.githubusercontent.com/45562402/100294481-eb843300-2f54-11eb-8ce5-8332f7910e47.png)

cc @jreback @simonjayhawkins @WillAyd  > Because if not, we could also already use openpyxl _if installed_, and only warn if xlrd is the only available engine.

AFAIK openpyxl is slower than xlrd where they overlap, and so it might look like a perf issue to silently switch. Might have to be noisy about the default changing for a release or two to prompt users who care to specify a choice. > > Because if not, we could also already use openpyxl _if installed_, and only warn if xlrd is the only available engine.
> 
> AFAIK openpyxl is slower than xlrd where they overlap, and so it might look like a perf issue to silently switch. Might have to be noisy about the default changing for a release or two to prompt users who care to specify a choice.

I can confirm the performance issue:

> After seeing the deprecation notice, we switched our code, and found out our CI was taking one hour longer (!). We traced it back to openpyxl. @luk-f-a with what size of files did you notice it? And what factor of slowdown?

I did a quick test locally with a file of 10,000 rows, 6 columns (5 floats and 1 string), and reading that with xlrd takes around 500ms, with openpyxl around 900ms (so indeed slower).

But so, it can indeed be slower. But even then, there is not yet really an alternative? (apart from keep relying on a dead project until is actually breaks)

Does somebody know if xlrd/openpyxl have APIs to get the size of an excel file / sheet? (eg possible idea could be to take a threshold for which we warn, and for small files where performance is not an issue, we could then still automatically use openpyxl when available)  @jorisvandenbossche I have a file with 38 rows and 2 columns (tiny!), taking 5.65s with openpyxl and 387ms with xlrd. Since it seems that openpyxl is impossible to avoid in the long term I'll open an issue with openpyxl. This difference is too large for such a simple file.

 > I have a file with 38 rows and 2 columns (tiny!), taking 5.65s with openpyxl and 387ms with xlrd. Since it seems that openpyxl is impossible to avoid in the long term I'll open an issue with openpyxl. This difference is too large for such a simple file.

Is the file heavily formatted?  This is shockingly long. at first sight it looks like a normal table. there must be something invisible because copying and pasting the content (including text format) into a new file reduces the time.  Is it an excel file you can share? 
But indeed, that sounds as an issue in openpyxl, so would be good to report it there. I was about to share it but then I found the reason for the slow reading performance: a link to another file. Even though no formula uses that link, and independently of whether that file exists or not (I tested on two different computers), it slows down reading the excel file. Unfortunately now I cannot share it because the link shows the entire internal file structure. I tried to reproduce by linking to another file (with a safe path) but the problem does not re-appear.
So maybe the performance problem I had is not so common, only with some kind of linked files. cc @jreback @jorisvandenbossche

Thanks for the reviews, changes made. With the change from emitting a FutureWarning to now a DeprecationWarning, I don't see a way to check stacklevel in the tests because of the other DeprecationWarning. So even though the stacklevel is now properly set for both read_excel and ExcelFile, it isn't tested for.

In updating the docs,  `user_guide/io.rst` has some examples with xls files. I will open an issue to remove/convert those.

A few minor questions:
 - In adding `engine="openpyxl"` to read_excel calls in the docs to avoid the warnings, I even did the 0.17 whatsnew. I think that's okay?
 - Should a note/explanation about the deprecation be added to `intro_tutorial/02_read_write.rst`, `user_guide/10min.rst`, and `user_guide/io.rst` where we need to add engine="openpyxl"? I can do so tomorrow.
 > cc @jreback @jorisvandenbossche
> 
> Thanks for the reviews, changes made. With the change from emitting a FutureWarning to now a DeprecationWarning, I don't see a way to check stacklevel in the tests because of the other DeprecationWarning. So even though the stacklevel is now properly set for both read_excel and ExcelFile, it isn't tested for.
> 
> In updating the docs, `user_guide/io.rst` has some examples with xls files. I will open an issue to remove/convert those.
> 
> A few minor questions:
> 
> * In adding `engine="openpyxl"` to read_excel calls in the docs to avoid the warnings, I even did the 0.17 whatsnew. I think that's okay?
> * Should a note/explanation about the deprecation be added to `intro_tutorial/02_read_write.rst`, `user_guide/10min.rst`, and `user_guide/io.rst` where we need to add engine="openpyxl"? I can do so tomorrow.

also would add a big warning box in the io.rst for reading excel about this change. also *maybe* add in the top of the whatsnew as well (a warning box). @jreback Changes made, added the warnings to both io.rst and whatsnew. Can remove from whatsnew if we don't want it.

![image](https://user-images.githubusercontent.com/45562402/100548005-ee727280-3237-11eb-8f3c-b0b23f96d532.png)

![image](https://user-images.githubusercontent.com/45562402/100548017-fb8f6180-3237-11eb-96d0-b3df61d0fb1f.png)
 One last tweak in the warning boxes added to io.rst/whatsnew: the end "...to suppress this warning." was out of context and has been changed to "...to avoid raising a FutureWarning." ok will merge on green. we can always change this if major pushback in the rc > And I want to repeat my question from above: do we know about differences in behaviour between both engines (apart from performance)? (to which no one answered, it's probably also not an easy question)
Because if not, I think we should consider automatically using openpyxl when possible, and only raise a warning when it is not installed.

isn't this part of the point? > There are also still plenty of warnings raised by the tests. We could maybe ignore then with pytest on a file or class level (with pytest.mark.filterwarnings)

yeah should address this here @rhshadrach ping when this is updated.