COMPAT: reading json with lines=True from s3, xref #17200 #17201

alph486 · 2017-08-08T18:39:45Z

Attempt to decode the bytes array with encoding passed to the call.

closes read_json(lines=True) broken for s3 urls in Python 3 (v0.20.3) #17200

Attempt to decode the bytes array with `encoding` passed to the call.

gfyoung · 2017-08-08T18:46:01Z

@alph486 : Thanks for the PR! You will need to write tests for us to merge this. I would also take a look at what @TomAugspurger suggested in terms of patching this.

alph486 · 2017-08-08T19:13:55Z

Is there a particular way you guys go about handling s3 in your tests? I see some files in a bucket somewhere, but idk if there are any jsonl type files or how to add one. For that matter, I don't see any tests for read_json where the file is in s3.

This reverts commit 8255cd0.

pep8speaks · 2017-08-08T20:37:41Z

Hello @alph486! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on November 27, 2017 at 00:04 Hours UTC

alph486 · 2017-08-08T20:43:09Z

@gfyoung After going back to clean this up I realized jsonl from URL based file also suffers from the same issue. I have a cleaner fix that I think works in both cases.

Can you let me know how I should proceed for getting a JSONL file uploaded to the public s3 bucket for testing this in the test suite? I don't have write access.

gfyoung · 2017-08-08T20:44:11Z

Can you let me know how I should proceed for getting a JSONL file uploaded to the public s3 bucket for testing this in the test suite? I don't have write access.

@TomAugspurger @jreback : thoughts?

jreback · 2017-08-08T23:25:20Z

if you put the file on a gist somewhere @TomAugspurger or I can upload to the bucket.

alph486 · 2017-08-09T14:42:23Z

@jreback @gfyoung Thanks for the assist here. I'll wrap this up soon; just after I get my current project completed that requires this change.

alph486 · 2017-08-15T19:12:35Z

@jreback @TomAugspurger Here's a file that I use in my tests. If you can give me an s3 path where you drop it I'll push the tests. https://gist.github.com/alph486/c7eff9a896d80dccae5a3b3283e75a4e#file-items-jsonl

alph486 · 2017-08-18T18:50:56Z

bump?

gfyoung · 2017-08-18T18:53:44Z

@alph486 : Are you still waiting on the updated URL to patch the failing tests?

alph486 · 2017-08-18T18:55:50Z

@gfyoung yeah, i just need a target s3 url to put in my code

gfyoung · 2017-08-18T18:59:02Z

ping @jreback @TomAugspurger

TomAugspurger · 2017-08-18T21:02:21Z

I don't think I have access to the pandas s3 bucket.

…

On Fri, Aug 18, 2017 at 1:59 PM, gfyoung ***@***.***> wrote: ping @jreback <https://github.com/jreback> @TomAugspurger <https://github.com/tomaugspurger> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIvlpJ-cAwFzSmyAFeY270PMEynkhks5sZd78gaJpZM4OxISG> .

alph486 · 2017-08-21T15:01:11Z

@jreback ?

jreback · 2017-08-21T23:56:32Z

yeah I don't think I have this either. @TomAugspurger IIRC you can login using the Continuum dashboard.

jreback · 2017-08-21T23:56:46Z

if that works we should prob change it :>

TomAugspurger · 2017-08-22T01:32:24Z

Hmm I'm not seeing it in the environment I have access to. I'll ask IT. Probably best to have it under an account we manage anyways.

…

On Mon, Aug 21, 2017 at 6:56 PM, Jeff Reback ***@***.***> wrote: if that works we should prob change it :> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIjccf2YRr9hMExOy0a3VnV3HTBXBks5sahlCgaJpZM4OxISG> .

alph486 · 2017-08-24T02:54:33Z

sorry to keep nudging here, but any progress from IT @TomAugspurger ? Id love to try to a patched official version out so we can use our feature in production.

TomAugspurger · 2017-08-24T11:04:29Z

No word yet.

Really, we should be using https://github.com/spulec/moto to mock all our S3 calls. Now that s3fs (and boto3) are handling all the network stuff, we don't need to be testing it. Just our text parsing. I'll open an issue about that (and you can look at fixing it if you're interested :)

#17325

alph486 · 2017-08-24T13:16:19Z

I can try to implement my tests using moto... we'll see if that works Thanks, Kevin

…

On Aug 24, 2017, 6:04 AM -0500, Tom Augspurger ***@***.***>, wrote: No word yet. Really, we should be using https://github.com/spulec/moto to mock all our S3 calls. Now that s3fs (and boto3) are handling all the network stuff, we don't need to be testing it. Just our text parsing. I'll open an issue about that (and you can look at fixing it if you're interested :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

TomAugspurger · 2017-08-24T13:28:39Z

Thanks, I'd be totally fine with just using moto for your test for now, and following up moving most of the other S3 tests to use moto.

You'll need to edit one of the ci files to include moto. Probably ci/requirements-3.6.run

alph486 · 2017-08-24T13:31:26Z

Ah, thanks for that. I hope to get to trying that locally today, I'll update here when I know. Thanks, Kevin

…

On Aug 24, 2017, 8:28 AM -0500, Tom Augspurger ***@***.***>, wrote: Thanks, I'd be totally fine with just using moto for your test for now, and following up moving most of the other S3 tests to use moto. You'll need to edit one of the ci files to include moto. Probably ci/requirements-3.6.run — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

alph486 · 2017-08-29T16:57:33Z

Any guidance on which requirements files I should change for both release packaging, development, and CI? (I currently have ci/requirements-3.6.run and requirements_dev.txt). I just have to add s3fs to some things and moto where appropriate.

jreback · 2017-08-29T17:01:46Z

you shouldn't add s3fs anywhere

alph486 · 2017-08-29T17:44:08Z

Unless I'm misunderstanding: you cannot run any of the test suites that require pandas.io.s3 locally unless s3fs is part of requirements_dev.txt no?

Edit:

For example from my attempt of running pytest pandas/tests/io/json/test_pandas.py -v:


pandas/tests/io/json/test_pandas.py:996:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pandas/io/json/json.py:324: in read_json
    encoding=encoding)
pandas/io/common.py:208: in get_filepath_or_buffer
    from pandas.io import s3
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    """ s3 support for remote file interactivity """
    from pandas import compat
    try:
        import s3fs
        from botocore.exceptions import NoCredentialsError
    except:
>       raise ImportError("The s3fs library is required to handle s3 files")
E       ImportError: The s3fs library is required to handle s3 files

pandas/io/s3.py:7: ImportError
=====================================

TomAugspurger · 2017-09-25T19:12:34Z

@alph486 close! Still a few style things in https://travis-ci.org/pandas-dev/pandas/jobs/279576239#L1938

FYI if you run flake8 pandas/tests/io it should print out all the issues. LMK if you want me to push a commit cleaning these up.

alph486 · 2017-09-25T21:04:38Z

@TomAugspurger Yeah, but part of those unused imports are a backwards way of me leveraging the s3_resource that arent necessarily just cleanup. I replied to a comment of yours earlier on about using s3_resource. I'd need to move it and potentially refactor some of that code to use it in my module. Can you check that out and lmk your thoughts?

TomAugspurger · 2017-09-25T21:11:47Z

@alph486 sorry I missed your comment earlier. I'll push a commit in a minute that refactors the s3_resource stuff.

alph486 · 2017-09-25T21:16:53Z

Ok thanks, sorry about it taking so long. I'm not super confident in here.

TomAugspurger · 2017-09-25T21:23:28Z

OK pushed two commits. One gets you back up to date with master, and 6979fb8 is the actual changes. I added a pandas/tests/io/conftest.py with the s3_resource that both places use now.

If you git pull to test locally, you'll have to rebuild the C extensions (`python setup.py build_ext --inplace). Hopefully everything is OK though.

alph486 · 2017-09-26T14:58:04Z

@jreback are you okay with the changes made per your review?

End in newline

Fixed the whitespace

TomAugspurger · 2017-09-26T17:36:23Z

Should be all good. ping on green @alph486

jreback · 2017-09-26T18:55:54Z

ci/requirements-3.5.pip

@@ -1,2 +1,3 @@
 xarray==0.9.1
 pandas-gbq


this needds to be reverted

jreback · 2017-09-26T18:56:49Z

ci/requirements_dev.txt

@@ -5,4 +5,4 @@ cython
 pytest>=3.1.0
 pytest-cov
 flake8


revert this, s3fs is NOT a requirement for dev; we should be robust to not having this installed

jreback · 2017-09-26T18:56:54Z

ci/requirements_all.txt

@@ -26,3 +26,4 @@ sqlalchemy
 bottleneck
 pymysql


jreback · 2017-09-26T18:57:14Z

pandas/io/json/json.py

+        """
+            If PY3 and/or isinstance(json, bytes)
+        """
+        if isinstance(json, bytes):


1 line comment

jreback · 2017-09-26T18:57:54Z

pandas/tests/io/parser/data/items.jsonl

@@ -0,0 +1,2 @@
+{"a": 1, "b": 2}


what is the purpose of this file?

I see, ok you have to have this named .json otherwise it won't be picked up by setup.py (IOW the install test will fail).

jreback · 2017-09-26T18:58:01Z

pandas/tests/io/parser/test_network.py

-@pytest.fixture(scope='module')
-def tips_file():
-    return os.path.join(tm.get_data_path(), 'tips.csv')
-


jreback · 2017-09-26T18:59:11Z

pandas/tests/io/parser/data/items.jsonl

@@ -0,0 +1,2 @@
+{"a": 1, "b": 2}


I see, ok you have to have this named .json otherwise it won't be picked up by setup.py (IOW the install test will fail).

jreback · 2017-11-10T20:20:00Z

can you rebase / update

jreback · 2017-11-27T11:35:00Z

thanks @alph486

…andas-dev#17201) (cherry picked from commit 4fd104a)

(cherry picked from commit 4fd104a)

Fix for #17200

8255cd0

Attempt to decode the bytes array with `encoding` passed to the call.

gfyoung added the IO JSON read_json, to_json, json_normalize label Aug 8, 2017

Revert "Fix for #17200"

caa3c80

This reverts commit 8255cd0.

Wrap BytesIO based streams when using

4709251

jreback changed the title ~~Fix for #17200~~ COMPAT: reading json with lines=True from s3, xref #17200 Aug 8, 2017

Kevin Kuhl added 2 commits September 22, 2017 09:37

Fixing link errors

78ee720

Code review formatting and compliance. Also use for testing

9d7e75b

Merge remote-tracking branch 'upstream/master' into alph486-patch-1

b21401b

REF: Move s3 mocking to conftest

6979fb8

TomAugspurger and others added 3 commits September 26, 2017 12:33

Update requirements-2.7.run

bb600cb

End in newline

Update requirements-2.7_SLOW.run

5b036ec

End in newline

End in newlines

c5c4d07

jreback requested changes Sep 26, 2017

View reviewed changes

TomAugspurger mentioned this pull request Oct 6, 2017

ENH: Add tranparent compression to json reading/writing #17798

Merged

4 tasks

jreback added 2 commits November 26, 2017 18:53

Merge branch 'master' into PR_TOOL_MERGE_PR_17201

f1122ca

linting & fix

b913972

jreback added this to the 0.21.1 milestone Nov 27, 2017

jreback added the Compat pandas objects compatability with Numpy or Python functions label Nov 27, 2017

jreback approved these changes Nov 27, 2017

View reviewed changes

jreback merged commit 4fd104a into pandas-dev:master Nov 27, 2017

jreback added the Needs Backport label Nov 27, 2017

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request Dec 8, 2017

COMPAT: reading json with lines=True from s3, xref pandas-dev#17200 (p…

80efa3e

…andas-dev#17201) (cherry picked from commit 4fd104a)

TomAugspurger pushed a commit that referenced this pull request Dec 11, 2017

COMPAT: reading json with lines=True from s3, xref #17200 (#17201)

30358fd

(cherry picked from commit 4fd104a)

TomAugspurger removed the Needs Backport label Dec 11, 2017

COMPAT: reading json with lines=True from s3, xref #17200 #17201

COMPAT: reading json with lines=True from s3, xref #17200 #17201

Conversation

alph486 commented Aug 8, 2017

gfyoung commented Aug 8, 2017

alph486 commented Aug 8, 2017

pep8speaks commented Aug 8, 2017 • edited Loading

Comment last updated on November 27, 2017 at 00:04 Hours UTC

alph486 commented Aug 8, 2017

gfyoung commented Aug 8, 2017

jreback commented Aug 8, 2017

alph486 commented Aug 9, 2017

alph486 commented Aug 15, 2017

alph486 commented Aug 18, 2017

gfyoung commented Aug 18, 2017

alph486 commented Aug 18, 2017

gfyoung commented Aug 18, 2017

TomAugspurger commented Aug 18, 2017 via email

alph486 commented Aug 21, 2017

jreback commented Aug 21, 2017

jreback commented Aug 21, 2017

TomAugspurger commented Aug 22, 2017 via email

alph486 commented Aug 24, 2017

TomAugspurger commented Aug 24, 2017 • edited Loading

alph486 commented Aug 24, 2017 via email

TomAugspurger commented Aug 24, 2017

alph486 commented Aug 24, 2017 via email

alph486 commented Aug 29, 2017

jreback commented Aug 29, 2017

alph486 commented Aug 29, 2017 • edited Loading

TomAugspurger commented Sep 25, 2017

alph486 commented Sep 25, 2017

TomAugspurger commented Sep 25, 2017

alph486 commented Sep 25, 2017

TomAugspurger commented Sep 25, 2017

alph486 commented Sep 26, 2017

TomAugspurger commented Sep 26, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 10, 2017

jreback commented Nov 27, 2017

pep8speaks commented Aug 8, 2017 •

edited

Loading

TomAugspurger commented Aug 24, 2017 •

edited

Loading

alph486 commented Aug 29, 2017 •

edited

Loading