DEPR: Adjust read excel behavior for xlrd >= 2.0
State: closed

- [x] closes #38424
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Alternative to #38522. I've been testing this locally using both xlrd 1.2.0 and 2.0.1.

One test fails because we used to default to xlrd but now default to openpyxl, it's not clear to me if this test should be passing with openpyxl.

cc @cjw296, @jreback, @jorisvandenbossche 



Labels:
name: IO Excel
description: read_excel, to_excel

Comments:
Windows failures are related, will be investigating.
I believe this would also replace #38456.

I find the desire to support xlrd 1.2 at a time when pandas users are already upgrading a package to be both surprising and disappointing, given the potential security issues and poor parsing experience associated with sticking with xlrd 1.2.

This PR has code that is more complex than #38522, even ignoring that which is in place to support the convoluted deprecation process it advocates for. I haven't seen code coverage metrics as part of the pandas PR process, but are you sure all the new conditional branches you've introduced are covered by sufficient tests?

In case it wasn't clear in  #38522, I believe the most robust approach in this area is to:
- obtain the stream we're going to end up with anyway
- peek the minimum number of bytes from it to do content-based inference
- `.seek(0)` to get the stream back to its initial state.
Thanks for the comments @cjw296, I was able to simplify/improve a lot from them.
@rhshadrach - I'm confused as to why you didn't just start with  #38522 and add the fallback logic you're advocating for. (corrected)
@cjw296 

> I'm confused as to why you didn't just start with #38424 and add the fallback logic you're advocating for.

I think you mean your PRs #38456/#38522. I had started this work 7 days ago, prior to the existence of them.

2 linux tests failed to start, all other checks passed. Rerunning.
/azp run
<samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
Update: Failure if from one of the builds using zh_CN.utf8

Failure is related, though I don't understand it:

```
>               handle = open(handle, ioargs.mode)
E               FileNotFoundError: [Errno 2] 没有那个文件或目录: 'foo.xlsm'

[snip]

with pytest.raises(FileNotFoundError, match="No such file or directory"):
>           pd.read_excel(bad_file)
E           AssertionError: Regex pattern 'No such file or directory' does not match "[Errno 2] 没有那个文件或目录: 'foo.xlsm'".
```
Regardless of which PR was started first, it's *also* an option to update https://github.com/pandas-dev/pandas/pull/38522 to use the behaviour we want (fallback to xlrd if openpyxl is not installed, for xlrd < 2.0, see also https://github.com/pandas-dev/pandas/pull/38522#pullrequestreview-554024616, I think it should be fairly straightforward to add that to that PR)
> Regardless of which PR was started first, it's _also_ an option to update #38522 to use the behaviour we want (fallback to xlrd if openpyxl is not installed, for xlrd < 2.0, see also [#38522 (review)](https://github.com/pandas-dev/pandas/pull/38522#pullrequestreview-554024616), I think it should be fairly straightforward to add that to that PR)

ok sure. let's try to get this in today and cut the release. i view better inference as good but shouldn't hold this up.
@jreback @jorisvandenbossche Is there room for improving the inference done here? As far as I can tell, the inference done here is that of #38522 but also handles bytes and urls (which I believe is causing failures in #38522).
Two of the travis builds failed to start, the third one passed. Restarted manually.
great let's merge on greenish
> @jreback @jorisvandenbossche Is there room for improving the inference done here? As far as I can tell, the inference done here is that of #38522 but also handles bytes and urls (which I believe is causing failures in #38522).

Yeah, that's certainly fine for me as well. I am just not very familiar with the details, so can't really judge whether your version also covers the different edge cases that @cjw296 took care of in https://github.com/pandas-dev/pandas/pull/38522 (cfr https://github.com/pandas-dev/pandas/pull/38571#discussion_r546235310)

(and thanks both @rhshadrach and @cjw296 for working on this!)
> > @jreback @jorisvandenbossche Is there room for improving the inference done here? As far as I can tell, the inference done here is that of #38522 but also handles bytes and urls (which I believe is causing failures in #38522).
> 
> Yeah, that's certainly fine for me as well. I am just not very familiar with the details, so can't really judge whether your version also covers the different edge cases that @cjw296 took care of in #38522 (cfr [#38571 (comment)](https://github.com/pandas-dev/pandas/pull/38571#discussion_r546235310))

sure, let's just merge this and can always do another update
@rhshadrach can you unpin xlrd in some of the CI builds, so we ensure to actually test the case with xlrd == 2.0
> sure, let's just merge this and can always do another update

I'd prefer it if you added the warnings and fallback in #38522 and use that. That code certainly works with bytes and urls, I remember fixing the tests. I stopped short of cleaning up the remaining test failures because it was unclear if the branch stood any chance of being merged, and I believe it's code is cleaner and simpler than the code in this PR.

(#38522 is before any CI pinning was done...)
@jorisvandenbossche I have the changes made, but unsure of which CI builds to unpin; any guidance here? Or should I just pick a few?
> @jorisvandenbossche I have the changes made, but unsure of which CI builds to unpin; any guidance here? Or should I just pick a few?

```
jreback@lhs2:~/pandas-dev/ci$ grep xlrd deps/*
deps/azure-37-locale_slow.yaml:  - xlrd=1.2.0
deps/azure-37-minimum_versions.yaml:  - xlrd=1.2.0
deps/azure-37-slow.yaml:  - xlrd<2.0
deps/azure-38-locale.yaml:  - xlrd<2.0
deps/azure-38-slow.yaml:  - xlrd<2.0
deps/azure-macos-37.yaml:  - xlrd<2.0
deps/azure-windows-37.yaml:  - xlrd<2.0
deps/azure-windows-38.yaml:  - xlrd<2.0
deps/travis-37-cov.yaml:  - xlrd<2.0
deps/travis-37-locale.yaml:  - xlrd<2.0
```

I would say change one of the window and 38-slow to >2 leave the rest
@cjw296 

> That code certainly works with bytes and urls,

Indeed, agreed. I had missed your usage of get_handle and concluded too quickly when I saw url tests failing. My apologies.
@rhshadrach - would you be interested in adding the warnings in the way I described in https://github.com/pandas-dev/pandas/pull/38522#issuecomment-749097931 and fixing up the remaining test failures there? The branch is open to commits from pandas people.

(the diff looks messy, and I think the branch is much clearer if you look at the `_base.py` at the head of the branch instead of the diff)
@cjw296 No, I am not.
> No, I am not.

*sigh*
@rhshadrach 

https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=50726&view=logs&j=404760ec-14d3-5d48-e580-13034792878f&t=f81e4cc8-d61a-5fb8-36be-36768e5c561a looks like a legit failure
@cjw296 adding the additional logic (fallback, warning, etc) to #38522 would also make it less clean. And in the meantime, effort in refining that logic has been done in this PR, so it would be harder to move that. Do you think there are still cases you covered in #38522 that are not covered here? As far as I can see, this PR uses much of the same inspection code, and also added the similar tests as you added in #38522.

> @rhshadrach
> 
> https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=50726&view=logs&j=404760ec-14d3-5d48-e580-13034792878f&t=f81e4cc8-d61a-5fb8-36be-36768e5c561a looks like a legit failure

@jreback - Agreed, it is, but I don't understand it. The warning is emitted when openpyxl is not installed but xlrd < 2.0 is and a non-xls file is used. Replicating this in my environment, the test passes with the warning emitted with the right stacklevel (checked manually). So maybe this is a Windows vs Linux issue?

What really doesn't make sense is that `actual_warning.filename` is `pandas\io\excel\_base.py` where the Warning is emitted, but the stacklevel is very clearly being set to either be 2 or 4. I don't see how it's possible that `_base.py` is then the filename.

Attempting to debug via CI now.
If it only fails on windows, I think it is also fine to add a `check_stacklevel=False` to the particular test ..
> If it only fails on windows, I think it is also fine to add a `check_stacklevel=False` to the particular test ..

Will do, is this a known issue with Windows? Couldn't find an issue on github, will add one if it is.
Actually, it's a test asserting warnings about positional arguments, not about the engine. So that might interfere with the test, because it now also raises another warning than the one it is testing?