Better inference of spreadsheet formats.
State: closed

See:

https://github.com/pandas-dev/pandas/issues/38424#issuecomment-744062773
https://github.com/pandas-dev/pandas/pull/38456

Discussion happening on #38424, code review happening here ;-)

Labels:
name: IO Excel
description: read_excel, to_excel

Comments:
Why is `{repr(foo)}` preferred (insisted on by commit hook...) to `{foo!r}`?
> Why is `{repr(foo)}` preferred (insisted on by commit hook...) to `{foo!r}`?

I don't think we have settled on this explicitly, likely we have a mix of these in the codebase
cc @pandas-dev/pandas-core 
Hello @cjw296! 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 2020-12-16 19:16:29 UTC
> I don't think we have settled on this explicitly, likely we have a mix of these in the codebase

I don't feel strongly, so have changed it in this PR, but was curious as to the reasoning :-)
(I'll squash the commits down to semantics before merging - just easier to review if I leave them unsquashed for now)
> (I'll squash the commits down to semantics before merging - just easier to review if I leave them unsquashed for now)

Not need to do so, we squash on merge
appears we are now picking up `xlrd 2.0.1` and causing some builds to fail.
@jreback - that should be fine on this branch, I'll dig into the CI failures once there's consensus on the approach.
> @jreback - that should be fine on this branch, I'll dig into the CI failures once there's consensus on the approach.

yeah i think its just 1 or 2 tests where we maybe are not safe importing
> In such a case, we would need to distinguish between the "inferred" engine and the user specified one. And if the inferred one is openpyxl, but openpyxl is not installed, we can fallback to xlrd with a clear warning.

Thinking further on it: we should maybe do this anyway, even when not doing the fallback+warning. We might want to give a custom error message explaining that xlrd is no longer maintained / does no longer support files other than .xls, and so you now need to install another dependency, i.e. openpyxl. 
Although the error message ("Missing optional dependency: openpyxl") is clear, it's missing the "why" (since people have always been able to read their excel files with xlrd up to now)
@c123w can you see if can get passing (also merge master).
i hate to block on this for 1.2, we can always do this in 1.2.1.

@jorisvandenbossche @simonjayhawkins 
@jreback - when you tagged @c123w, did you mean to tag me?
> Just to be clear: I think we can keep almost all of these changes, even if we decide in #38424 to keep the xlrd-fallback-with-warning, right?
In such a case, we would need to distinguish between the "inferred" engine and the user specified one. And if the inferred one is openpyxl, but openpyxl is not installed, we can fallback to xlrd with a clear warning.

@cjw296 assuming we keep the decision of have the fallback+warning, do you want to update this PR to reflect that behaviour? Or, would you be OK with someone of us pushing some changes to this PR for that?
I won't stop you, I'll just be really disappointed. https://github.com/pandas-dev/pandas/pull/38522/files#diff-63200ddb7f5656b8ee868a28d9cb7720ffe50689b0e3fb0b4e15cc5c0ae80dd7R1065 seems like an obvious place to insert an `else` clause that can look at the infered engine and do a set intersection with the libraries installed. It could also be commented such that it can be cleanly excised at the absolute earliest opportunity.

If you go this route, please ensure you put a LOUD WARNING (if only we had exceptions that could be really loud warnings, eh?) that using xlrd for xlsx files is explicitly unsupported and that under no circumstances should issues, PRs or other [completely](https://github.com/python-excel/xlrd/commit/58ccbb12e2c0f9803905c90b29bd67adbd58a356#commitcomment-45053861) [inappropriate](https://github.com/python-excel/xlrd/commit/b37d159ecffa871cca77743f6313079e320ad5e6#commitcomment-45294950) comments be added to the xlrd repository.

I would also appreciate if this warning is included in the "what's news", particularly the part about being explicitly unsupported and exposing users to potentially dangerous security issues also with the most verbose form of the above warning about raising upstream issues.
> If you go this route, please ensure you put a LOUD WARNING

Again, that is already what we do on master (except for a few corner cases, as discussed in https://github.com/pandas-dev/pandas/issues/38424, and those can be fixed). 
It's also already included on top of the whatsnew page. You actually have a PR to improve the wording in the warning messages, so if you have further suggestions about the precise message, feel free to further update that.

And I am sorry that you get such backlash on it. 
(but, it's also a bit annoying situation right now with the stable pandas release still defaulting to xlrd even for xlsx files (which is not the fault ox xlrd of course, but influences the experience of pandas users). So the 1.2 release will be a big help to improve that in any case regardless of the discussion above).
@jorisvandenbossche - the frustration is that I advertised the deprecation of this package for non-xls files 4 years ago and nothing has been done... Now I try and help here, and the same lukewarm attitude continues.
And I understand that frustration. We also acted on it too slowly in pandas (although we haven't been aware of it for 4 years, I think, the main issue where the discussion happened was only opened last year: https://github.com/pandas-dev/pandas/issues/28547). But you also have to understand that the average pandas user was not aware of this. They simply used `pd.read_excel`, and it worked (I suppose most pandas users wouldn't even know what `xlrd` is, let alone know that it was no longer maintained).

It's always difficult to communicate with users ("nowbody reads the docs"), and so a main way of communicating future changes that we use in pandas is through warnings. 

Okay, but as I see it, to get to pandas 1.2 you will have done one of two things:

- set up a new environment.
- upgraded pandas.

In either case, a clear exception that says "you need to install package x" would be the cleanest and quickest way to get people using the best software.

"nobody looks at warnings that doesn't stop their code working" is sadly as true as "nobody reads the docs", especially given the poor use of warnings in cpython itself over the last few years.

> "nobody looks at warnings that doesn't stop their code working" is sadly as true as "nobody reads the docs", especially given the poor use of warnings in cpython itself over the last few years.

Warnings have done decently well for us empirically speaking.  If they don't want to heed the warnings and upgrade, then that is on them because they will have had sufficient time to consume and prepare.

Just like we unfortunately did not see your warnings (that went above and beyond BTW) about `xlrd`, we now have to deal with the ramifications of being slow to upgrade and switching to `openpyxl`.
closing in favor of #38571