use requests when it is installed #28874

ocefpaf · 2019-10-09T18:31:52Z

closes #16716
closes #28825
closes #28826
solves #16910

tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Continuation of #21504 and #17087

This PR is not ready but I'd appreciate any early comment/feedback.

pandas/io/common.py

ocefpaf · 2019-10-09T18:39:47Z

pandas/io/excel/_base.py

@@ -336,10 +336,10 @@ def read_excel(


 class _BaseExcelReader(metaclass=abc.ABCMeta):
-    def __init__(self, filepath_or_buffer):
+    def __init__(self, filepath_or_buffer, session=None):


I'm a bit lost on how to pass the session object to this class, right now it defaults to None b/c it does not get the value from read_excel.

jbrockmendel · 2019-10-09T19:36:41Z

pandas/io/excel/_base.py

@@ -20,7 +20,7 @@
    _stringify_path,
    _validate_header_arg,
    get_filepath_or_buffer,
-    urlopen,
+    _urlopen,


can you use the non-private version

jbrockmendel · 2019-10-09T19:37:16Z

pandas/io/common.py

+        r.raise_for_status()
+        content = r.content
+        r.close()
+    except ImportError:


can you use pandas.compat._optional.import_optional_dependency here

Not sure if I got it right.

WillAyd · 2019-10-10T01:47:04Z

Haven't read through the attached issue but this is a little more complicated than I would have hoped for. Is there a reason why you think this is in scope of pandas? Pandas can operate for the most part on items that adhered to the IOBase definition.

I don't think dealing with the requests.models.Response object is within scope

jreback · 2019-10-10T12:57:06Z

pandas/io/parsers.py

@@ -342,6 +342,9 @@
    values. The options are `None` for the ordinary converter,
    `high` for the high-precision converter, and `round_trip` for the
    round-trip converter.
+session : requests.Session
+    object with the a requests session configuration for remote file.
+    (requires the requests library)


add a version added tag

Not sure what that means.

pandas/io/common.py

jreback · 2019-10-10T13:00:29Z

pandas/io/common.py

+    compression = None
+    content_encoding = None
+    try:
+        import requests


need to use the pandas.compat._optional.import_optional_dependency

ocefpaf · 2019-10-10T14:53:57Z

Haven't read through the attached issue but this is a little more complicated than I would have hoped for. Is there a reason why you think this is in scope of pandas? Pandas can operate for the most part on items that adhered to the IOBase definition.

I don't think dealing with the requests.models.Response object is within scope

I'm assuming you are taking about the session object, right? If so what did you hope for and what do you recommend me to modify to improve this?

Some background. At NOAA we need the session object in order to read URLs that are under password protected servers. I guess we can try the .netrc approach but it would be nicer to do this without relying on local files.

jbrockmendel · 2019-10-10T15:09:49Z

IIRC pandas_datareader supports requests Sessions

ocefpaf · 2019-10-10T15:17:17Z

IIRC pandas_datareader supports requests Sessions

I know. That is what inspired the original poster of this PR.

I'm traveling back home today and I'll address the review comments during the weekend. Thanks!

WillAyd · 2019-10-10T15:47:21Z

I guess I'm not clear on what Sessions are doing here. Neither of the issues that this closes reference Session objects anywhere - are you sure this closes them?

Can you add test cases?

ocefpaf

I guess I'm not clear on what Sessions are doing here. Neither of the issues that this closes reference Session objects anywhere - are you sure this closes them?

The original issue was #16716 by @skynss and the original PR used session. See #17087

Allow us to pass user/password, for example, for password protected servers.
One can also use a local .netrc file. Let's remove the session for now to see if we can make progress.

Can you add test cases?

Sure. Writing those, just wanted to be sure this will get a green signal before investing more time on it.

TomAugspurger

FWIW, we've been having multiple conversations about how users pass through kwargs from a read like pd.read_csv(...) to the "file open" call. Sometimes it's just the builtin open(path), but in this case it might be requests.get(...) At some point, I'd like to unify these into a backend_kwargs or open_kwargs dict that gets passed through (but that's for a separate issue. I'll try to open one).

TomAugspurger · 2019-11-04T11:25:09Z

pandas/io/common.py

+        r = urllib.request.urlopen(*args, **kwargs)
+        content = r.read()
+        content_encoding = r.headers.get("Content-Encoding", None)
+    if content_encoding == "gzip":


I think this also needs to be under the except ImportError? From what I can tell, requests .content will automatically decode gzip: https://requests.readthedocs.io/en/master/user/quickstart/#binary-response-content

WillAyd · 2019-12-17T17:25:08Z

@ocefpaf still active? Can you address comments and check CI failures?

ocefpaf · 2019-12-17T18:11:10Z

I'll probably have some time for this during the end of the year break. If someone wants to pick it up from here before that please feel free to do so.

jreback · 2020-01-01T20:57:33Z

@ocefpaf if you can merge master and see if you can get passing would be great.

MarcoGorelli · 2020-01-21T13:27:23Z

Hi @ocefpaf - sorry to chase you up, just wanted to ask whether you're still working on this :)
Seems that there's some test failures, but I can't check them as it says "build not found"

WillAyd · 2020-02-06T23:59:52Z

Closing as I think stale, but certainly ping if you'd like to continue

use requests when it is installed

2031cfe

ocefpaf commented Oct 9, 2019

View reviewed changes

pandas/io/common.py Outdated Show resolved Hide resolved

ocefpaf commented Oct 9, 2019

View reviewed changes

pop session out before calling urllib.request.urlopen

95e3b75

jbrockmendel reviewed Oct 9, 2019

View reviewed changes

simonjayhawkins added API Design Enhancement IO CSV read_csv, to_csv IO Excel read_excel, to_excel IO HTML read_html, to_html, Styler.apply, Styler.applymap IO JSON read_json, to_json, json_normalize labels Oct 9, 2019

jreback reviewed Oct 10, 2019

View reviewed changes

ocefpaf added 2 commits October 10, 2019 11:58

no session obj for now

ac39c2e

use import_optional_dependency

03959aa

ocefpaf commented Oct 10, 2019

View reviewed changes

documento min requests version

02a2365

WillAyd mentioned this pull request Oct 11, 2019

BUG: Patch for skipping seek() when loading Excel files from urllib3.response.HTTPResponse object #28826

Closed

4 tasks

onsimon mentioned this pull request Oct 18, 2019

UnsupportedOperation 'seek' when loading excel files from urllib3.response.HTTPResponse object #28825

Closed

TomAugspurger reviewed Nov 4, 2019

View reviewed changes

WillAyd closed this Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use requests when it is installed #28874

use requests when it is installed #28874

ocefpaf commented Oct 9, 2019 •

edited

Loading

ocefpaf Oct 9, 2019

jbrockmendel Oct 9, 2019

ocefpaf Oct 10, 2019

jbrockmendel Oct 9, 2019

ocefpaf Oct 10, 2019

WillAyd commented Oct 10, 2019

jreback Oct 10, 2019

ocefpaf Oct 10, 2019

jreback Oct 10, 2019

ocefpaf commented Oct 10, 2019

jbrockmendel commented Oct 10, 2019

ocefpaf commented Oct 10, 2019

WillAyd commented Oct 10, 2019

ocefpaf left a comment •

edited

Loading

TomAugspurger left a comment

TomAugspurger Nov 4, 2019

WillAyd commented Dec 17, 2019

ocefpaf commented Dec 17, 2019

jreback commented Jan 1, 2020

MarcoGorelli commented Jan 21, 2020

WillAyd commented Feb 6, 2020

use requests when it is installed #28874

use requests when it is installed #28874

Conversation

ocefpaf commented Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocefpaf commented Oct 10, 2019

jbrockmendel commented Oct 10, 2019

ocefpaf commented Oct 10, 2019

WillAyd commented Oct 10, 2019

ocefpaf left a comment • edited Loading

Choose a reason for hiding this comment

TomAugspurger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Dec 17, 2019

ocefpaf commented Dec 17, 2019

jreback commented Jan 1, 2020

MarcoGorelli commented Jan 21, 2020

WillAyd commented Feb 6, 2020

ocefpaf commented Oct 9, 2019 •

edited

Loading

ocefpaf left a comment •

edited

Loading