ARROW-4372: [C++] Embed precompiled bitcode in the gandiva library #3484

kszucs · 2019-01-25T11:56:58Z

We were not running the pyarrow tests after installing the manylinux wheels, which can lead to uncaught issues, like: https://travis-ci.org/kszucs/crossbow/builds/484284104

kszucs · 2019-01-28T15:13:09Z

@xhochy We're not bundling gandiva generated bytecode files. I'm trying to resolve it, but You might know the solution.

kszucs · 2019-01-28T16:41:48Z

I've managed to bundle irhelpers.bc as gandiva_irhelpers.bc in the python package, but We still need configure the engine accordingly, bot only for wheels.

xhochy · 2019-01-28T18:07:57Z

but We still need configure the engine accordingly

What do you mean with that?

wesm · 2019-01-28T18:18:14Z

I think the bitcode path is still being hard-coded, is that right?

kszucs · 2019-01-28T21:02:25Z

@xhochy according to this commit @kou made it configurable.

kou · 2019-01-28T22:05:21Z

The change just for creating a configuration for custom path. This is enough just for testing. But it's not suitable for normal use.
We can't change the default path yet. I think that we need a feature to customize the default path dynamically or finding the byte code path from the loaded shared library path or something.

kszucs · 2019-01-29T17:34:15Z

.travis.yml

This will reduce the build times. There are nightly tests for all versions.

@pitrou This will resolve the random timeouts of wheel builds.

kszucs · 2019-01-29T19:28:18Z

@kou could You elaborate on customizing the default path dynamically?

If I understand correctly the second option is to maintain a list of default search paths relative to the gandiva binary (queried via dladdr or boost::dll::program_location())?;

kou · 2019-01-29T21:50:19Z

could You elaborate on customizing the default path dynamically?

The easiest way is using an environment variable:

diff --git a/cpp/src/gandiva/configuration.h b/cpp/src/gandiva/configuration.h
index 04e2eed2..fd948400 100644
--- a/cpp/src/gandiva/configuration.h
+++ b/cpp/src/gandiva/configuration.h
@@ -75,7 +75,14 @@ class ConfigurationBuilder {
   std::string byte_code_file_path_;
 
   static std::shared_ptr<Configuration> InitDefaultConfig() {
-    std::shared_ptr<Configuration> configuration(new Configuration(kByteCodeFilePath));
+    std::string path;
+    auto path_env = std::getenv("GANDIVA_BYTE_CODE_PATH");
+    if (path_env) {
+      path = path_env;
+    } else {
+      path = kByteCodeFilePath;
+    }
+    std::shared_ptr<Configuration> configuration(new Configuration(path));
     return configuration;
   }

But it may have a security problem.

kou · 2019-01-29T22:09:17Z

If I understand correctly the second option is to maintain a list of default search paths relative to the gandiva binary (queried via dladdr or boost::dll::program_location())?

Right. But it may be difficult. Because Gandiva may be built as a static library. (We may able to assume that Gandiva is built as a shared library in wheel.)

This approach is popular on Windows. Many products search their configuration file by relative path from their DLL.
But it's not popular on Linux.

kszucs · 2019-01-29T22:54:51Z

@wesm any thoughts on this?

pitrou · 2019-01-30T17:07:03Z

ci/travis_script_manylinux.sh

Aren't we crashy when Tensorflow is loaded in the same process?

I guess this is supposed to test that.

We should not crash, there is some special safeaguarding code for that.

But it doesn't always work, see https://issues.apache.org/jira/browse/ARROW-4272

So shouldn't we remove this import?

Why should We? If We want to test that both arrow and tensorflow can live in the same interperter.

As you prefer... but it will probably fail at one point or the other, because Tensorflow publishes rogue wheels.

python/setup.py

kszucs · 2019-01-31T11:46:12Z

@pravindra couldn't We embed the bitcode into the library?

pravindra · 2019-01-31T12:38:45Z

@pravindra couldn't We embed the bitcode into the library?

sorry, I didn't understand. can you please elaborate some more ? The bit-code file is generated by a series of clang tool commands during the build process. Is there a way to add such a file into the .so or .a, and extract it later at runtime ?

pitrou · 2019-01-31T12:43:17Z

The easiest and most portable way (though slightly clunky) would probably to generate a C++ file containing static const char arrays of the embedded bitcodes...

See https://stackoverflow.com/questions/11813271/embed-resources-eg-shader-code-images-into-executable-library-with-cmake for an example, though we'd probably want to write the generator utility in Python.

kszucs · 2019-01-31T13:19:21Z

@pravindra yes, like Antoine's example. Although it might be easier to just configure the right path from the python module (similarly like from java). Not sure which one would be cleaner...

kszucs · 2019-01-31T13:20:12Z

But We still need to keep the ability to load bitcode from files, right?

pitrou · 2019-01-31T13:22:00Z

Apparently the bitcode path is hardcoded as an absolute path currently (see the generated bc_file_path.cc), so the approach would have to be fixed anyway:

namespace gandiva {

// Path to the byte-code file.
extern const char kByteCodeFilePath[] = "/home/antoine/miniconda3/envs/pyarrow/lib/gandiva/irhelpers.bc";

} // namespace gandiva

kszucs · 2019-01-31T13:33:23Z

So We have a couple of options:

don't set default path at all, and let the binding (java, python) configure it
set default path relative to the shared lib (is it fragile in case of static libs?)
compile the bitcode as a string into the lib

which one should We go with?

pitrou · 2019-01-31T13:46:47Z

I think compiling the bitcode as a string is the most pain-free approach. Other approaches have to deal with locating the files on disk in different environments (Python, Java, ...).

kszucs · 2019-01-31T13:56:32Z

Any downstream libraries would like to provide precompiled gandiva expressions should do the same?

pitrou · 2019-01-31T13:57:31Z

I think that's a separate concern. It would probably be possible to let them choose their preferred way.

kszucs · 2019-01-31T14:00:04Z

Ok, I'll go with the string approach then. Thanks @pitrou !

pravindra · 2019-01-31T14:06:04Z

I do see the advantages of the embed-as-string approach.

Early on, we had plans to support pluggable function registries in gandiva I.e given a new bitcode file and the fn signatures, it would be possible to add new functions to gandiva by doing a module reload (without actually changing arrow/gandiva code). so, passing the location(s) of the bitcode file(s) as a config param made sense.

We didn’t get around to implementing it properly, and I think it’s harder to do now since there are cross references from the bitcode files into gandiva/arrow (for date/decimal).

Embedding the bc code into the library will mean that we cannot go down that path anymore.

wesm · 2019-02-09T21:29:07Z

Where do things stand on this?

kszucs · 2019-02-12T10:36:22Z

I can continue to work on this tomorrow, right now I'm busy with buildbot's infra.

kszucs · 2019-02-15T12:22:49Z

cpp/src/gandiva/projector_cache_key.h

I'm not sure that We want to hash the precompiled bitcode, so I've disabled it.

maybe, keep this Hash(), but dummy out the configuration->Hash() (just return 0) ?

kszucs · 2019-02-15T12:23:28Z

cpp/src/gandiva/tests/projector_test.cc

We don't hash byte_code_file_path anymore, so I plan to remove it.

Is it ok if I give you a patch for the test ?

…s and crossbow scripts

kszucs · 2019-02-20T00:26:43Z

We need to ensure that the remaining wheels (OSX and Windows) and conda packages are shipping gandiva as well.
Crossbow builds: https://github.com/kszucs/crossbow/branches/all?query=build-443

dev/tasks/python-wheels/travis.linux.yml

python/manylinux1/build_arrow.sh

cpp/Dockerfile

kszucs · 2019-02-21T11:11:05Z

Follow-up issues:

ARROW-4644: [C++/Docker] Build Gandiva in the docker containers
ARROW-4645: [C++/Packaging] Ship Gandiva with OSX and Windows wheels
ARROW-4646: [C++/Packaging] Ship gandiva with the conda-forge packages

pravindra

the gandiva changes LGTM. I skipped over the python changes.

kszucs · 2019-02-21T11:33:14Z

Thanks for your review @pravindra!

xhochy

+1, LGTM

We were not running the pyarrow tests after installing the manylinux wheels, which can lead to uncaught issues, like: https://travis-ci.org/kszucs/crossbow/builds/484284104 Author: Krisztián Szűcs <szucs.krisztian@gmail.com> Closes apache#3484 from kszucs/manylinux_tests and squashes the following commits: 3b1da30 <Krisztián Szűcs> use sudo c573a56 <Krisztián Szűcs> use env variables insude the container fd5e3fe <Krisztián Szűcs> use latest docker image tag d5531d9 <Krisztián Szűcs> test imports inside the wheel container 1aa19f1 <Krisztián Szűcs> reenable travis builds b399496 <Krisztián Szűcs> test py27mu and py36m wheels 71233c7 <Krisztián Szűcs> test 2.7,16 wheel 2372f3d <Krisztián Szűcs> fix requirements path; disable other CI tests 3e4ec2a <Krisztián Szűcs> unterminated llvm:MemoryBuffer; fix check_import.py path 7c88d61 <Krisztián Szűcs> only build python 3.6 wheel 18c5488 <Krisztián Szűcs> install wheel from dist dir 0bb07a7 <Krisztián Szűcs> better bash split 54fc653 <Krisztián Szűcs> don't export d3cb058 <Krisztián Szűcs> fix wheel building script 0d29b31 <Krisztián Szűcs> remove not existing resources from gandiva's pom 5d75adb <Krisztián Szűcs> initialize jni loader 09d829a <Krisztián Szűcs> build wheels for a single python distribution at a time; adjust travis and crossbow scripts 79abc0e <Krisztián Szűcs> mark .cc file as generated af78be2 <Krisztián Szűcs> don't bundle irhelpers in the jar a88cd37 <Krisztián Szűcs> cmake format 7deb359 <Krisztián Szűcs> fix REGEX; remove byteCodeFilePath from java configuration object fa19529 <Krisztián Szűcs> properly construct llvm:StringRef 5841dcd <Krisztián Szűcs> remove commented code 42391b1 <Krisztián Szűcs> don't pass precompiled bitcode all around the constructors d480c83 <Krisztián Szűcs> use const string ref for now b0b1117 <Krisztián Szűcs> conda llvmdev 169f43a <Krisztián Szűcs> build gandiva in cpp docker image cb69625 <Krisztián Szűcs> silent maven download msgs 19200c3 <Krisztián Szűcs> don't run wheel tests twice; cmake format f2205d0 <Krisztián Szűcs> gandiva jni dbf5b1c <Krisztián Szűcs> embed precompiled bitcode as char array; load precompiled IR from string 00d98e0 <Krisztián Szűcs> try to bundle bytecode files 97fe94b <Krisztián Szűcs> fix requirements-test.txt path 86e7e5b <Krisztián Szűcs> run pyarrow tests in manylinux CI build

kszucs commented Jan 29, 2019

View reviewed changes

pitrou reviewed Jan 30, 2019

View reviewed changes

kszucs force-pushed the manylinux_tests branch from 96d2b76 to b21c88e Compare February 15, 2019 11:54

kszucs changed the title ~~ARROW-4372: [Python] Run pyarrow tests in manylinux travis build~~ ARROW-4372: [C++] Embed precompiled bitcode in the gandiva library Feb 15, 2019

kszucs commented Feb 15, 2019

View reviewed changes

kszucs added 10 commits February 19, 2019 21:12

mark .cc file as generated

79abc0e

build wheels for a single python distribution at a time; adjust travi…

09d829a

…s and crossbow scripts

initialize jni loader

5d75adb

remove not existing resources from gandiva's pom

0d29b31

fix wheel building script

d3cb058

don't export

54fc653

better bash split

0bb07a7

install wheel from dist dir

18c5488

only build python 3.6 wheel

7c88d61

unterminated llvm:MemoryBuffer; fix check_import.py path

3e4ec2a

kszucs force-pushed the manylinux_tests branch from 4d61141 to 3e4ec2a Compare February 19, 2019 20:27

kszucs added 4 commits February 20, 2019 00:06

fix requirements path; disable other CI tests

2372f3d

test 2.7,16 wheel

71233c7

test py27mu and py36m wheels

b399496

reenable travis builds

1aa19f1

kszucs requested a review from xhochy February 20, 2019 00:39

xhochy reviewed Feb 20, 2019

View reviewed changes

dev/tasks/python-wheels/travis.linux.yml Show resolved Hide resolved

python/manylinux1/build_arrow.sh Outdated Show resolved Hide resolved

cpp/Dockerfile Show resolved Hide resolved

kszucs added 4 commits February 20, 2019 13:42

test imports inside the wheel container

d5531d9

use latest docker image tag

fd5e3fe

use env variables insude the container

c573a56

use sudo

3b1da30

kszucs force-pushed the manylinux_tests branch from 14aa22e to 3b1da30 Compare February 21, 2019 11:03

pravindra approved these changes Feb 21, 2019

View reviewed changes

xhochy approved these changes Feb 21, 2019

View reviewed changes

kszucs closed this in e8cc48b Feb 21, 2019

asfimport mentioned this pull request Feb 21, 2019

[C++] Embed precompiled bitcode in the gandiva library #20939

Closed

Uh oh!

ARROW-4372: [C++] Embed precompiled bitcode in the gandiva library #3484

ARROW-4372: [C++] Embed precompiled bitcode in the gandiva library #3484

Uh oh!

Conversation

kszucs commented Jan 25, 2019

Uh oh!

kszucs commented Jan 28, 2019

Uh oh!

kszucs commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xhochy commented Jan 28, 2019

Uh oh!

wesm commented Jan 28, 2019

Uh oh!

kszucs commented Jan 28, 2019

Uh oh!

kou commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kszucs commented Jan 29, 2019

Uh oh!

kou commented Jan 29, 2019

Uh oh!

kou commented Jan 29, 2019

Uh oh!

kszucs commented Jan 29, 2019

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!

kszucs commented Jan 31, 2019

Uh oh!

pravindra commented Jan 31, 2019

Uh oh!

pitrou commented Jan 31, 2019

Uh oh!

kszucs commented Jan 31, 2019

Uh oh!

kszucs commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Jan 31, 2019

Uh oh!

kszucs commented Jan 31, 2019

Uh oh!

pitrou commented Jan 31, 2019

Uh oh!

kszucs commented Jan 31, 2019

Uh oh!

pitrou commented Jan 31, 2019

Uh oh!

kszucs commented Jan 31, 2019

Uh oh!

pravindra commented Jan 31, 2019

Uh oh!

wesm commented Feb 9, 2019

Uh oh!

kszucs commented Feb 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kszucs commented Jan 28, 2019 •

edited

Loading

kszucs commented Jan 31, 2019 •

edited

Loading

kszucs commented Feb 20, 2019 •

edited

Loading