[ENH] Add read support for Google Cloud Storage #20729

bnaul · 2018-04-18T01:48:25Z

closes ENH Adding read_* support for Google Cloud Storage gs:// URLs #19454
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Couple of remaining issues:

I had to do some weird logging magic to suppress the output of gcsfs when catching authentication exceptions
Proper Parquet support seems to require more specialization so I didn't try
I'm not sure how to properly test this functionality without a pandas-test bucket and/or a mock GCS library like moto

cc @martindurant who might have some thoughts on other things I am doing wrong

bnaul · 2018-04-18T02:24:48Z

I'm also not sure what the all the various requirements files I touched are, I just mimicked the s3fs pattern; looks like Circle is unhappy with that, should I revert everything in ci/ or is there some other step I'm missing?

martindurant · 2018-04-18T13:11:42Z

doc/source/install.rst

@@ -275,6 +275,7 @@ Optional Dependencies

 * `Jinja2 <http://jinja.pocoo.org/>`__: Template engine for conditional HTML formatting.
 * `s3fs <http://s3fs.readthedocs.io/>`__: necessary for Amazon S3 access (s3fs >= 0.0.7).
+* `gcsfs <http://gcsfs.readthedocs.io/>`__: necessary for Google Cloud Storage access (gcsfs >= 0.6.0).


martindurant · 2018-04-18T13:15:18Z

Testing in gcsfs is done via the VCR package, which records all HTTP calls and their responses from the server, for playback during the running of a test. It is rather finicky/awkward to use.

Parquet is difficult because you need to supply the open function rather than file-like objects?

TomAugspurger

Not sure about testing yet. VCR has sounded like more trouble than it's worth.

TomAugspurger · 2018-04-18T14:10:47Z

pandas/io/common.py

+    """Check for a gcs url"""
+    try:
+        return parse_url(url).scheme in ['gcs', 'gs']
+    except:  # noqa


Make this except Exception, or even better figure out what parse_url can raise, and catch those (plus AttributeError).

From looking over urlparse it seems that it will also only raise AttributeError so I used that. For consistency I changed the other url parsers excepts as well.

TomAugspurger · 2018-04-18T14:11:36Z

pandas/io/gcs.py

+        mode = 'rb'
+
+    gcsfs_logger_disabled = gcsfs.core.logger.disabled
+    gcsfs.core.logger.disabled = True


Why disable this? I'd rather leave that up to the user if possible.

This was probably an overly complicated solution; the problem I was facing was that gcsfs retries several times if it fails to authenticate, and each failure prints a long traceback. But if that's the behavior of the library then I guess there's no sense in using hacks to get around it. For now I removed all of the logging magic.

TomAugspurger · 2018-04-18T14:14:12Z

pandas/io/gcs.py

+    try:
+        filepath_or_buffer = fs.open(filepath_or_buffer, mode)
+    except (compat.FileNotFoundError, GoogleAuthError, gcsfs.utils.HtmlError):
+        fs = gcsfs.GCSFileSystem(token='anon')


note: this was originally a hack for not breaking backwards compat with pre-s3fs pandas. I'd like to eventually move to something like dask's store_options here, a dict of keywords that gets passed down to the backend (#19904)

I agree this would be the best way to do things. For now I simplified things and am just using gcsfs's default authentication preferences. If allowing anonymous access to public files is important we could try to think of a better way to accomplish that, but arguably gcsfs should handle that logic?

Note that gcsfs defaults to anon access if no other mechanisms worked - but this will not happen if one of the other mechanisms contained real credentials that turned out to be no longer valid.

That's what I thought but it didn't seem to be the case when I tested it. In a python:3.6.4 Docker image w/ gcsfs installed I get:

# default auth fails In [2]: gcsfs.GCSFileSystem().open('gs://gcp-public-data-landsat/index.csv.gz').read(10) # lots of error logs from failing to access /computeMetadata/v1/instance/service-accounts/default/?recursive=true ... RefreshError: HTTPConnectionPool(host='metadata.google.internal', port=80): Max retries exceeded with url: /computeMetadata/v1/instance/service-accounts/default/?recursive=true (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fab36881518>: Failed to establish a new connection: [Errno -2] Name or service not known',)) # specifying token='anon' works In [3]: gcsfs.GCSFileSystem(token='anon').open('gs://gcp-public-data-landsat/index.csv.gz').read(10) Out[3]: b'\x1f\x8b\x08\x08\x89\xd8\xd6Z\x02\xff'

Should I create a separate gcsfs issue for this?

Hm, OK, it should be an issue - but worth checking whether it's still the case with current master.

TomAugspurger · 2018-04-18T14:15:46Z

pandas/io/gcs.py

+        fs = gcsfs.GCSFileSystem(token='anon')
+        filepath_or_buffer = fs.open(filepath_or_buffer, mode)
+    finally:
+        gcsfs.core.logger.disabled = gcsfs_logger_disabled


If the anon version fails, this line is not hit, correct? It would have to be wrapped in a try / finally as well (or ideally the logger stuff would be removed).

codecov · 2018-04-18T17:09:32Z

Codecov Report

Merging #20729 into master will decrease coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master   #20729      +/-   ##
==========================================
- Coverage   91.91%    91.9%   -0.01%     
==========================================
  Files         153      154       +1     
  Lines       49532    49550      +18     
==========================================
+ Hits        45525    45541      +16     
- Misses       4007     4009       +2

Flag	Coverage Δ
#multiple	`90.3% <90%> (-0.01%)`	⬇️
#single	`41.86% <40%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/_print_versions.py	`15.71% <ø> (ø)`	⬆️
pandas/io/json/json.py	`92.47% <ø> (ø)`	⬆️
pandas/io/common.py	`71.2% <100%> (+0.92%)`	⬆️
pandas/io/gcs.py	`80% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1033e8b...2745359. Read the comment docs.

bnaul · 2018-04-20T16:52:14Z

The issue with anonymous credentials in gcsfs appears to be resolved by fsspec/gcsfs#100, so once that is released I think this PR should cover most use cases.

Any more thoughts re: testing? Would simply mocking out gcsfs.GCSFileSystem.read to return a simple CSV string be sufficient?

martindurant · 2018-04-20T16:56:37Z

This was not implemented in Dask (yet), but the MemoryFileSystem might be a nice and simple mock for GCSFileSystem.

martindurant · 2018-04-20T17:18:36Z

gcsfs v0.0.7 being released: conda-forge/gcsfs-feedstock#7

bnaul · 2018-05-07T20:32:50Z

Any more thoughts about testing? Would a limited set of tests based on mock.patching GCSFileSystem.open be sufficient for now?

jreback · 2018-05-08T00:03:52Z

I think we would need some tests, mocking would be ok

TomAugspurger · 2018-05-10T12:06:14Z

pandas/io/gcs.py

+        mode = 'rb'
+
+    fs = gcsfs.GCSFileSystem()
+    filepath_or_buffer = fs.open(filepath_or_buffer, mode)


@bnaul do you think this is an appropriate place to mock? fs.open could return a BytesIO object?

Added a simple mock-based test here; any other methods I should include besides read_csv?

TomAugspurger

Could you also

add a whatsnew 0.24 probably
rerun python scripts/convert_deps.py so that gcsfs is in the optional pip deps.

TomAugspurger · 2018-05-26T19:06:27Z

pandas/tests/io/test_gcs.py

+    with patch('gcsfs.GCSFileSystem') as MockFileSystem:
+        instance = MockFileSystem.return_value
+        instance.open.return_value = BytesIO(b'a,b\n1,2\n3,4')
+        df = read_csv('gs://test/test.csv')


Mock let's you assert that certain functions are called, right?

I guess, maybe we could have a second test that ensures pandas.io.gcs.get_filepath_or_buffer is called for gs:// type filepath_or_buffer?

pep8speaks · 2018-06-07T00:39:59Z

Hello @bnaul! Thanks for updating the PR.

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

Comment last updated on June 25, 2018 at 23:43 Hours UTC

bnaul · 2018-06-07T00:44:26Z

@TomAugspurger sorry for the delay, was away for a while; made the requested changes!

TomAugspurger · 2018-06-07T12:43:53Z

Changes look good, but the CI is failing on builds that don't have gcsfs. Could you 1. Add a `td.skip_if_no('gcsfs')` decorator to the tests that use gcsfs 2. Add a test that is skipped if gcsfs *is* present, and checks that not having gcsfs raises with the right error message.

…

On Wed, Jun 6, 2018 at 7:44 PM, Brett Naul ***@***.***> wrote: @TomAugspurger <https://github.com/TomAugspurger> sorry for the delay, was away for a while; made the requested changes! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20729 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIrs1pKdLMMEHHo_6qsHegV6FrlDyks5t6HdugaJpZM4TZS32> .

jreback · 2018-06-14T10:27:01Z

ci/requirements_dev.txt

@@ -9,4 +9,4 @@ python-dateutil>=2.5.0
 pytz
 setuptools>=24.2.0
 sphinx
-sphinxcontrib-spelling


what changed here? can you revert this file

jreback · 2018-06-14T10:28:07Z

ci/travis-36-slow.yaml

@@ -5,6 +5,7 @@ channels:
 dependencies:
  - beautifulsoup4
  - cython
+  - gcsfs


so I would remove gcsfs from the slow ci files (not tested anyhow as these tests are not slow)

jreback · 2018-06-14T10:29:16Z

ci/circle-36-locale.yaml

@@ -5,6 +5,7 @@ channels:
 dependencies:
  - beautifulsoup4
  - cython
+  - gcsfs


let's remove from here to make sure the tests are skipped properly (as tested in 2.7 / 3.6 builds already)

jreback · 2018-06-14T10:33:49Z

pandas/tests/io/test_gcs.py

+
+@td.skip_if_no('gcsfs')
+def test_read_csv_gcs():
+    try:


hmm we do install mock in py27, can you move this to pandas.util.testing (and then just import patch from there)

Looks like a custom version is already in util.testing; it's only used in one place so I swapped it out for mock.patch

I don't think we should make changes to util/testing.py. We don't want to require mock (for py27), and testing will be awkward if it's in pandas/util.

Better to make a little fixture or something for it.

@TomAugspurger do you mean a fixture that does use mock, but in another directory that won't get imported outside of testing? Or a fixture that doesn't use mock at all?

Edit: for now I just reverted to the try/except Import in the test. I'm not sure how to reconcile this w/ @jreback 's comment but it seems like any way is fine (I'm surprised there aren't already more places in the tests where mock objects need to be used...).

jreback · 2018-06-14T10:34:55Z

pandas/tests/io/test_gcs.py

+        instance = MockFileSystem.return_value
+        instance.open.return_value = BytesIO(b'a,b\n1,2\n3,4')
+        df = read_csv('gs://test/test.csv')
+


can you use tm.assert_from_equal so dtypes are tested

jreback · 2018-06-14T10:35:00Z

pandas/tests/io/test_gcs.py

+                                        False)
+        df = read_csv('gs://test/test.csv')
+
+    assert isinstance(df, DataFrame)


can you add other dtypes such as float, datetime, string, and some NaNs

TomAugspurger · 2018-06-25T16:56:52Z

I meant a fixture that provides the imported module. Like pytest imporotskip, but with the try / except for the two locations.

________________________________ From: Brett Naul <notifications@github.com> Sent: Monday, June 25, 2018 11:40:55 AM To: pandas-dev/pandas Cc: Tom Augspurger; Mention Subject: Re: [pandas-dev/pandas] [ENH] Add read support for Google Cloud Storage (#20729) @bnaul commented on this pull request.

________________________________ In pandas/tests/io/test_gcs.py<#20729 (comment)>:

+

+from pandas import DataFrame, read_csv +from pandas.compat import BytesIO +from pandas.io.common import is_gcs_url +from pandas.util import _test_decorators as td + + +def test_is_gcs_url(): + assert is_gcs_url("gcs://pandas/somethingelse.com") + assert is_gcs_url("gs://pandas/somethingelse.com") + assert not is_gcs_url("s3://pandas/somethingelse.com") + + +@td.skip_if_no('gcsfs') +def test_read_csv_gcs(): + try: @TomAugspurger<https://github.com/TomAugspurger> do you mean a fixture that does use mock, but in another directory that won't get imported outside of testing? Or a fixture that doesn't use mock at all? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#20729 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIvOshqCcoEHlHlqX7eiP8SMY_LhIks5uARKXgaJpZM4TZS32>.

bnaul · 2018-06-25T17:12:29Z

Made a pytest fixture; if it belongs in somewhere more generic let me know, the various testing utils seem spread out over quite a few different places and it's a bit confusing to me

TomAugspurger · 2018-06-25T17:23:51Z

Not quite what I had in mind. I'll push a change in a minute :)

bnaul · 2018-06-25T17:30:37Z

Sounds good 🙃

TomAugspurger · 2018-06-25T17:32:05Z

OK, pushed. Only real changes were

returning mock instead of mock.patch
moving to conftest.py, since it's a generally useful fixture
skipping if neither unittest.mock nor mock are available

TomAugspurger · 2018-06-25T18:18:38Z

Fixed the linting error I introduced.

May have a random failure on PY37, will investigate more later.

bnaul · 2018-06-25T19:07:02Z

Oops might have overwritten that w/ the same change but I assume it's fine

bnaul · 2018-06-25T21:39:01Z

@jreback looking 🍏

TomAugspurger · 2018-06-26T14:59:28Z

Thanks @bnaul!

* Google Cloud Storage support using gcsfs

bnaul force-pushed the gcsfs branch from 1240f1e to 049313d Compare April 18, 2018 01:49

martindurant reviewed Apr 18, 2018

View reviewed changes

TomAugspurger reviewed Apr 18, 2018

View reviewed changes

bnaul force-pushed the gcsfs branch from 049313d to 4868bbc Compare April 18, 2018 17:09

bnaul force-pushed the gcsfs branch 2 times, most recently from 23e34bc to 0168784 Compare April 20, 2018 16:42

bnaul force-pushed the gcsfs branch from 0168784 to 4b3801a Compare May 7, 2018 20:31

bnaul force-pushed the gcsfs branch from 4b3801a to 77e8e56 Compare May 7, 2018 21:14

jreback added Enhancement IO Google labels May 8, 2018

TomAugspurger reviewed May 10, 2018

View reviewed changes

bnaul force-pushed the gcsfs branch 2 times, most recently from b11b2f8 to c722d60 Compare May 23, 2018 21:15

TomAugspurger reviewed May 26, 2018

View reviewed changes

bnaul force-pushed the gcsfs branch from c722d60 to 16cbf11 Compare June 7, 2018 00:39

bnaul force-pushed the gcsfs branch 2 times, most recently from 31e7ef5 to 6d6e727 Compare June 7, 2018 00:42

bnaul force-pushed the gcsfs branch 2 times, most recently from 5e270e8 to f6aba54 Compare June 8, 2018 21:21

jreback requested changes Jun 14, 2018

View reviewed changes

bnaul force-pushed the gcsfs branch 5 times, most recently from f996427 to 456734b Compare June 25, 2018 16:49

bnaul force-pushed the gcsfs branch from 456734b to c83546c Compare June 25, 2018 17:10

TomAugspurger approved these changes Jun 25, 2018

View reviewed changes

bnaul force-pushed the gcsfs branch from 9a824b1 to 6ca2586 Compare June 25, 2018 19:02

bnaul and others added 3 commits June 25, 2018 16:43

Google Cloud Storage support using gcsfs

7e31a04

Fixture for mock

e5aee91

Fix PEP8 unused import

2745359

bnaul force-pushed the gcsfs branch from 6ca2586 to 2745359 Compare June 25, 2018 23:43

TomAugspurger merged commit 95427d5 into pandas-dev:master Jun 26, 2018

bnaul deleted the gcsfs branch June 26, 2018 18:23

TomAugspurger mentioned this pull request Jun 28, 2018

BUG: Fix MI repr with long names #21655

Merged

jorisvandenbossche added this to the 0.24.0 milestone Jul 2, 2018

This was referenced Sep 13, 2018

Allow writing to S3 paths #8508

Closed

Support writing CSV to GCS #22704

Merged

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

[ENH] Add read support for Google Cloud Storage (pandas-dev#20729)

6369bf3

* Google Cloud Storage support using gcsfs

leg100 mentioned this pull request Aug 13, 2019

Credentials check fails when using VPC Service Controls fsspec/gcsfs#164

Closed

[ENH] Add read support for Google Cloud Storage #20729

[ENH] Add read support for Google Cloud Storage #20729

Conversation

bnaul commented Apr 18, 2018 • edited Loading

bnaul commented Apr 18, 2018

Choose a reason for hiding this comment

martindurant commented Apr 18, 2018

TomAugspurger left a 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

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

codecov bot commented Apr 18, 2018 • edited Loading

Codecov Report

bnaul commented Apr 20, 2018 • edited Loading

martindurant commented Apr 20, 2018 • edited Loading

martindurant commented Apr 20, 2018

bnaul commented May 7, 2018

jreback commented May 8, 2018

Choose a reason for hiding this comment

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

pep8speaks commented Jun 7, 2018 • edited Loading

Comment last updated on June 25, 2018 at 23:43 Hours UTC

bnaul commented Jun 7, 2018

TomAugspurger commented Jun 7, 2018 via email

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

Choose a reason for hiding this comment

bnaul Jun 25, 2018 • 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

Choose a reason for hiding this comment

TomAugspurger commented Jun 25, 2018 via email

bnaul commented Jun 25, 2018

TomAugspurger commented Jun 25, 2018

bnaul commented Jun 25, 2018

TomAugspurger commented Jun 25, 2018

TomAugspurger commented Jun 25, 2018

bnaul commented Jun 25, 2018

bnaul commented Jun 25, 2018

TomAugspurger commented Jun 26, 2018

bnaul commented Apr 18, 2018 •

edited

Loading

codecov bot commented Apr 18, 2018 •

edited

Loading

bnaul commented Apr 20, 2018 •

edited

Loading

martindurant commented Apr 20, 2018 •

edited

Loading

pep8speaks commented Jun 7, 2018 •

edited

Loading

bnaul Jun 25, 2018 •

edited

Loading