Statically link `perspective-python` and `libarrow` #1290

sc1f · 2021-01-12T23:17:45Z

This PR builds Apache Arrow as a static library and reconfigures perspective-python to no longer link against a version of PyArrow present on the system/installed as part of perspective-python dependencies.

Instead, Perspective's C++ library will link against the pre-compiled, minimal arrow static library, and the user is free to use any version of PyArrow they choose to install as long as the Arrow binary format is compatible with the binaries output by Perspective's version of Arrow, which is v1.0.1 at this time.

At present, Perspective does not import pyarrow or use any of Arrow's Python-specific C++ code. If we were to use PyArrow in the Python runtime, however (like in the test suite), Perspective's C++ version of Arrow will continue to work as expected—perspective-python will use its own version of Arrow, and pyarrow will use the libarrow.so that shipped with the PyArrow install. This means further integration with PyArrow is not blocked/degraded, and if anything should be accelerated as we now have a consistent Arrow version across all Perspective runtimes.

In effect, we have taken a "pinned" version of PyArrow that used to depend on external assets we could not fully/consistently control, and turned it into a pinned version of PyArrow that we have full management and control over.

Benefits

With this change, the complicated and admittedly brittle code we have in place to deal with detecting PyArrow's install location, managing PyArrow versions, copying Arrow DLLs (on Windows), etc. can be removed, as Perspective's build no longer depends on the existence and correct versioning of an asset we have little control over.

Additionally, users can install the newest version of PyArrow in order to gain access to features/fixes without worrying about Perspective compatibility beyond the binary format, which is expected to remain stable and compatible between versions. Finally, the developer experience should be improved for both source builds and configuring the conda-forge build, which can unpin its PyArrow dependency.

Testing

Perspective's WASM build has built and used the minimal version of Arrow since late 2019, and perspective-python's test suite contains a litany of tests that utilize PyArrow to generate/ingest arrow binaries to and from Perspective. This PR updates the Manylinux Dockerfiles and Python test suite to use PyArrow v2.0.0 (except for Python 2, which continues to use v0.16.0). This in effect gives us full coverage over forwards and backwards compatibility between our static Arrow pinned to v.1.0.1 and the versions of PyArrow used by the tests.

TODO:

Remove PyArrow maintenance code/requirements from setup.py, pyproject.toml etc.
Test that Perspective works with the latest version of PyArrow, and update the test suite to use latest Arrow
Remove PyArrow custom installs from all Dockerfiles, and regenerate Docker images
Clean up CMake code around PyArrow

cmake/arrow/CMakeLists.txt

WIP windows flatbuffers Install flatbuffers using vcpkg link against rt for python2 manylinux use vcpkg toolchain file WIP why not try chocolatey

pull in flatbuffers as an external dep wip fix windows build nocopy arrow dlls

sc1f · 2021-01-15T21:54:02Z

@texodus this PR now has the changes from #1215 in it, so once this is ready to merge we can close #1215.

…to install PyArrow 2

sc1f · 2021-01-15T23:12:13Z

@texodus tests now run against PyArrow 2.0.0, checked locally and on Azure Mac/Windows jobs. Just need to regen the docker images and should be all clear.

@timkpaine would you be able to pull down this branch and take a look if you have time? I don't have a Windows machine to verify, but the build should work as long as you have flatbuffers installed before build.

timkpaine · 2021-01-15T23:56:20Z

Need to check that an arrow passed in from python will be binary compatible with the version we built against, so expose the arrow api version into python and check against whatever attribute is there on the python side, otherwise it will likely just seg fault when the arrow versions are incompatible

right now if arrow versions are incompatible, you get the error message that disables the c++ code, so something similar but just for arrow

texodus · 2021-01-17T12:55:31Z

@timkpaine Thanks for the feedback! I don't think this is necessarily an issue, since

None of the logic of how we use pyarrow internally has changed at all, and this PR does not modify any .py files.
The arrow::Table load logic used in C++ does version validation (though these don't yet work well in js, see Async Table and View Constructor #1289).
The format is backwards-compatible (?) from 0.16
Adding pyarrow to check the version from the Python-side, re-introduces the exact dependency on pyarrow that this PR seeks to remove.

I dug up a very old superstore.arrow from Perspective 0.1.0 on unpkg here, and it throws an exception with a not super helpful error message but does not segfault:

Failed to open RecordBatchStreamReader: Invalid: Expected to read 1329865020 metadata bytes, but only read 36639

I concur this needs work, lets aim to improve the quality of our error reporting in a PR which targets this specifically.

texodus · 2021-01-17T13:01:27Z

cpp/perspective/CMakeLists.txt

+# Chocolatey for our Azure Windows job.
+if (WIN32)
+	psp_build_dep("flatbuffers" "${PSP_CMAKE_MODULE_PATH}/flatbuffers.txt.in")
+endif()


Why do the flatbuffers headers need to be available only on Windows?

texodus · 2021-01-17T13:02:23Z

docs/md/development.md

-you have installed. To disable this, pass the `--no-build-isolation` flag to
-pip.
-
+- Flatbuffers not installed prior to installing Perspective


Need a newline, markdown is picky sometimes

texodus · 2021-01-17T13:03:55Z

Looks good! Thanks for the PR!

timkpaine · 2021-01-17T22:21:25Z

I dug up a very old superstore.arrow from Perspective 0.1.0 on unpkg here, and it throws an exception with a not super helpful error message but does not segfault:

Failed to open RecordBatchStreamReader: Invalid: Expected to read 1329865020 metadata bytes, but only read 36639

I concur this needs work, lets aim to improve the quality of our error reporting in a PR which targets this specifically.

So long as it fails in python and not in c++ (e.g. no segfault, user can handle in python) its "fine".

Adding pyarrow to check the version from the Python-side, re-introduces the exact dependency on pyarrow that this PR seeks to remove.

Note we won't need to re-add libarrow_python, we just need to expose the version of arrow that perspective statically linked against into python and we can do the check purely in python at load time. If you're passing in a pyarrow.Table (e.g. pyarrow in sys.modules) we can do a version check. Right now we treat any bytes as arrow which should probably have these types of checks anyway. I assume the version is encoded in the table binary in some form as well, so we can even use that

sc1f · 2021-01-17T23:00:36Z

@sc1f this also seems to break Mac or linux when lib binding looks for libpsp

I think I might have removed the $ORIGIN from the rpath setting when I was cutting out the old pyarrow - will take a look and fix.

timkpaine · 2021-01-17T23:44:43Z

I've fixed on my jlab 3 PR

texodus · 2021-01-18T05:51:44Z

@timkpaine pyarrow is not imported by perspective-python, and perspective.Table() doesn't support pyarrow.Table as a constructor argument (unless there is some Python coercion coincidence I am unaware of ..). As I stated above, arrow::Table does validation on blobs, as it should since we don't even know what ranges to validate without reading the Arrow docs like savages.

Just to re-emphasize - this PR changes no python, it only removes the dependency shared libarrow which came packaged with pyarrow (e.g. the non-Python part). Or as stated in the 3rd paragraph of the PR:

At present, Perspective does not import pyarrow or use any of Arrow's Python-specific C++ code. If we were to use PyArrow in the Python runtime, however (like in the test suite), Perspective's C++ version of Arrow will continue to work as expected—perspective-python will use its own version of Arrow, and pyarrow will use the libarrow.so that shipped with the PyArrow install. This means further integration with PyArrow is not blocked/degraded, and if anything should be accelerated as we now have a consistent Arrow version across all Perspective runtimes.

timkpaine · 2021-01-18T06:51:16Z

So long as the arrow validation doesn't trigger un catchable errors in python, it should be a straight improvement.

If you're shoving an arrow into perspective in python, you're probably using pyarrow. Right now we go pyarrow table -> byte string copy -> perspective (2 copies total). We can no longer do pyarrow table -> no copy arrow table -> perspective (1 copy total), but I think it's ok given the better install experience (and we never implemented it anyway).

So the only negative is that if you're shoving an arrow from your version of pyarrow into our version of arrow and it's incompatible, you get an obscure error. We should make it a PerspectiveCPPException or whatever we called it.

texodus · 2021-01-18T21:17:09Z

#1157

timkpaine · 2021-01-18T21:19:43Z

you mean this comment? #1157 (comment)

sc1f · 2021-01-19T13:59:48Z

So the only negative is that if you're shoving an arrow from your version of pyarrow into our version of arrow and it's incompatible, you get an obscure error. We should make it a PerspectiveCPPException or whatever we called it.

IIRC it should be a PerspectiveCppException already, although I agree the "Expected to read..." error is a little obscure. We have more detailed error messages when we check the status of our arrow reads during the load, so it reports more verbose messages to tell you which columns failed, whether it could read the recordbatch, etc. It seems like this "Expected to read..." error is coming from the Arrow C++ lib itself, so it would be harder to transform that message.

I saw the fix for rpath on #1294 - I definitely was heavy-handed in removing PyArrow-related code from our CMakeLists, and cut out the "set rpath to $ORIGIN" block as well. Thanks for the fix!

timkpaine · 2021-01-19T14:03:36Z

Yep because we expose PerspectiveCppException into python you can try: except it, whereas an error out of Arrow C++ is not going to be exceptable. Worse case we just try:catch in C++ and in the catch handler bubble it up into python

sc1f · 2021-01-19T14:29:37Z

My recollection of how we set up PyBind is that all exceptions from C++ get raised as PerspectiveCppException in Python - let me get the old version of superstore.arrow from @texodus. I tried the oldest version I could find from May 2019 and it worked fine.

This reverts commit 5032199.

finos-cla-bot bot added the cla-present label Jan 12, 2021

sc1f added breaking internal Internal refactoring and code quality improvement Python and removed cla-present labels Jan 12, 2021

sc1f self-assigned this Jan 12, 2021

texodus changed the title ~~Unlink PyArrow from perspective-python~~ Statically link perspective-python and libarrow Jan 13, 2021

texodus reviewed Jan 13, 2021

View reviewed changes

cmake/arrow/CMakeLists.txt Show resolved Hide resolved

sc1f marked this pull request as draft January 13, 2021 19:20

finos-cla-bot bot added the cla-present label Jan 14, 2021

sc1f force-pushed the unlink-pyarrow branch 5 times, most recently from 74e74b8 to 938421e Compare January 15, 2021 16:44

sc1f removed the breaking label Jan 15, 2021

sc1f added 6 commits January 15, 2021 16:04

Pin date.h to v3.0.0

a2841d4

Build minimal arrow for python

0d576f5

install flatbuffers from brew

10c3a25

Install flatbuffers on Windows using Chocolatey

7d0aeed

WIP windows flatbuffers Install flatbuffers using vcpkg link against rt for python2 manylinux use vcpkg toolchain file WIP why not try chocolatey

Pull in Flatbuffers headers on Windows

067f01c

pull in flatbuffers as an external dep wip fix windows build nocopy arrow dlls

Clean up and document CMakeLists

871a289

sc1f force-pushed the unlink-pyarrow branch from 9eb86d0 to 6d66634 Compare January 15, 2021 21:39

sc1f requested a review from timkpaine January 15, 2021 21:40

Upgrade pyarrow requirements, remove FindPyArrow, update dockerfiles …

2ee1646

…to install PyArrow 2

sc1f force-pushed the unlink-pyarrow branch from 6d66634 to 2ee1646 Compare January 15, 2021 22:11

sc1f marked this pull request as ready for review January 15, 2021 23:10

texodus reviewed Jan 17, 2021

View reviewed changes

texodus merged commit 1725f48 into master Jan 17, 2021

texodus deleted the unlink-pyarrow branch January 17, 2021 13:04

texodus mentioned this pull request Jan 17, 2021

Upgrade pyarrow to 1.0.1 #1215

Closed

timkpaine added a commit that referenced this pull request Jan 17, 2021

Add arrow renderer, fix some problems from #1290

7f25bbe

zepaz pushed a commit to zepaz/perspective that referenced this pull request Jan 22, 2021

Add arrow renderer, fix some problems from finos#1290

5032199

zepaz added a commit to zepaz/perspective that referenced this pull request Jan 22, 2021

Revert "Add arrow renderer, fix some problems from finos#1290"

c2afc22

This reverts commit 5032199.

zepaz added a commit to zepaz/perspective that referenced this pull request Jan 22, 2021

Revert "Add arrow renderer, fix some problems from finos#1290"

6414edc

This reverts commit 5032199.

timkpaine mentioned this pull request Feb 26, 2021

perspective v0.6.2 conda-forge/perspective-feedstock#18

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Statically link `perspective-python` and `libarrow` #1290

Statically link `perspective-python` and `libarrow` #1290

sc1f commented Jan 12, 2021 •

edited

Loading

sc1f commented Jan 15, 2021

sc1f commented Jan 15, 2021

timkpaine commented Jan 15, 2021 •

edited

Loading

texodus commented Jan 17, 2021

texodus Jan 17, 2021

texodus Jan 17, 2021

texodus commented Jan 17, 2021

timkpaine commented Jan 17, 2021

sc1f commented Jan 17, 2021

timkpaine commented Jan 17, 2021

texodus commented Jan 18, 2021

timkpaine commented Jan 18, 2021

texodus commented Jan 18, 2021

timkpaine commented Jan 18, 2021

sc1f commented Jan 19, 2021

timkpaine commented Jan 19, 2021

sc1f commented Jan 19, 2021

Statically link perspective-python and libarrow #1290

Statically link perspective-python and libarrow #1290

Conversation

sc1f commented Jan 12, 2021 • edited Loading

Benefits

Testing

TODO:

sc1f commented Jan 15, 2021

sc1f commented Jan 15, 2021

timkpaine commented Jan 15, 2021 • edited Loading

texodus commented Jan 17, 2021

texodus Jan 17, 2021

Choose a reason for hiding this comment

texodus Jan 17, 2021

Choose a reason for hiding this comment

texodus commented Jan 17, 2021

timkpaine commented Jan 17, 2021

sc1f commented Jan 17, 2021

timkpaine commented Jan 17, 2021

texodus commented Jan 18, 2021

timkpaine commented Jan 18, 2021

texodus commented Jan 18, 2021

timkpaine commented Jan 18, 2021

sc1f commented Jan 19, 2021

timkpaine commented Jan 19, 2021

sc1f commented Jan 19, 2021

Statically link `perspective-python` and `libarrow` #1290

Statically link `perspective-python` and `libarrow` #1290

sc1f commented Jan 12, 2021 •

edited

Loading

timkpaine commented Jan 15, 2021 •

edited

Loading