Upgrade WebAssembly build to Arrow 1.0.1 by texodus · Pull Request #1207 · perspective-dev/perspective

texodus · 2020-09-25T16:46:17Z

Upgrades Apache Arrow to v1.0.1 for all targets except Python 2.7. For the WebAssembly/Emscripten build, CSV loading and writing (via to_csv()) has been rewritten to use Arrow over C++, resulting in a ~10x load performance increase, depending on whether you provide a schema and what data types your columns are.

For Python 2.7, Arrow requirements are pyarrow<2 only, which is 0.16.1. I've done some limited testing to ensure this is cross-compatible with Arrow Table data generated or read-from 1.0.1, but this is something we should probably keep an eye on; especially regarding the JupyterLab plugin @finos/perspective-jupyterlab, whose client/server distributed mode would use engines compiled to both versions simultaneously when Python 2.7 runs the kernel.

timkpaine · 2020-09-25T17:22:24Z

cmake/modules/FindPyArrow.cmake

Don't hardcode the version, just use "arrow_python.dylib" etc

We are going to have to cope with the conditional naming for these libraries, as they are not disted in this version under this name (and don't seem to exhibit any consistent version convention at all).

Let's link this into an apache arrow issue, for pyarrow < 1 they had symlinks and we'll need those for some platforms

apache/arrow#7334

@wesm this makes linking against these libraries super ugly. Since the version is managed by python anyway (e.g. in the version of the python package), wouldn't it be better to ship just the unversioned binary (e.g. libarrow.dylib)?

See http://arrow.apache.org/docs/python/extending.html#building-extensions-against-pypi-wheels. We had to make changes to prevent shared libraries from being duplicated in the wheels. It might be possible to change things to ship unversioned shared libraries instead, but it will require some surgery on the Cython modules to get them to link with the unversioned libs (because they look for the libraries with the ABI version when they link). Feel free to open a JIRA issue in Apache Arrow with a proposal -- I have definitely hit the limit for how much time I can personally spend on it

Thanks @wesm

@timkpaine I am not particularly bothered by this, it is easy to fix and this file must be manually updated for new Arrow versions regardless.

cmake/modules/FindPyArrow.cmake

timkpaine · 2020-09-28T02:01:58Z

cpp/perspective/CMakeLists.txt

 		target_link_libraries(binding tbb)
-
 		target_link_libraries(binding psp)
+		target_compile_options(psp PRIVATE -Wno-deprecated-register)


what are these for?

Silences python2 deprecation warnings which make our build logs un-readable.

You're doing this on windows too though, are you sure this flag will work there?

I don't know ... our windows azure builds do not work ...

timkpaine · 2020-09-28T02:03:18Z

cpp/perspective/src/cpp/emscripten.cpp

 #include <perspective/emscripten.h>
 #include <perspective/arrow_loader.h>
 #include <perspective/arrow_writer.h>
+#include "arrow/csv/api.h"


match style with <>

timkpaine · 2020-09-28T02:04:49Z

python/perspective/setup.py

    requires.append("backports.shutil-which")
+    requires.append("pyarrow<2")
+else:
+    requires.append("pyarrow>=1.0.1,<2")


this should match pyproject.toml

This version is conditional on python version - does this not make the looser bounds "pyarrow<2" for pyproject.toml correct? Perspective under this PR will compile with either pyarrow.

pyproject.toml specifies it as pyarrow<2 regardless of python version. So it could satisfy with pyarrow<0.16 even though that won't work.

You have a couple of options, something like:
"pyarrow>0.16.0,<2; python_version < '3'", "pyarrow>1.0.1,<2; python_version >= '3'" would work ok.

Stylistically, every place where we specify requirements should match exactly the behavior of pyproject.toml, otherwise we can very easily start to break our builds and wheels like the state we were in before.

timkpaine · 2020-09-28T02:05:10Z

azure-pipelines.yml


 #     - script: |
-#         python -m pip install numpy "pyarrow>=0.16.0,<1"
+#         python -m pip install numpy "pyarrow>=1.0.1,<2"


match pyproject.toml

timkpaine · 2020-09-28T02:05:31Z

azure-pipelines.yml

    - script: |
-        python -m pip install delocate wheel numpy "pyarrow>=0.16.0,<1"
-      displayName: "Python deps"
+        python -m pip install delocate wheel numpy "pyarrow>=1.0.1,<2"


match pyproject.toml

timkpaine · 2020-09-28T02:06:49Z

@texodus the only issue really is that pyproject.toml has to match wherever you install pyarrow. So if you install <2 for python2, and >1<2 for python3, you need to reflect this in the pyproject.toml file so that the build system matches the setup.py requirements matches what we install in CI

texodus · 2020-10-04T22:10:49Z

@timkpaine I've moved the pyarrow upgrade to a new PR. What's left in the PR is just the C++ and Javascript changes necessary to upgrade, plus the new CSV parser. Since this was already compilable for ARROW_VERSION_MAJOR < 1, the PR continues to compile fine with perspective-python's pyarrow dependency bounds as they were.

timkpaine · 2020-10-04T22:30:15Z

@texodus do you want me to make the python changes on that branch or do you want them in a separate PR?

texodus · 2020-10-04T23:30:11Z

I already made most of them - just the pyproject.toml one is left. I will set you as a reviewer when it is ready to merge.

finos-cla-bot bot added the cla-present label Sep 25, 2020

texodus force-pushed the csv-arrow branch 8 times, most recently from 50d9237 to 229f764 Compare September 26, 2020 22:01

texodus marked this pull request as ready for review September 26, 2020 22:21

texodus requested a review from timkpaine September 26, 2020 22:21

timkpaine requested changes Sep 28, 2020

View reviewed changes

texodus mentioned this pull request Oct 4, 2020

Simplify python build #1118

Closed

texodus force-pushed the csv-arrow branch from 229f764 to 9159822 Compare October 4, 2020 22:00

Use arrow C++ for CSV parsing in JS

ec09c74

texodus force-pushed the csv-arrow branch from 9159822 to ec09c74 Compare October 4, 2020 22:04

texodus changed the title ~~Upgrade to Arrow 1.0.1~~ Upgrade WebAssembly build to Arrow 1.0.1 Oct 4, 2020

texodus merged commit 3926623 into master Oct 4, 2020

texodus deleted the csv-arrow branch October 5, 2020 02:21

texodus added enhancement Feature requests or improvements JS labels Oct 7, 2020

texodus mentioned this pull request Oct 7, 2020

Upgrade pyarrow to 1.0.1 #1215

Closed

Conversation

texodus commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

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!

timkpaine commented Sep 28, 2020

Uh oh!

texodus commented Oct 4, 2020

Uh oh!

timkpaine commented Oct 4, 2020

Uh oh!

texodus commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

texodus commented Sep 25, 2020 •

edited

Loading

texodus commented Oct 4, 2020 •

edited

Loading