Repository: pandas-dev/pandas
Pull request: 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

Problem statement:
Deprecate using `xlrd` engine in favor of openpyxl
xlrd is unmaintained and the previous maintainer has asked us to move towards openpyxl. xlrd works now, but *might* have some issues when Python 3.9 or later gets released and changes some elements of the XML parser, as default usage right now throws a `PendingDeprecationWarning`

Considering that I think we need to deprecate using `xlrd` in favor of `openpyxl`. We might not necessarily need to remove the former and it does offer some functionality the latter doesn't (namely reading `.xls` files) but should at the very least start moving towards the latter

Hints from issue discussion:
@WillAyd Should we start with adding `openpyxl` as an engine in [pandas.read_excel](https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.read_excel.html#pandas-read-excel).

Happy to contribute a PR for it.
It is already available just need to make it default over time, so want to raise a FutureWarning when the user doesn’t explicitly provide an engine that it will change to openpxyl in a future releaae
Would it be an option to simply switch the default, without first raising a warning? Or do the two engines give different results in quite some cases? 

Reason I am asking is because if for 99% of the use cases both give exactly the same, raising a warning for all those users feels a bit annoying.
The docstrings already require some updating as they currently indicate `'xlrd'` is the only option for 'engine'.
> Would it be an option to simply switch the default, without first raising a warning? Or do the two engines give different results in quite some cases?

It would require installing a different optional package, so a version with deprecation messages/future warnings would be useful.
> Would it be an option to simply switch the default, without first raising a warning? Or do the two engines give different results in quite some cases?
> 
> Reason I am asking is because if for 99% of the use cases both give exactly the same, raising a warning for all those users feels a bit annoying.

Just to add - xlrd is AFAIK the only library that can read the legacy .xls format. From experience even ".xlsx" formats aren't as standardized as you'd hope. The openpyxl reader is pretty new so I guess will see through proper deprecation cycle what differences, if any, arise
> Just to add - xlrd is AFAIK the only library that can read the legacy .xls format.

And to be clear, for those the default would stay xlrd, and this would not be deprecated, right?

So the question about "switching the default" was only for xlsx files.
Up for debate but for that I think we want to push people towards reading .xlsx files. xlrd is not maintained any more and might break with Python 3.9, so would want to get ahead of that as much as possible
So what is the decision ? 

Dump xlrd disregarding .xls support and to replace it with openpyxl ?
We need to deprecate using xlrd by default. I think it's fine to do for all extensions, including .xls - interesting in a PR?

Commit message: Revert change to stacklevel

Patch to review:
diff --git a/doc/source/whatsnew/v1.2.0.rst b/doc/source/whatsnew/v1.2.0.rst
index 27511c96faa5a..1a48a4de223de 100644
--- a/doc/source/whatsnew/v1.2.0.rst
+++ b/doc/source/whatsnew/v1.2.0.rst
@@ -484,6 +484,7 @@ Deprecations
 - Deprecated :meth:`Index.asi8` for :class:`Index` subclasses other than :class:`DatetimeIndex`, :class:`TimedeltaIndex`, and :class:`PeriodIndex` (:issue:`37877`)
 - The ``inplace`` parameter of :meth:`Categorical.remove_unused_categories` is deprecated and will be removed in a future version (:issue:`37643`)
 - The ``null_counts`` parameter of :meth:`DataFrame.info` is deprecated and replaced by ``show_counts``. It will be removed in a future version (:issue:`37999`)
+- Deprecated the default argument ``engine=None`` of the function :func:`read_excel`, which uses the no longer maintained xlrd engine. Not specifying the engine will raise a ``FutureWarning``. This argument will default to ``"openpyxl"`` in a future version, which is now the recommended engine for xlsx and xlsm files (:issue:`28547`)
 
 .. ---------------------------------------------------------------------------
 
diff --git a/pandas/io/excel/_base.py b/pandas/io/excel/_base.py
index c519baa4c21da..0cdc769047be0 100644
--- a/pandas/io/excel/_base.py
+++ b/pandas/io/excel/_base.py
@@ -4,6 +4,7 @@
 import os
 from textwrap import fill
 from typing import Any, Dict, Mapping, Union, cast
+import warnings
 
 from pandas._config import config
 
@@ -101,10 +102,15 @@
     If io is not a buffer or path, this must be set to identify io.
     Supported engines: "xlrd", "openpyxl", "odf", "pyxlsb", default "xlrd".
     Engine compatibility :
-    - "xlrd" supports most old/new Excel file formats.
+    - "xlrd" supports most old/new Excel file formats but is no longer maintained.
     - "openpyxl" supports newer Excel file formats.
     - "odf" supports OpenDocument file formats (.odf, .ods, .odt).
     - "pyxlsb" supports Binary Excel files.
+
+    .. deprecated:: 1.2.0
+        The default value ``None`` is deprecated and will be changed to ``"openpyxl"``
+        in a future version. Not specifying an engine will raise a FutureWarning.
+
 converters : dict, default None
     Dict of functions for converting values in certain columns. Keys can
     either be integers or column labels, values are functions that take one
@@ -878,12 +884,17 @@ class ExcelFile:
     engine : str, default None
         If io is not a buffer or path, this must be set to identify io.
         Supported engines: ``xlrd``, ``openpyxl``, ``odf``, ``pyxlsb``,
-        default ``xlrd``.
+        default ``xlrd`` for .xls* files, ``odf`` for .ods files.
         Engine compatibility :
-        - ``xlrd`` supports most old/new Excel file formats.
+        - ``xlrd`` supports most old/new Excel file formats but is no longer maintained.
         - ``openpyxl`` supports newer Excel file formats.
         - ``odf`` supports OpenDocument file formats (.odf, .ods, .odt).
         - ``pyxlsb`` supports Binary Excel files.
+
+        .. deprecated:: 1.2.0
+            The default value ``None`` is deprecated and will be changed to
+            ``"openpyxl"`` in a future version. Not specifying an engine will
+            raise a FutureWarning.
     """
 
     from pandas.io.excel._odfreader import ODFReader
@@ -902,6 +913,12 @@ def __init__(
         self, path_or_buffer, engine=None, storage_options: StorageOptions = None
     ):
         if engine is None:
+            warnings.warn(
+                "The default argument engine=None is deprecated. "
+                "Specify the engine argument to suppress this warning.",
+                FutureWarning,
+                stacklevel=2,
+            )
             engine = "xlrd"
             if isinstance(path_or_buffer, (BufferedIOBase, RawIOBase)):
                 if _is_ods_stream(path_or_buffer):
diff --git a/pandas/tests/io/excel/test_readers.py b/pandas/tests/io/excel/test_readers.py
index c582a0fa23577..af911c1deffb6 100644
--- a/pandas/tests/io/excel/test_readers.py
+++ b/pandas/tests/io/excel/test_readers.py
@@ -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)
diff --git a/pandas/tests/io/excel/test_xlrd.py b/pandas/tests/io/excel/test_xlrd.py
index 26190edaa4960..108c55cf1948c 100644
--- a/pandas/tests/io/excel/test_xlrd.py
+++ b/pandas/tests/io/excel/test_xlrd.py
@@ -38,6 +38,28 @@ def test_read_xlrd_book(read_ext, frame):
 # TODO: test for openpyxl as well
 def test_excel_table_sheet_by_index(datapath, read_ext):
     path = datapath("io", "data", "excel", f"test1{read_ext}")
-    with ExcelFile(path) as excel:
+    with ExcelFile(path, engine="xlrd") as excel:
         with pytest.raises(xlrd.XLRDError):
             pd.read_excel(excel, sheet_name="asdf")
+
+
+def test_excel_file_warning_with_xlsx_file(datapath):
+    # GH 29375
+    path = datapath("io", "data", "excel", "test1.xlsx")
+    # 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
+    ):
+        ExcelFile(path, engine=None)
+
+
+def test_read_excel_warning_with_xlsx_file(tmpdir, datapath):
+    # GH 29375
+    path = datapath("io", "data", "excel", "test1.xlsx")
+    # 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
+    ):
+        pd.read_excel(path, "Sheet1", engine=None)


Code review comments:
diff_hunk: @@ -902,6 +913,12 @@ def __init__(
         self, path_or_buffer, engine=None, storage_options: StorageOptions = None
     ):
         if engine is None:
+            warnings.warn(
path: pandas/io/excel/_base.py
text: @user1:
Shouldn't we be more specific about *when* to warn?

* If people only have openpyxl installed, shouldn't we default to use that instead of having the warning?
* From earlier discussion, I understood that we are (for now) fine to keep xlrd support for old excel files for which it is the only. So for those cases (eg `.xls`), we shouldn't raise a warning?

@user2:
and python < 3.9

@user3:
@user1 I've implemented the xls check, however I don't understand your first bullet. Isn't having the deprecation warning so that we don't change behavior without notice? Why is it okay to change behavior if they have openpyxl installed?

@user1:
> Why is it okay to change behavior if they have openpyxl installed?

My suggestions was for when they have *only* openpyxl installed. It will currently not be a very common situation, since we don't try to use openpyxl if xlrd is not present, unless the user actually specifies `engine="openpyxl"`. But, given the deprecation warnings, this could be another solution for users: ensure you update your environment to only have openpyxl and not xlrd, and then you don't need to update all occurrences of `read_excel` in your code.

@user3:
Ahh, thanks! I completely misread.

diff_hunk: @@ -878,12 +884,17 @@ class ExcelFile:
     engine : str, default None
         If io is not a buffer or path, this must be set to identify io.
         Supported engines: ``xlrd``, ``openpyxl``, ``odf``, ``pyxlsb``,
-        default ``xlrd``.
+        default ``xlrd`` for .xls* files, ``odf`` for .ods files.
         Engine compatibility :
-        - ``xlrd`` supports most old/new Excel file formats.
+        - ``xlrd`` supports most old/new Excel file formats but is no longer maintained.
path: pandas/io/excel/_base.py
text: @user1:
i think can you add a note that xlrd not support for python >= 3.9 (everywhere you mention)

@user2:
Sorry - I don't follow how this is different from python < 3.9, what do you mean by support? I am able to install xlrd on a python 3.9 environment:

![image](https://user-images.githubusercontent.com/45562402/100463250-86862580-3099-11eb-85ca-af4ef01000e4.png)


@user1:
see the issues when you actually use xlrd and pandas on 3.9. this is the entire reason for the deprecation; it is completely broken on 3.9

@user2:
I'm not seeing that:

```
Python 3.9.0 (default, Nov 15 2020, 14:28:56) 
[GCC 7.3.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pandas as pd
>>> pd.__version__
'1.2.0.dev0+1364.g65319af6e'
>>> pd.DataFrame({'a': [1, 2, 3]}).to_excel('test.xlsx')
>>> pd.read_excel('test.xlsx', engine='xlrd')
   Unnamed: 0  a
0           0  1
1           1  2
2           2  3
```

From the issue, I see this in the OP

> xlrd is unmaintained and the previous maintainer has asked us to move towards openpyxl. xlrd works now, but might have some issues when Python 3.9 or later gets released and changes some elements of the XML parser, as default usage right now throws a PendingDeprecationWarning

but no mention of xlrd being broken on Python 3.9.

@user1:
#37795

diff_hunk: @@ -902,6 +913,12 @@ def __init__(
         self, path_or_buffer, engine=None, storage_options: StorageOptions = None
     ):
         if engine is None:
+            warnings.warn(
+                "The default argument engine=None is deprecated. "
+                "Specify the engine argument to suppress this warning.",
path: pandas/io/excel/_base.py
text: @user1:
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

@user2:
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.

@user1:
For the current behaviour, that sounds good!

diff_hunk: @@ -902,6 +913,12 @@ def __init__(
         self, path_or_buffer, engine=None, storage_options: StorageOptions = None
     ):
         if engine is None:
+            warnings.warn(
+                "The default argument engine=None is deprecated. "
+                "Specify the engine argument to suppress this warning.",
+                FutureWarning,
+                stacklevel=2,
path: pandas/io/excel/_base.py
text: @user1:
> 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`

@user2:
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
```

@user3:
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.

@user1:
@user2 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)