ARROW-3162: Flight Python bindings #3566

ghost · 2019-02-05T18:31:53Z

Add docs
Format code
Include Python in integration tests (requires binding the JSON reader/writer from C++)
Validate performance?
Complete server bindings if approach makes sense

cpp/src/arrow/python/CMakeLists.txt

python/thirdparty

wesm · 2019-02-06T01:00:48Z

@lihalite glad to see you pushing this along. I think there's a JIRA already for Flight+Python so you can assign yourself that issue.

I'll defer to @pitrou on this code review -- if I can be of assistance please ping me and I will make time to review

codecov-io · 2019-02-06T15:11:03Z

Codecov Report

Merging #3566 into master will increase coverage by 0.85%.
The diff coverage is 41.17%.

@@            Coverage Diff             @@
##           master    #3566      +/-   ##
==========================================
+ Coverage   87.82%   88.67%   +0.85%     
==========================================
  Files         666      537     -129     
  Lines       82371    73408    -8963     
  Branches     1069        0    -1069     
==========================================
- Hits        72341    65098    -7243     
+ Misses       9919     8310    -1609     
+ Partials      111        0     -111

Impacted Files	Coverage Δ
python/pyarrow/includes/common.pxd	`75% <ø> (ø)`	⬆️
python/pyarrow/lib.pxd	`0% <ø> (ø)`	⬆️
python/pyarrow/ipc.pxi	`66.15% <41.17%> (-1.39%)`	⬇️
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
js/src/enum.ts
go/arrow/array/builder.go
js/src/Arrow.node.ts
js/src/schema.ts
go/arrow/type_traits_boolean.go
... and 123 more

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 4004b72...1977c45. Read the comment docs.

pitrou · 2019-02-07T16:10:04Z

Sorry for the delay. I have a flu, but will try to review this shortly.

pitrou

I see this PR is still work in progress, so consider this a preliminary review. Thanks for doing this :-)

pitrou · 2019-02-07T16:11:43Z

python/pyarrow/ipc.pxi

Why are you defining those two different classes _CRecordBatchReader and _RecordBatchReader?

I will do some more refactoring here, but I'd like to have other bindings of arrow::RecordBatchReader that share the same Python interface. The existing bindings are fairly tied to binding RecordBatchStreamReader even when they don't need to be. Properly, _CRecordBatchReader should be AbstractRecordBatchReader or something similar and _RecordBatchReader should be _RecordBatchStreamReader, if that sounds OK.

pitrou · 2019-02-07T16:14:35Z

python/pyarrow/ipc.pxi

Why are you defining those two different classes _CRecordBatchWriter and _RecordBatchWriter?

python/pyarrow/_flight.pxd

cpp/src/arrow/python/flight.h

python/pyarrow/_flight.pyx

pitrou · 2019-02-07T17:01:57Z

python/pyarrow/_flight.pyx

Should certainly run without the GIL, no?

Ah good point. One thing I also want to circle back to is this breaks Ctrl-C on the Python side currently.

python/pyarrow/_flight.pyx

ghost · 2019-02-07T17:25:27Z

Sorry for the delay. I have a flu, but will try to review this shortly.

No worries, hope you feel better soon, and thank you for the comments!

xhochy · 2019-02-08T21:29:06Z

@lihalite Can you rebase on master or would you like help with that?

ghost · 2019-02-08T21:30:29Z

@xhochy I can, will do so in a moment (sorry, didn't notice what just went in)

wesm · 2019-02-11T22:41:50Z

I had to squash and rebase due to the CMake file reformatting. I will review this also -- rather than try to boil the ocean in a single PR I suggest fixing any obvious problems with this patch and then opening a bunch of JIRAs so that the remaining work can proceed in parallel

ghost · 2019-02-11T22:44:05Z

Sounds good, thanks. The one thing I want to fix is the _CRecordBatchReader situation, then I'll open JIRAs for integration testing/completing the server side bindings. (I've been busy with other work but hopefully can pick this back up soon.)

ghost · 2019-02-12T20:51:39Z

Okay, if the changes in ipc.pxi around _CRecordBatchReader and _CRecordBatchWriter make sense, this is ready; I'll follow up with the rest soon.

Add simple demo Flight client in Python Add basic docstrings for Python Flight client Move RecordBatchReader/Writer wrappers to lib.pxd Link to PROTOBUF_LIBRARY, not protobuf_static Update for FlightPutWriter Start binding Flight server to Python Allow Flight DoPut to be implemented in Python Revert unnecessary build process changes Fix failing pyarrow unit tests Lint Python flight code Lint Cython code Delete unnecessary Flight pxd Use Arrow utilities for interfacing with Python in Flight Simplify API of Python Flight bindings Add Python enum to mirror DescriptorType in Flight Replace static factory methods with constructors

ghost · 2019-02-13T15:50:05Z

Rebased to fix conflict.

pitrou

Looks mostly fine to me. Are you going to submit tests in a separate PR?

pitrou · 2019-02-13T16:07:36Z

python/pyarrow/_flight.pyx

+    result = (<object> self).get_flight_info(descriptor)
+    if not result:
+        info.reset(NULL)
+        # TODO:


Is there something missing here and below? I suppose you should convert result to a CFlightInfo?

I haven't finished binding the server methods yet - I'll change this to raise NotImplementedError.

pitrou · 2019-02-13T16:11:41Z

python/setup.py

    'six >= 1.0.0',
-    'futures; python_version < "3.2"'
+    'futures; python_version < "3.2"',
+    'enum34 >= 1.1.6; python_version < "3.4"',


You must also update requirements.txt.

wesm · 2019-02-14T17:34:08Z

Thanks @lihalite! Seems like there are a number of follow ups, are there JIRA issues for these yet?

ghost · 2019-02-14T17:52:06Z

I will follow up those now @wesm. Thanks everyone for the help here!

ghost · 2019-02-14T18:01:35Z

Looks like some have already been filed, here are the relevant issues:

https://issues.apache.org/jira/browse/ARROW-3150 (ship wheels)
https://issues.apache.org/jira/browse/ARROW-3162 (complete server side)
https://issues.apache.org/jira/browse/ARROW-4573 (unit tests)
https://issues.apache.org/jira/browse/ARROW-4575 (integration tests)

ghost commented Feb 5, 2019

View reviewed changes

cpp/src/arrow/python/CMakeLists.txt Outdated Show resolved Hide resolved

ghost commented Feb 5, 2019

View reviewed changes

python/thirdparty Outdated Show resolved Hide resolved

ghost changed the title ~~[WIP] Flight Python bindings~~ [WIP] ARROW-3162: Flight Python bindings Feb 6, 2019

pitrou reviewed Feb 7, 2019

View reviewed changes

xhochy force-pushed the master branch from 29f76df to 2b9155a Compare February 8, 2019 21:00

David Li and others added 4 commits February 13, 2019 10:48

Reformat cmake

7764444

Lint CMake files

e1c298a

Clarify various RecordBatchStream{Reader,Writer} wrappers

9d5442a

pitrou changed the title ~~[WIP] ARROW-3162: Flight Python bindings~~ ARROW-3162: Flight Python bindings Feb 13, 2019

pitrou reviewed Feb 13, 2019

View reviewed changes

Clean up to-be-implemented parts of Flight Python bindings

ac29ab8

pitrou closed this in 148213e Feb 14, 2019

asfimport mentioned this pull request Feb 26, 2019

[Python] Enable Flight servers to be implemented in pure Python #19510

Closed

Uh oh!

ARROW-3162: Flight Python bindings #3566

ARROW-3162: Flight Python bindings #3566

Uh oh!

Conversation

ghost commented Feb 5, 2019

Uh oh!

Uh oh!

Uh oh!

wesm commented Feb 6, 2019

Uh oh!

codecov-io commented Feb 6, 2019

Codecov Report

Uh oh!

pitrou commented Feb 7, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Feb 7, 2019

Uh oh!

xhochy commented Feb 8, 2019

Uh oh!

ghost commented Feb 8, 2019

Uh oh!

wesm commented Feb 11, 2019

Uh oh!

ghost commented Feb 11, 2019

Uh oh!

ghost commented Feb 12, 2019

Uh oh!

ghost commented Feb 13, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

ghost commented Feb 14, 2019

Uh oh!

ghost commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants