ARROW-6655: [Python] Filesystem bindings for S3 #5423

kszucs · 2019-09-18T16:13:31Z

Add support for S3FileSystem in the python bindings.
Fixed issue with reading all the content of an S3 object
Introduce minio_server fixture for parametrized testing of all filesystem implementations
Fixed s3fs parquet test and updated it to use minio_server fixture

codecov-io · 2019-09-18T23:10:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@511c089). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5423   +/-   ##
=========================================
  Coverage          ?   80.84%           
=========================================
  Files             ?       90           
  Lines             ?     8854           
  Branches          ?        0           
=========================================
  Hits              ?     7158           
  Misses            ?     1342           
  Partials          ?      354

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 511c089...db89859. Read the comment docs.

python/pyarrow/_s3.pyx

kszucs · 2019-09-23T17:10:42Z

@pitrou PTAL, I still need to fix the AppVeyor builds.

pitrou

I'd rather see the testing simplified. Also I'm not sure we want to depend on the minio client for testing. Actual checks are already done on the C++ side.

ci/conda_env_python.yml

cpp/cmake_modules/ThirdpartyToolchain.cmake

python/pyarrow/_fs.pyx

python/pyarrow/_fs.pxd

python/pyarrow/_s3.pyx

python/pyarrow/fs.py

python/pyarrow/includes/libarrow_fs.pxd

python/pyarrow/tests/conftest.py

python/pyarrow/tests/test_fs.py

kszucs · 2019-09-24T14:48:06Z

python/pyarrow/tests/test_fs.py

-    with pytest.raises(ValueError):
-        aaa_stat.size
+    assert mtime_almost_equal(aaa_stat.mtime, fs.mtime(_aaa))
+    # type is inconsistent base_name has a trailing slas for 'aaa' and 'aaa/'


@pitrou if the directory's path ends with a slash then the file type is File, otherwise it is Directory, is this the expected outcome?

In which situation exactly? For guidance, you can take a look at the C++ tests, they should tell you what is expected.

kszucs · 2019-09-25T11:52:16Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -340,6 +340,12 @@ class ObjectInputFile : public io::RandomAccessFile {
    RETURN_NOT_OK(CheckClosed());
    RETURN_NOT_OK(CheckPosition(position, "read"));

+    nbytes = std::min(nbytes, content_length_ - position);


@pitrou this is the fix for stream.read() issue

Thanks! And thanks for the tests too.

kszucs · 2019-09-25T12:15:37Z

@pitrou I have a question about S3Options above.

Follow-up issues:

ARROW-6684: [C++/Python] S3FileSystem.create_dir should raise for a nested directory with recursive keyword set to False
ARROW-6685: [C++/Python] S3 FileStat object's base_path and type depends on trailing slash
Upstream aws-sdk-cpp issue about the hardcoded Xcode dependency

pitrou · 2019-09-25T12:18:49Z

@kszucs Sorry, what is the S3Options question already?

Would be nice if you could create those JIRAs and point them to me :-)

kszucs · 2019-09-25T12:37:45Z

@pitrou Created the JIRAs. That question I'm referring to is the keywords vs options object one. It is hidden by default in the GitHub thread, here is the link #5423 (comment)

pitrou · 2019-09-26T09:44:15Z

@kszucs I agree that using kwargs directly on S3FileSystem may look reasonable with the current settings. But S3 may/will need to grow more settings in the future. For example see the settings supported by boto3.

Once there are many settings it is nicer to have property access on an options object, like we already have on the CSV and JSON options objects.

kszucs · 2019-09-26T14:50:50Z

@pitrou now that has convinced me :)

pitrou

It's getting good! Just a couple questions below.

pitrou · 2019-09-30T14:37:37Z

ci/travis_script_python.sh

@@ -164,6 +168,10 @@ export PYARROW_BUILD_TYPE=$ARROW_BUILD_TYPE
 export PYARROW_WITH_PARQUET=1
 export PYARROW_WITH_PLASMA=1
 export PYARROW_WITH_ORC=1
+export PYARROW_WITH_S3=1
+if [ "$ARROW_TRAVIS_S3" == "1" ]; then
+  export PYARROW_WITH_S3=1


Hmm... I suppose there's a leftover here or above?

ARROW_TRAVIS_S3 is still optional, so we need it.

Yes, but look at the diff :-)

What do you mean, have I removed ARROW_TRAVIS_S3 or PYARROW_WITH_S3?

I mean this:

export PYARROW_WITH_S3=1 if [ "$ARROW_TRAVIS_S3" == "1" ]; then export PYARROW_WITH_S3=1

:), fixing it

pitrou · 2019-09-30T14:40:39Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -340,6 +340,12 @@ class ObjectInputFile : public io::RandomAccessFile {
    RETURN_NOT_OK(CheckClosed());
    RETURN_NOT_OK(CheckPosition(position, "read"));

+    nbytes = std::min(nbytes, content_length_ - position);


Thanks! And thanks for the tests too.

pitrou · 2019-09-30T14:42:55Z

python/pyarrow/_s3fs.pyx

+
+    Parameters
+    ----------
+    access_key: str, default None


Should update the docstring now :-)

python/pyarrow/tests/test_parquet.py

ci/conda_env_python.yml

python/pyarrow/tests/test_fs.py

kszucs · 2019-09-30T20:32:33Z

@pitrou updated

depends on #5423 Closes #5484 from kszucs/s3-conda and squashes the following commits: 7a3c67c <Krisztián Szűcs> test s3fs import ff6eb22 <Krisztián Szűcs> Include s3 support in the conda packages Authored-by: Krisztián Szűcs <[email protected]> Signed-off-by: Wes McKinney <[email protected]>

- Add support for S3FileSystem in the python bindings. - Fixed issue with reading all the content of an S3 object - Introduce `minio_server` fixture for parametrized testing of all filesystem implementations - Fixed s3fs parquet test and updated it to use minio_server fixture Closes #5423 from kszucs/s3 and squashes the following commits: 384c960 <Krisztián Szűcs> Resolve review comments 73e6625 <Krisztián Szűcs> S3Options 98bd91a <Krisztián Szűcs> remove commented tests db89859 <Krisztián Szűcs> rename to s3fs 4478458 <Krisztián Szűcs> fix read() issue c1df10b <Krisztián Szűcs> initialization in first use 192ab65 <Krisztián Szűcs> flake8 f70f9fb <Krisztián Szűcs> remove minio-client dependency d399643 <Krisztián Szűcs> simplify test suite fee57a9 <Krisztián Szűcs> remove accidentally committed files 751cfd4 <Krisztián Szűcs> resolve a couple of review comments; enum workaround 45436f7 <Krisztián Szűcs> cython flake8 38dcb88 <Krisztián Szűcs> rat c541b3e <Krisztián Szűcs> comment left 098048a <Krisztián Szűcs> more compat 00340ed <Krisztián Szűcs> fixture error handling 2be25ce <Krisztián Szűcs> auto initialize s3 on import 88e0c9f <Krisztián Szűcs> py2 compat 8585a60 <Krisztián Szűcs> py2 compat d372287 <Krisztián Szűcs> install minio in the conda-toolchain build 041cad4 <Krisztián Szűcs> executable flag fb0f281 <Krisztián Szűcs> travis minio install script 8cbe0ee <Krisztián Szűcs> travis osx 72e56a6 <Krisztián Szűcs> enable S3 in travis python builds 7800c75 <Krisztián Szűcs> appveyor flag 7daf566 <Krisztián Szűcs> fix syntax error in travis script 68eb591 <Krisztián Szűcs> enable PYARROW_WITH_S3 on appveyor 2cb19d1 <Krisztián Szűcs> conditional import of test dependencies efa05d2 <Krisztián Szűcs> use minio for dask.s3fs test too f25ae5a <Krisztián Szűcs> travis 9042c7e <Krisztián Szűcs> use S3FS_DIR 9ce7180 <Krisztián Szűcs> cmake format; fix orc cimport 45a2a17 <Krisztián Szűcs> docstrings c0b9162 <Krisztián Szűcs> test requirements; flake8 44aedfd <Krisztián Szűcs> stat test a343950 <Krisztián Szűcs> testing suite 1551b52 <Krisztián Szűcs> wip dd41d21 <Krisztián Szűcs> imports 5200af1 <Krisztián Szűcs> s3 filesystem bindings Authored-by: Krisztián Szűcs <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

depends on #5423 Closes #5484 from kszucs/s3-conda and squashes the following commits: 7a3c67c <Krisztián Szűcs> test s3fs import ff6eb22 <Krisztián Szűcs> Include s3 support in the conda packages Authored-by: Krisztián Szűcs <[email protected]> Signed-off-by: Wes McKinney <[email protected]>

pitrou reviewed Sep 19, 2019

View reviewed changes

python/pyarrow/_s3.pyx Outdated Show resolved Hide resolved

kszucs marked this pull request as ready for review September 22, 2019 10:51

kszucs force-pushed the s3 branch from 38ef63b to 87cd669 Compare September 22, 2019 10:54

kszucs changed the title ~~[Python] Filesystem bindings for S3~~ ARROW-6655: [Python] Filesystem bindings for S3 Sep 22, 2019

pitrou requested changes Sep 24, 2019

View reviewed changes

kszucs mentioned this pull request Sep 24, 2019

ARROW-6688: [Packaging] Include s3 support in the conda packages #5484

Closed

kszucs commented Sep 24, 2019

View reviewed changes

kszucs added 21 commits September 24, 2019 16:49

s3 filesystem bindings

5200af1

imports [skip ci]

dd41d21

wip [skip ci]

1551b52

testing suite

a343950

stat test

44aedfd

test requirements; flake8

c0b9162

docstrings

45a2a17

cmake format; fix orc cimport

9ce7180

use S3FS_DIR

9042c7e

travis

f25ae5a

use minio for dask.s3fs test too

efa05d2

conditional import of test dependencies

2cb19d1

enable PYARROW_WITH_S3 on appveyor

68eb591

fix syntax error in travis script

7daf566

appveyor flag

7800c75

enable S3 in travis python builds

72e56a6

travis osx

8cbe0ee

travis minio install script

fb0f281

executable flag

041cad4

install minio in the conda-toolchain build

d372287

py2 compat

8585a60

simplify test suite

d399643

kszucs force-pushed the s3 branch from 2630615 to d399643 Compare September 24, 2019 16:54

kszucs added 2 commits September 24, 2019 19:49

remove minio-client dependency

f70f9fb

flake8

192ab65

rok mentioned this pull request Sep 24, 2019

ARROW-1456: [Python] Run s3fs unit tests in Travis CI #5200

Closed

kszucs added 2 commits September 25, 2019 10:03

initialization in first use

c1df10b

fix read() issue

4478458

kszucs commented Sep 25, 2019

View reviewed changes

kszucs force-pushed the s3 branch from 2485fb4 to eed866b Compare September 25, 2019 11:58

rename to s3fs

db89859

kszucs force-pushed the s3 branch from eed866b to db89859 Compare September 25, 2019 12:08

remove commented tests

98bd91a

S3Options

73e6625

kszucs requested a review from pitrou September 27, 2019 13:04

pitrou reviewed Sep 30, 2019

View reviewed changes

ci/conda_env_python.yml Show resolved Hide resolved

kszucs commented Sep 30, 2019

View reviewed changes

python/pyarrow/tests/test_fs.py Show resolved Hide resolved

Resolve review comments

384c960

pitrou closed this in 155415c Oct 1, 2019

This was referenced Oct 1, 2019

[Python] Run s3fs unit tests in Travis CI #17476

Closed

[Python] Filesystem bindings for S3 #23006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-6655: [Python] Filesystem bindings for S3 #5423

ARROW-6655: [Python] Filesystem bindings for S3 #5423

kszucs commented Sep 18, 2019 •

edited

Loading

codecov-io commented Sep 18, 2019 •

edited

Loading

kszucs commented Sep 23, 2019

pitrou left a comment

kszucs Sep 24, 2019

pitrou Sep 24, 2019

kszucs Sep 25, 2019

pitrou Sep 30, 2019

kszucs commented Sep 25, 2019 •

edited

Loading

pitrou commented Sep 25, 2019

kszucs commented Sep 25, 2019 •

edited

Loading

pitrou commented Sep 26, 2019

kszucs commented Sep 26, 2019

pitrou left a comment

pitrou Sep 30, 2019

kszucs Sep 30, 2019

pitrou Sep 30, 2019

kszucs Sep 30, 2019

pitrou Sep 30, 2019

kszucs Sep 30, 2019

pitrou Sep 30, 2019

pitrou Sep 30, 2019

kszucs Sep 30, 2019

kszucs commented Sep 30, 2019

ARROW-6655: [Python] Filesystem bindings for S3 #5423

ARROW-6655: [Python] Filesystem bindings for S3 #5423

Conversation

kszucs commented Sep 18, 2019 • edited Loading

codecov-io commented Sep 18, 2019 • edited Loading

Codecov Report

kszucs commented Sep 23, 2019

pitrou 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

kszucs commented Sep 25, 2019 • edited Loading

pitrou commented Sep 25, 2019

kszucs commented Sep 25, 2019 • edited Loading

pitrou commented Sep 26, 2019

kszucs commented Sep 26, 2019

pitrou 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

kszucs commented Sep 30, 2019

kszucs commented Sep 18, 2019 •

edited

Loading

codecov-io commented Sep 18, 2019 •

edited

Loading

kszucs commented Sep 25, 2019 •

edited

Loading

kszucs commented Sep 25, 2019 •

edited

Loading