Repository: pandas-dev/pandas
Pull request: REG: read_excel with engine specified raises on non-path/non-buffer
- [x] closes #39528
- [x] tests added / passed
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [x] whatsnew entry


Problem statement:
BUG: read_excel with Workbook and engine="openpyxl" raises ValueError
- [x] I have checked that this issue has not already been reported.

- [x] I have confirmed this bug exists on the latest version of pandas.

- [ ] (optional) I have confirmed this bug exists on the master branch of pandas.

---

#### Code Sample

```python
from openpyxl import load_workbook
from pandas import read_excel

wb = load_workbook("testfile.xlsx")
read_excel(wb, engine="openpyxl")
```

#### Problem description

In pandas 1.1.5, the above code completes with no problems.

In pandas 1.2.1, it causes the following exception:

```
Traceback (most recent call last):
  File "c:/Users/akaijanaho/scratch/pandas-openpyxl-bug/bug.py", line 5, in <module>
    read_excel(wb, engine="openpyxl")
  File "C:\Users\akaijanaho\scratch\pandas-openpyxl-bug\venv\lib\site-packages\pandas\util\_decorators.py", line 299, in wrapper
    return func(*args, **kwargs)
  File "C:\Users\akaijanaho\scratch\pandas-openpyxl-bug\venv\lib\site-packages\pandas\io\excel\_base.py", line 336, in read_excel
    io = ExcelFile(io, storage_options=storage_options, engine=engine)
  File "C:\Users\akaijanaho\scratch\pandas-openpyxl-bug\venv\lib\site-packages\pandas\io\excel\_base.py", line 1057, in __init__
    ext = inspect_excel_format(
  File "C:\Users\akaijanaho\scratch\pandas-openpyxl-bug\venv\lib\site-packages\pandas\io\excel\_base.py", line 938, in inspect_excel_format
    with get_handle(
  File "C:\Users\akaijanaho\scratch\pandas-openpyxl-bug\venv\lib\site-packages\pandas\io\common.py", line 558, in get_handle
    ioargs = _get_filepath_or_buffer(
  File "C:\Users\akaijanaho\scratch\pandas-openpyxl-bug\venv\lib\site-packages\pandas\io\common.py", line 371, in _get_filepath_or_buffer
    raise ValueError(msg)
ValueError: Invalid file path or buffer object type: <class 'openpyxl.workbook.workbook.Workbook'>
```

The documentation does not specify Workbook as an acceptable value type for io, but supporting it seems reasonable and accords with the 1.1.5 behavior.

In my use case, we mainly parse an Excel file with openpyxl but use pandas with a specific sub-problem. We would like to reuse the same Workbook instead of having pandas re-read the file.
</details>

Hints from issue discussion:
I will have time later today to look into this. Can you open/wrap the workbook with `ExcelFile` and then pass it to `read_excel`? I'm not too familiar with the ExcelFile syntax, something like the code below might avoid opening the file twice as a workaround for 1.2.1.
```py
import pandas as pd

with pd.ExcelFile('test.xlsx', engine="openpyxl") as excel:
    dataframe = pd.read_excel(excel)
```
Can reproduce on master. 
```
>>> from openpyxl import load_workbook
>>> from pandas import read_excel
>>> wb = load_workbook("pandas/tests/io/data/excel/blank.xlsx") 
>>> read_excel(wb, engine="openpyxl")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\liwende\pandas\pandas\util\_decorators.py", line 299, in wrapper
    return func(*args, **kwargs)
  File "C:\Users\liwende\pandas\pandas\io\excel\_base.py", line 340, in read_excel
    io = ExcelFile(io, storage_options=storage_options, engine=engine)
  File "C:\Users\liwende\pandas\pandas\io\excel\_base.py", line 1063, in __init__
    content_or_path=path_or_buffer, storage_options=storage_options
  File "C:\Users\liwende\pandas\pandas\io\excel\_base.py", line 945, in inspect_excel_format
    content_or_path, "rb", storage_options=storage_options, is_text=False
  File "C:\Users\liwende\pandas\pandas\io\common.py", line 576, in get_handle
    storage_options=storage_options,
  File "C:\Users\liwende\pandas\pandas\io\common.py", line 378, in _get_filepath_or_buffer
    raise ValueError(msg)
ValueError: Invalid file path or buffer object type: <class 'openpyxl.workbook.workbook.Workbook'>
```
@twoertwein 
I suspect that https://github.com/pandas-dev/pandas/blob/master/pandas/io/excel/_base.py#L1072-L1077 (master) and https://github.com/pandas-dev/pandas/blob/1.2.x/pandas/io/excel/_base.py#L1065-L1070 (for pandas 1.2) should probably be only executed with no engine specified. 
However, the bigger issue is that inspect_excel_format should be able to pick up that this is an openpyxl workbook, which it doesn't since it errors in get_handle(which makes sense as an openpyxl workbook is not a buffer I think).
Yes, it could probably be fixed by something like:

```
diff --git a/pandas/io/excel/_base.py b/pandas/io/excel/_base.py
index 213be7c..fb6ca55 100644
--- a/pandas/io/excel/_base.py
+++ b/pandas/io/excel/_base.py
@@ -1071,7 +1071,7 @@ class ExcelFile:

         if xlrd_version is not None and isinstance(path_or_buffer, xlrd.Book):
             ext = "xls"
-        else:
+        elif engine in (None, "xlrd", "auto"):
             ext = inspect_excel_format(
                 content_or_path=path_or_buffer, storage_options=storage_options
             )
```

Thanks for the report @ajkaijanaho! @twoertwein - seems to me like the patch should be improving inspect_excel_format itself instead of avoiding calling it.
> I will have time later today to look into this. Can you open/wrap the workbook with `ExcelFile` and then pass it to `read_excel`? I'm not too familiar with the ExcelFile syntax, something like the code below might avoid opening the file twice as a workaround for 1.2.1.
> 
> ```python
> import pandas as pd
> 
> with pd.ExcelFile('test.xlsx', engine="openpyxl") as excel:
>     dataframe = pd.read_excel(excel)
> ```

Not really useful for us, because in our code the Workbook object is created somewhere else and passed around the call chain quite a bit before we get to Pandas. Rewriting all those calls to pass around ExcelFile is not really worth the effort. Our workaround is to stay with 1.1.5 for now, and it works well enough.
@twoertwein - I was wrong above, your suggested patch is much better. We should really avoid calling `inspect_excel_format` unless necessary in order to not have to check (and maintain) all sorts of dependency instances.
> @twoertwein - I was wrong above, your suggested patch is much better. We should really avoid calling `inspect_excel_format` unless necessary in order to not have to check (and maintain) all sorts of dependency instances.

Depending on which behavior is expected, this simple elif-patch is probably not enough. If a user provides a workbook compatible with one of the engines but does not specify an engine explicitly, do we need to auto-detect the engine from the workbook type? If we need that, it should probably go into `inspect_excel_format`.

@twoertwein: Comparing to 1.1.x:

```
        if engine is None:
            engine = "xlrd"
            if isinstance(path_or_buffer, (BufferedIOBase, RawIOBase)):
                if _is_ods_stream(path_or_buffer):
                    engine = "odf"
            else:
                ext = os.path.splitext(str(path_or_buffer))[-1]
                if ext == ".ods":
                    engine = "odf"
        if engine not in self._engines:
            raise ValueError(f"Unknown engine: {engine}")
```

It appears to me that passing a Workbook with `engine=None` would raise (need to test this though). Assuming that, the patch should be a minimal change: only restore support for a openpyxl Workbook when passing engine='openpyxl', and in more generality, avoid the unnecessary type-detection when engine is specified. 

Supporting engine=None with Workbooks would be an additional feature, which might be welcome, and should go into 1.3 (or later). My only hesitation here is that it seems the implementation would have to special-case all engines and their individual workbook (and worksheet?) types which I think is a bit undesirable. If there is an implementation that is free of this, then I'd certainly be +1.
> Depending on which behavior is expected, this simple elif-patch is probably not enough. If a user provides a workbook compatible with one of the engines but does not specify an engine explicitly, do we need to auto-detect the engine from the workbook type? If we need that, it should probably go into `inspect_excel_format`.

The [read_excel documentation](https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.read_excel.html) is quite explicit about this point: "engine str, default None If io is not a buffer or path, this must be set to identify io"

Commit message: GH # in test

Patch to review:
diff --git a/doc/source/whatsnew/v1.2.2.rst b/doc/source/whatsnew/v1.2.2.rst
index 0ee1abaa2a0eb..dc27ceba138c9 100644
--- a/doc/source/whatsnew/v1.2.2.rst
+++ b/doc/source/whatsnew/v1.2.2.rst
@@ -21,7 +21,7 @@ Fixed regressions
 - Fixed regression in :meth:`~DataFrame.to_pickle` failing to create bz2/xz compressed pickle files with ``protocol=5`` (:issue:`39002`)
 - Fixed regression in :func:`pandas.testing.assert_series_equal` and :func:`pandas.testing.assert_frame_equal` always raising ``AssertionError`` when comparing extension dtypes (:issue:`39410`)
 - Fixed regression in :meth:`~DataFrame.to_csv` opening ``codecs.StreamWriter`` in binary mode instead of in text mode and ignoring user-provided ``mode`` (:issue:`39247`)
--
+- Fixed regression in :func:`read_excel` that incorrectly raised when the argument ``io`` was a non-path and non-buffer and the ``engine`` argument was specified (:issue:`39528`)
 
 .. ---------------------------------------------------------------------------
 
diff --git a/pandas/io/excel/_base.py b/pandas/io/excel/_base.py
index 213be7c05b370..84b5cae09acce 100644
--- a/pandas/io/excel/_base.py
+++ b/pandas/io/excel/_base.py
@@ -1069,26 +1069,37 @@ def __init__(
 
             xlrd_version = LooseVersion(get_version(xlrd))
 
-        if xlrd_version is not None and isinstance(path_or_buffer, xlrd.Book):
-            ext = "xls"
-        else:
-            ext = inspect_excel_format(
-                content_or_path=path_or_buffer, storage_options=storage_options
-            )
-
+        ext = None
         if engine is None:
+            # Only determine ext if it is needed
+            if xlrd_version is not None and isinstance(path_or_buffer, xlrd.Book):
+                ext = "xls"
+            else:
+                ext = inspect_excel_format(
+                    content_or_path=path_or_buffer, storage_options=storage_options
+                )
+
             # ext will always be valid, otherwise inspect_excel_format would raise
             engine = config.get_option(f"io.excel.{ext}.reader", silent=True)
             if engine == "auto":
                 engine = get_default_engine(ext, mode="reader")
 
-        if engine == "xlrd" and ext != "xls" and xlrd_version is not None:
-            if xlrd_version >= "2":
+        if engine == "xlrd" and xlrd_version is not None:
+            if ext is None:
+                # Need ext to determine ext in order to raise/warn
+                if isinstance(path_or_buffer, xlrd.Book):
+                    ext = "xls"
+                else:
+                    ext = inspect_excel_format(
+                        path_or_buffer, storage_options=storage_options
+                    )
+
+            if ext != "xls" and xlrd_version >= "2":
                 raise ValueError(
                     f"Your version of xlrd is {xlrd_version}. In xlrd >= 2.0, "
                     f"only the xls format is supported. Install openpyxl instead."
                 )
-            else:
+            elif ext != "xls":
                 caller = inspect.stack()[1]
                 if (
                     caller.filename.endswith(
diff --git a/pandas/tests/io/excel/test_openpyxl.py b/pandas/tests/io/excel/test_openpyxl.py
index 3155e22d3ff5d..bc2aca862546b 100644
--- a/pandas/tests/io/excel/test_openpyxl.py
+++ b/pandas/tests/io/excel/test_openpyxl.py
@@ -116,3 +116,12 @@ def test_to_excel_with_openpyxl_engine(ext):
         ).highlight_max()
 
         styled.to_excel(filename, engine="openpyxl")
+
+
+def test_read_workbook(datapath, ext):
+    # GH 39528
+    filename = datapath("io", "data", "excel", "test1" + ext)
+    wb = openpyxl.load_workbook(filename)
+    result = pd.read_excel(wb, engine="openpyxl")
+    expected = pd.read_excel(filename)
+    tm.assert_frame_equal(result, expected)
diff --git a/pandas/tests/io/excel/test_readers.py b/pandas/tests/io/excel/test_readers.py
index b2e87de5580e6..a594718bd62d9 100644
--- a/pandas/tests/io/excel/test_readers.py
+++ b/pandas/tests/io/excel/test_readers.py
@@ -2,6 +2,7 @@
 from functools import partial
 import os
 from urllib.error import URLError
+from zipfile import BadZipFile
 
 import numpy as np
 import pytest
@@ -685,7 +686,13 @@ def test_missing_file_raises(self, read_ext):
 
     def test_corrupt_bytes_raises(self, read_ext, engine):
         bad_stream = b"foo"
-        with pytest.raises(ValueError, match="File is not a recognized excel file"):
+        if engine is None or engine == "xlrd":
+            error = ValueError
+            msg = "File is not a recognized excel file"
+        else:
+            error = BadZipFile
+            msg = "File is not a zip file"
+        with pytest.raises(error, match=msg):
             pd.read_excel(bad_stream)
 
     @tm.network


Code review comments:
diff_hunk: @@ -116,3 +116,12 @@ def test_to_excel_with_openpyxl_engine(ext):
         ).highlight_max()
 
         styled.to_excel(filename, engine="openpyxl")
+
+
+def test_read_workbook(datapath, ext):
+    # GH 39528
+    filename = datapath("io", "data", "excel", "test1" + ext)
+    wb = openpyxl.load_workbook(filename)
+    result = pd.read_excel(wb, engine="openpyxl")
+    expected = pd.read_excel(filename)
path: pandas/tests/io/excel/test_openpyxl.py
text: @user1:
It cannot hurt to call `wb.close()` before `tm.assert_frame_equal`

@author:
Thanks @user1, added.