Skip to content

DEPR: Deprecate using xlrd engine for read_excel #35029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Dec 1, 2020
Merged

DEPR: Deprecate using xlrd engine for read_excel #35029

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3a76a36
Deprecate using `xlrd` engine and change default engine to read excel…
cruzzoe Nov 2, 2019
101aa97
Revert all changes related to switching to openpyxl as the default
roberthdevries Aug 23, 2020
081ecf8
Reword whatsnew message for the benefit of end users.
roberthdevries Aug 26, 2020
ada4354
Merge branch 'master' of https://github.com/pandas-dev/pandas into 35029
rhshadrach Nov 25, 2020
3233381
Fixed FutureWarning emitting logic, reverted openpyxl workaround
rhshadrach Nov 25, 2020
0f4c8a1
Revert change to stacklevel
rhshadrach Nov 26, 2020
499f9a0
-
rhshadrach Nov 27, 2020
825c61c
Merge branch 'master' of https://github.com/pandas-dev/pandas into 35029
rhshadrach Nov 28, 2020
88093f6
Changes from review
rhshadrach Nov 28, 2020
44f157b
DeprecationWarning -> FutureWarning; added warning to io.rst/whatsnew
rhshadrach Nov 29, 2020
fffbacb
"to suppress this warning." -> "to avoid raising a FutureWarning."
rhshadrach Nov 29, 2020
bb53725
Changed engine=None to mostly using openpyxl
rhshadrach Nov 30, 2020
d8dcb04
Minor doc touchups
rhshadrach Nov 30, 2020
f9876dd
Re-added tests, minor doc touchups
rhshadrach Nov 30, 2020
bc3ec47
Test for no warning as well
rhshadrach Nov 30, 2020
fe10a89
Doc tweaks
rhshadrach Dec 1, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert change to stacklevel
  • Loading branch information
@rhshadrach
rhshadrach committed Nov 26, 2020
commit 0f4c8a1987fb532aa8afe8531aca2eeeae7b7b7b
2 changes: 1 addition & 1 deletion pandas/io/excel/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,7 @@ def __init__(
"The default argument engine=None is deprecated. "
"Specify the engine argument to suppress this warning.",
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to clarify this message, to explain the reason why it is deprecated, and what people can do: either specify "xlrd", or either install openpyxl and specify that
Copy link
Member

@rhshadrach rhshadrach Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to the following. Let me know of any thoughts.

DeprecationWarning: The default argument engine=None is deprecated. Using None defaults to the xlrd engine which is no longer maintained. The default value will be 'openpyxl' in a future version of pandas, although xlrd will continue to be allowed for the indefinite future. Either install openpyxl and specify it as the engine, or specify 'xlrd' to suppress this warning.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the current behaviour, that sounds good!
FutureWarning,
stacklevel=4,
stacklevel=2,
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Given that read_excel is used much more, it might make sense to ensure it is correct for read_excel
Copy link
Member

@rhshadrach rhshadrach Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the expense of having usage of ExcelFile directly have a stack trace that starts two levels too high (which includes the possibility of no stack trace)? IMO going too deep here is preferable, at least then the offending line is always visible.

Another solution would be using inspect:

import inspect caller = inspect.stack()[1] if ( caller.filename.endswith("pandas/io/excel/_base.py") and caller.function == "read_excel" ): stacklevel = 4 else: stacklevel = 2
Copy link
Contributor

@jreback jreback Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to do this is is fine, or ust turn off the check for stacklevel where needed. i don't think its worth jumping thru too many hoops.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhshadrach as far as I know, warnings don't show a full stack trace, but only point to a single location. So whether this single location is wrong "higher" in the stack vs "lower" in the stack, I personally don't find the lower (deeper) one any more informative or less misleading ..
The only advantage that the the deeper one has is that it will always show that the warning is coming from internal pandas code, so can give the hint this has to do with pandas functionality. But I think that goal can also be achieved by having a clear warning message (for example, include "pandas.read_excel" in the message)
)
engine = "xlrd"
if isinstance(path_or_buffer, (BufferedIOBase, RawIOBase)):
Expand Down
4 changes: 3 additions & 1 deletion pandas/tests/io/excel/test_readers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,9 @@ def test_read_excel_squeeze(self, read_ext):
tm.assert_series_equal(actual, expected)

def test_deprecated_kwargs(self, read_ext):
with tm.assert_produces_warning(FutureWarning, raise_on_extra_warnings=False):
with tm.assert_produces_warning(
FutureWarning, check_stacklevel=False, raise_on_extra_warnings=False
):
pd.read_excel("test1" + read_ext, "Sheet1", 0)

pd.read_excel("test1" + read_ext)
Expand Down
4 changes: 2 additions & 2 deletions pandas/tests/io/excel/test_xlrd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def test_excel_file_warning_with_xlsx_file(datapath):
# DeprecationWarning: "This method will be removed in future versions.
# Use 'tree.iter()' or 'list(tree.iter())' instead."
with tm.assert_produces_warning(
FutureWarning, check_stacklevel=False, raise_on_extra_warnings=False
FutureWarning, check_stacklevel=True, raise_on_extra_warnings=False
):
ExcelFile(path, engine=None)

Expand All @@ -60,6 +60,6 @@ def test_read_excel_warning_with_xlsx_file(tmpdir, datapath):
# DeprecationWarning: "This method will be removed in future versions.
# Use 'tree.iter()' or 'list(tree.iter())' instead."
with tm.assert_produces_warning(
FutureWarning, check_stacklevel=True, raise_on_extra_warnings=False
FutureWarning, check_stacklevel=False, raise_on_extra_warnings=False
):
pd.read_excel(path, "Sheet1", engine=None)