Add Arrow - Velox conversion support #4450

sanjibansg · 2023-03-29T06:03:04Z

This PR introduces PyVelox functions for the conversion of Arrow Arrays to Velox Vectors and vice-versa.

facebook-github-bot · 2023-03-29T06:03:10Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

netlify · 2023-03-29T06:03:10Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`579010b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64820985835d670008c86fa4

facebook-github-bot · 2023-03-29T08:18:13Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

jorisvandenbossche · 2023-03-30T14:22:42Z

pyvelox/pyvelox.h

+      [](uintptr_t arrowArrayPtr, uintptr_t arrowSchemaPtr) {
+        auto arrowArray = reinterpret_cast<ArrowArray*>(arrowArrayPtr);
+        auto arrowSchema = reinterpret_cast<ArrowSchema*>(arrowSchemaPtr);


Currently, this function is written such that you pass the pointers as arguments, but alternatively, the function could accept a pyarrow.Array object, and then the pointers could be initialized here and the _export_to_c method could be called here? That way, the user doesn't need to call cffi themselves, and what now takes

c_schema = ffi.new("struct ArrowSchema*") schema_ptr = int(ffi.cast("uintptr_t", c_schema)) c_array = ffi.new("struct ArrowArray*") array_ptr = int(ffi.cast("uintptr_t", c_array)) arr = pa.array([1, 2, 3], type=pa.int32()) arr._export_to_c(array_ptr, schema_ptr) velox_vector = pv.import_from_arrow(array_ptr, schema_ptr)

could be reduced to

velox_vector = pv.import_from_arrow(arr)

?

Yes, that was my initial thought. But, I guess that would make PyArrow a dependency here, which I am not sure whether it will be a good approach.

It should be possible to make it only an optional dependency, I think. Like try to import it, and if not available, raise an informative error?
Long term, we should also make it possible to pass Arrow data without relying on pyarrow (using the C Data Interface, xref apache/arrow#34031). But short term I think assuming you get a pyarrow.Array is the easiest and most useful.

We can make it a proper dependency if thats the way to go. What would be concerns with making pyarrow a proper dependency for pyvelox ?

What would be concerns with making pyarrow a proper dependency for pyvelox ?

Just being an additional dependency that you need to have installed (and each additional dependency can give some installation problem), while it is not strictly needed for other functionality of PyVelox.
Now, since we won't require a development version of pyarrow, that's shouldn't give much problems.

Currently, the conversion function accepts a py::object and calls the method _export_to_c internally. Does this approach look good?

cc: @jorisvandenbossche @kgpai

@sanjibansg thanks for the update. Yes, that looks good to me! In addition, we could also do the same for the export_to_arrow function, so that the two methods are more each other counterparts?

assignUser · 2023-04-04T18:33:52Z

I rebased to fix the failing builds due to the arrow link change. The wheel workflow works now and make python-test should also install the extras_require.

kgpai

Thanks Sanjiban,

Some questions.

kgpai · 2023-04-04T19:14:52Z

Makefile

@@ -155,7 +155,8 @@ python-clean:
 	DEBUG=1 ${PYTHON_EXECUTABLE} setup.py clean

 python-build:
-	DEBUG=1 CMAKE_BUILD_PARALLEL_LEVEL=4 ${PYTHON_EXECUTABLE} setup.py develop
+	DEBUG=1 CMAKE_BUILD_PARALLEL_LEVEL=4 ${PYTHON_EXECUTABLE} -m pip install -e .$(extras)


curious why the pip install ?

So we can use the tests extras and also because it is the "correct" way to do it afaik

For another link, see the warning note here: https://setuptools.pypa.io/en/latest/deprecated/commands.html

pyvelox/pyvelox.h

kgpai · 2023-04-04T19:37:56Z

pyvelox/pyvelox.h

+    auto arrowArray = new ArrowArray();
+    std::shared_ptr<facebook::velox::memory::MemoryPool> pool_{
+        facebook::velox::memory::getDefaultMemoryPool()};
+    facebook::velox::exportToArrow(inputVector, *arrowArray, pool_.get());


Also looks like this isnt 0 copy , atleast for flat vectors it ought to be possible to have velox to arrow be 0 copy , right ?

atleast for flat vectors it ought to be possible to have velox to arrow be 0 copy , right ?

Yes, that should be fully zero-copy. It's only for those vector types that don't have an exact 1:1 mapping with arrow types that additional data might need to be allocated. (eg StringView)

pyvelox/pyvelox.h

kgpai · 2023-04-04T20:25:09Z

pyvelox/pyvelox.h

+      [](uintptr_t arrowArrayPtr, uintptr_t arrowSchemaPtr) {
+        auto arrowArray = reinterpret_cast<ArrowArray*>(arrowArrayPtr);
+        auto arrowSchema = reinterpret_cast<ArrowSchema*>(arrowSchemaPtr);


We can make it a proper dependency if thats the way to go. What would be concerns with making pyarrow a proper dependency for pyvelox ?

pyvelox/pyvelox.h

kgpai · 2023-04-04T20:28:25Z

pyvelox/test/test_vector.py

+
+    def test_export_to_arrow(self):
+        vector = pv.from_list([1, 2, 3])
+        vector_ptr = pv.export_to_arrow(vector)


Is it common to have c style names like _ptr etc ?

Are you asking about using the _ptr here, or the snake_case style used? For the _ptr usage, I can use better names, it was just to indicate that the integer value returned from the method actually refers to a memory address.

kgpai

Thanks Sanjiban. This looks very good. Some minor questions .

kgpai · 2023-05-03T05:25:46Z

pyvelox/conversion.cpp

+      [](py::object inputArrowArray) {
+        auto arrowArray = std::make_unique<ArrowArray>();
+        auto arrowSchema = std::make_unique<ArrowSchema>();
+        inputArrowArray.attr("_export_to_c")(reinterpret_cast<uintptr_t>(arrowArray.get()), reinterpret_cast<uintptr_t>(arrowSchema.get()));


Should there be some sort of validation that this is a valid arrow object and this attr is present , and otherwise a helpful error message ?

Longer term, I would like that we add some sort of python protocol (eg __arrow_c_data__()) in the Arrow ecosystem, so that any object that uses arrow-compatible memory can be consumed here.
But on the short term, checking that the input is indeed an pyarrow Array is indeed a good idea (just checking that the attribute is present, and if not raising an informative error, is probably sufficient)

kgpai · 2023-05-03T05:27:45Z

pyvelox/conversion.cpp

+        inputArrowArray.attr("_export_to_c")(reinterpret_cast<uintptr_t>(arrowArray.get()), reinterpret_cast<uintptr_t>(arrowSchema.get()));
+        std::shared_ptr<facebook::velox::memory::MemoryPool> pool_{
+            facebook::velox::memory::addDefaultLeafMemoryPool()};
+        return importFromArrowAsOwner(*arrowSchema, *arrowArray, pool_.get());


Curious, why not just use the default memory pool ?

Currently, I am using the memory pool from the static instance of the PyVeloxContext. I believe it correctly manages the memory, and cleans it after usage.

kgpai · 2023-05-03T05:33:32Z

pyvelox/test/test_vector.py

@@ -254,3 +255,35 @@ def test_append(self):

        with self.assertRaises(TypeError):
            ints2.append(strs2)
+
+    def test_export_to_arrow(self):


Thanks this is great, can we add export / import tests for all the types we support currently though ?

It's certainly good to add some more tests, but just to mention that the actual conversion itself (the C++ code) is also already tested at https://github.com/facebookincubator/velox/blob/main/velox/vector/arrow/tests/ArrowBridgeArrayTest.cpp

Thanks @jorisvandenbossche , nevertheless would be good to add tests for atleast the primitive types to make sure there are no inadvertent casts across types , since its going via Cpp to Python etc. Also PyVelox doesnt support all the types thats Velox supports yet :) .

I have added the test cases for integers, floats, and strings. Should I add some other test cases as well?

Also, while testing multiple test cases, I noticed, that the memory pool instance was not handled correctly earlier. So, I had to move the Instance struct to a separate header file, does this approach look good, or can there be a better way?

Yes, I think the latest vectorsaver changes also moved it to a different file. Please merge from latest.

pyvelox/conversion.cpp

kgpai

Looks good @sanjibansg . Can you rebase against latest main and It should be good to merge after that.

kgpai · 2023-05-30T15:59:30Z

pyvelox/test/test_vector.py

@@ -254,3 +255,35 @@ def test_append(self):

        with self.assertRaises(TypeError):
            ints2.append(strs2)
+
+    def test_export_to_arrow(self):


Yes, I think the latest vectorsaver changes also moved it to a different file. Please merge from latest.

sanjibansg · 2023-05-31T07:24:41Z

Looks good @sanjibansg . Can you rebase against latest main and It should be good to merge after that.

Rebased it to main, and did the format-check. Thanks!

kgpai · 2023-06-01T16:52:26Z

@sanjibansg Can you look into the failing PyVelox build ?

…ot memory address

… header

facebook-github-bot · 2023-06-08T19:47:48Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-06-14T12:31:35Z

@kgpai merged this pull request in 8e95993.

conbench-facebook · 2023-06-14T12:52:51Z

Conbench analyzed the 1 benchmark run on commit 8e959935.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

sighingnow · 2023-07-14T12:28:55Z

Hi, I would like to know when will the package on pypi https://pypi.org/project/pyvelox/ to be updated to include this important change?

Thanks!

kgpai · 2023-07-14T17:18:44Z

@sighingnow I have a job here to publish the new python package : https://github.com/facebookincubator/velox/actions/runs/5554843861 .

kgpai · 2023-07-14T18:14:07Z

The packages are published @sighingnow , you can get it from pypi.

sighingnow · 2023-07-15T04:32:55Z

The packages are published @sighingnow , you can get it from pypi.

Thank you!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 29, 2023

pedroerp requested a review from kgpai March 29, 2023 16:20

jorisvandenbossche reviewed Mar 30, 2023

View reviewed changes

assignUser force-pushed the arrow-velox-conversion branch from b695e6e to 9f88745 Compare April 4, 2023 18:32

kgpai reviewed Apr 4, 2023

View reviewed changes

sanjibansg force-pushed the arrow-velox-conversion branch from 4772c39 to e8720e6 Compare April 25, 2023 07:31

sanjibansg requested a review from kgpai May 2, 2023 04:10

sanjibansg force-pushed the arrow-velox-conversion branch from e8720e6 to 3ffd670 Compare May 2, 2023 05:12

sanjibansg requested a review from jorisvandenbossche May 2, 2023 05:13

kgpai reviewed May 3, 2023

View reviewed changes

jorisvandenbossche reviewed May 3, 2023

View reviewed changes

pyvelox/conversion.cpp Outdated Show resolved Hide resolved

sanjibansg requested review from jorisvandenbossche and kgpai May 16, 2023 10:08

kgpai approved these changes May 30, 2023

View reviewed changes

sanjibansg force-pushed the arrow-velox-conversion branch 2 times, most recently from 259598f to 70d85f9 Compare May 31, 2023 07:20

sanjibansg requested a review from kgpai May 31, 2023 07:23

sanjibansg and others added 7 commits June 3, 2023 08:08

feat: Support for Arrow-Velox conversion

b66d17f

fix: passing default memory pool while converting

d52ecfa

format

6c4b5ad

use extras_require for test deps

d93409d

feat: separate module for the conversion functions

2aa27b0

feat: import_from_arrow now can work only with the pyarrow.Array object

44271a2

feat: add roundtrip testing for conversion functions

3ff09cd

sanjibansg added 7 commits June 3, 2023 08:08

feat: using smart pointers for conversion memory movement

1931791

feat: pv.export_to_arrow() should return a pyarrow.array object and n…

5f42b1e

…ot memory address

fix: moved MemoryPool instance to util header, updated tests

703d846

fix: format-check

ea6c049

fix: formatting on CMakeLists.txt

633fd03

fix: util header not required as instance struct was moved to context…

f3108aa

… header

fix: using static method getSingletonInstance()

87e5f2b

sanjibansg force-pushed the arrow-velox-conversion branch from b4cd95a to 87e5f2b Compare June 3, 2023 02:38

sanjibansg added 2 commits June 3, 2023 08:09

fix: format-check

072244e

fix: python build should have verbose output for passing CI checks

579010b

kgpai approved these changes Jun 8, 2023

View reviewed changes

facebook-github-bot closed this in 8e95993 Jun 14, 2023

facebook-github-bot added the Merged label Jun 14, 2023

sanjibansg deleted the arrow-velox-conversion branch July 14, 2023 18:18

Add Arrow - Velox conversion support #4450

Add Arrow - Velox conversion support #4450

Conversation

sanjibansg commented Mar 29, 2023

facebook-github-bot commented Mar 29, 2023

Action Required

Process

netlify bot commented Mar 29, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Mar 29, 2023

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

sanjibansg Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser commented Apr 4, 2023 • edited Loading

kgpai 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

kgpai 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

kgpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjibansg commented May 31, 2023

kgpai commented Jun 1, 2023

facebook-github-bot commented Jun 8, 2023

facebook-github-bot commented Jun 14, 2023

conbench-facebook bot commented Jun 14, 2023

sighingnow commented Jul 14, 2023

kgpai commented Jul 14, 2023

kgpai commented Jul 14, 2023

sighingnow commented Jul 15, 2023

netlify bot commented Mar 29, 2023 •

edited

Loading

sanjibansg Apr 25, 2023 •

edited

Loading

assignUser commented Apr 4, 2023 •

edited

Loading