BUG: Patch for skipping seek() when loading Excel files from url
State: closed

- [x] closes #20434
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This is my first PR and I tried to follow the guideline. Sorry, if I overlooked something.
I don't have to add anything in whatsnew, because the bug isn't present in the current stable version 0.22, correct?

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: IO Excel
description: read_excel, to_excel

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=h1) Report
> Merging [#20437](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/73cb32e71469cf1615907544728bb9a751089de4?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20437/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #20437      +/-   ##
==========================================
+ Coverage   91.82%   91.84%   +0.02%     
==========================================
  Files         153      153              
  Lines       49256    49256              
==========================================
+ Hits        45229    45241      +12     
+ Misses       4027     4015      -12
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.23% <ø> (+0.02%)` | :arrow_up: |
| #single | `41.9% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/20437/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `66.81% <0%> (+1.73%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=footer). Last update [73cb32e...88d91f5](https://codecov.io/gh/pandas-dev/pandas/pull/20437?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

cc @WillAyd 
Pretty simple change so lgtm - fine with it if you are Tom. Thanks for the contribution @mcrot 
That sounds difficult. You might try adding a `conda list` to the end of
`ci/install_travis.sh` to print out the final version of all the packages.

My also throw a `python -c "import http.client"` there too, to ensure that
our hypothesis about a backport is correct.

On Wed, Mar 21, 2018 at 11:43 AM, Michael Röttger <notifications@github.com>
wrote:

> *@mcrot* commented on this pull request.
> ------------------------------
>
> In pandas/io/excel.py
> <https://github.com/pandas-dev/pandas/pull/20437#discussion_r176154165>:
>
> > @@ -10,6 +10,7 @@
>  import abc
>  import warnings
>  import numpy as np
> +from http.client import HTTPResponse
>
> Hm, I've built a local development environment mit py27 and the tests fail
> as expected, because http.client cannot be imported. So it seems this is
> only related to the CI mechanism..
>
> In order to check the CI dependencies, I've made a small py2 script which
> collects all dependencies from the files ci/requirements-2.7(_[A-Z]+)?\
> .(build|pip|run), imports them one by one (assuming the correct version
> is installed) and tries to import http.client after each import. Haven't
> found a backport of http by doing this.
>
> I've added the script and my output here for reference. I had to manually
> install some of the collected dependencies before (couldn't find 'nomkl'
> btw). Not sure whether this helps.
>
> import osimport re
> import logging
> logging.basicConfig(level=logging.INFO)
>     if __name__=='__main__':
>     DEPS_DIR = '/home/mcrot/prj/pandas-mcrot-py2/ci'
>     DEPS_REGEX = r'^requirements-2.7(_[A-Z]+)?\.(build|pip|run)$'
>
>     DEPS_BLACKLIST = ['python', 'ipython']
>
>     DEPS_IMPORT_NAMES = { 'PyCrypto': 'Crypto',
>                           'pytables': 'tables',
>                           'python-dateutil': 'dateutil',
>                           'beautifulsoup4': 'bs4',
>                           'pandas-gbq': 'pandas_gbq'}
>
>     deps = set()
>
>     for fn in os.listdir(DEPS_DIR):
>         if re.match(DEPS_REGEX, fn):
>             logging.info("Loading deps from file '%s'..", fn)
>
>             f = open(os.path.join(DEPS_DIR, fn))
>
>             new_deps = []
>             for s in f.readlines():
>                 dep = s.split('=')[0].strip()
>
>                 if dep in DEPS_BLACKLIST:
>                     continue
>
>                 # rename if possible
>                 try:
>                     dep = DEPS_IMPORT_NAMES[dep]
>                 except KeyError:
>                     pass
>
>                 new_deps.append(dep)
>
>             deps = deps.union(new_deps)
>
>     deps = sorted(d for d in deps)
>
>     logging.info("Checking these dependencies: %s", deps)
>
>     for dep in deps:
>
>         try:
>             mod = __import__(dep)
>         except ImportError:
>             logging.warning("Cannot import module '%s'", dep)
>             continue
>
>         try:
>             from http.client import HTTPResponse
>             logging.error("Module '%s' DOES IMPORT http.", dep)
>             raise Exception("Backport of http detected in module '%s'" % str(mod))
>         except ImportError as exc:
>             # as expected for py2
>             logging.info("Module '%s' does not import http.", mod)
>
> check-backport.txt
> <https://github.com/pandas-dev/pandas/files/1834240/check-backport.txt>
>
> So I guess, I need a local CI in order to find out why http.client can be
> imported?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20437#discussion_r176154165>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHInpeAh5hpMqgPugPwF7Y70MGz9cyks5tgoNDgaJpZM4SzZlx>
> .
>

this might simpler, in ``pandas/compat/__init__.py`` we *already* do

```
try:
    import __builtin__ as builtins
    # not writeable when instantiated with string, doesn't handle unicode well
    from cStringIO import StringIO as cStringIO
    # always writeable
    from StringIO import StringIO
    BytesIO = StringIO
    import cPickle
    import httplib
except ImportError:
    import builtins
    from io import StringIO, BytesIO
    cStringIO = StringIO
    import pickle as cPickle
    import http.client as httplib
```

so you are likely getting:https://docs.python.org/2/library/httplib.html in python 2 (assuming we are doing the import from compat, which we *should* be doing)
> which we should be doing

Right, but the latest CI run did an unconditional `import http.client`, which should have failed on py2 (and does for us locally, but not on any of the CI runs).
its possible this test is skipped on a 2.7 run?
Possibly, but I *think* that the import is hit by an `import pandas`, so that shouldn't matter. The entire test suite should fail (unless I'm misunderstanding something).
The test is not skipped and the `import http.client` statement is on top of `pandas.io.excel` and unconditional, so it is executed on every test which imports the `excel` modul. 
I've created a new branch in my repo, activated TravisCI and added the suggested commands at the end of the install script. Let's see..
Ok, the TravisCI run my branch [`why-does-import-http-client-work-in-py2`](https://github.com/mcrot/pandas/tree/why-does-import-http-client-work-in-py2) failed already  on the first `import pandas` as @TomAugspurger expected, see [here](https://travis-ci.org/mcrot/pandas). I don't understand why the pandas-dev TravisCI runs pass without problems - and also for the other two CI environments.

I've had a look at of a diff of TravisCI logs of both instances and haven't found a significant difference yet. The TravisCI on my repo had to built some wheels first and couldn't resuse Cython cache as the pandas-dev instance did. That shouldn't matter?!

However I'm going to temporarily add the extra output of `conda list` at the end of `travis_install.sh` also to this PR in order to see its output on pandas-dev TravisCI instance. Just to be sure we have the same package versions everywhere.

Concerning this PR here, maybe this py2 CI issue is slowly getting a little off-topic.. maybe I should be handled elsewhere in the long run, I'm not sure how you normally deal with CI issues like that. Or there is an easy explanation I cannot see, then sorry for the noise.

> Concerning this PR here, maybe this py2 CI issue is slowly getting a little off-topic.. maybe I should be handled elsewhere in the long run

Yes, feel free to just make the changes to fix the excel bug (ideally tested locally with Py2), and we'll fix the CI in a followup.
Ok, thanks .. it's a little weird, because this time the tests here on pandas-dev also fail for py27 as expected.

However I've continued with the PR and just catched `OSError` and `ValueError` as suggested by @TomAugspurger. Btw. would catching `io.UnsupportedOperation` be too specific? It is available in py2 and py3. 

Locally the tests run both for py27 and py36, I will push now. I hope this PR does not affect negatively affect ASV tests as mentioned by @WillAyd.
> Btw. would catching io.UnsupportedOperation be too specific? It is available in py2 and py3.

That should be equivalent, right? I just assumed it wasn't around in python2, but if so the use that.
Ok, I'm catching `io.UnsupportedOperation` now instead of `(OSError, ValueError)`. It's working locally for py36 and py27. I've pushed to this branch.
Great.

Could you open an issue with a small summary to what we found here about
the CI issue?

On Thu, Mar 22, 2018 at 10:00 AM, Michael Röttger <notifications@github.com>
wrote:

> Ok, I'm catching io.UnsupportedOperation now instead of (OSError,
> ValueError). It's working locally for py36 and py27. I've pushed to this
> branch.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20437#issuecomment-375337524>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIsROzV6mBjLbLc2acb7Y9fYe0Et4ks5tg7yMgaJpZM4SzZlx>
> .
>

Done, see #20453.
can you add the test from the issue? 
The test which reveals #20434 is already included in the code base. I openend #20434, because
I'd just setup a working environment following the guide [Contributing to pandas](https://pandas-docs.github.io/pandas-docs-travis/contributing.html) and afterwards, when running the tests, especially
```
pytest pandas/tests/io/test_excel.py
```
I got failures for the test method `TestXlrdReader.test_read_from_http_url`. The failure is described in #20434.

I understand now that is strange that my tests didn't run, because #19926 introduced the seek() method and the tests should have failed already in the past for that PR. Probably the issue here is more related to my setup than to the code base or the CI, but I'm not sure. Now I think, a better approach would have been to wait for somebody else to confirm the issue #20434 before creating this PR here, right?
if the test doesn't fail on CI, then either your fix doesn't actually fix anything, or the CI is not running it (or properly running it). so need to have a failure before it can get fixed.
@mcrot @jreback I suspect this is because we don't run slow tests on py3. So we can do a couple things

- mock the test at the network layer and remove the slow marker
- ensure that at least one of our slow jobs in running on 3.6

We should make sure that we run the slow test against 3.6 regardless of 2.7. I'm planning some cleanup of our CI setup, to make these easier to spot.

Regarding this PR, I'm fine with merging as is. I'm not sure how difficult mocking would be, but I'd like to get our tests passing again.
https://github.com/pandas-dev/pandas/pull/20559

let's see if this fails
can you rebase this. happy to merge this.
Merged in master and re-ran. @mcrot ping on green.
Green from my point of view. Thanks @jreback @TomAugspurger. If you still need me to do a rebase (not sure), I'm back in my office in about 15h.

Thanks for the fix, and for turning up the CI issues :)