Pyarrow #35589

teh · 2018-02-25T22:51:08Z

Motivation for this change

This is probably one for @FRidh. I've been meaning to package pyarrow for a while, but it turned out to be a bit fiddly, mainly because its reliance on static libraries.

This PR is a suggestion for how we could package pyarrow, though I understand that some changes might be a bit controversial (new static flags for snappy, thrift & lz4 e.g.).

I still need to run this through nox-review pr & test the plasma binary

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

dotlambda · 2018-02-25T23:03:05Z

pkgs/development/libraries/rapidjson/default.nix

@@ -1,16 +1,21 @@
-{ stdenv, lib, fetchFromGitHub, pkgconfig, cmake }:
+{ stdenv, lib, fetchFromGitHub, pkgconfig, cmake, fetchpatch }:


fetchpatch isn't used

dotlambda · 2018-02-25T23:04:03Z

pkgs/development/libraries/rapidjson/disable-implicit-warnings.patch

+Author: Tom Hunger <tehunger@gmail.com>
+Date:   Sun Feb 25 22:26:38 2018 +0000
+
+    bla


Please write something meaningful or leave it out.

dotlambda · 2018-02-25T23:08:55Z

pkgs/development/libraries/thrift/default.nix

  nativeBuildInputs = [ pkgconfig ];
  buildInputs = [
-    boost zlib libevent openssl python bison flex twisted
+    boost zlib libevent openssl python bison flex twisted cmake


cmake belongs into nativeBuildInputs

dotlambda · 2018-02-25T23:09:45Z

pkgs/development/python-modules/pyarrow/default.nix

+};
+in buildPythonPackage rec {
+  # Loosely based on the instructions here: https://arrow.apache.org/docs/python/development.html
+  name = "${pname}-${version}";


dotlambda · 2018-02-25T23:11:11Z

pkgs/development/libraries/snappy/default.nix

@@ -17,7 +17,7 @@ stdenv.mkDerivation rec {

  nativeBuildInputs = [ cmake ];

-  cmakeFlags = [ "-DBUILD_SHARED_LIBS=ON" "-DCMAKE_SKIP_BUILD_RPATH=OFF" ];
+  cmakeFlags = [ "-DBUILD_SHARED_LIBS=ON" "-DCMAKE_SKIP_BUILD_RPATH=OFF" ] ++ (if withStatic then ["-DBUILD_SHARED_LIBS=OFF"] else []);


I'm not sure if withStatic is a good name since if it's enabled, only static libraries seem to be built. However, I'm not sure what the effect is for other derivations using withStatic.

dotlambda · 2018-02-25T23:14:32Z

pkgs/development/python-modules/pyarrow/default.nix

+  nativeBuildInputs = [ cmake ];
+  buildInputs = [
+    cython
+    setuptools_scm


probably nativeBuildInputs

dotlambda · 2018-02-25T23:14:41Z

pkgs/development/python-modules/pyarrow/default.nix

+  buildInputs = [
+    cython
+    setuptools_scm
+    pytest


checkInputs

dotlambda · 2018-02-25T23:15:17Z

pkgs/development/python-modules/pyarrow/default.nix

+  meta = with stdenv.lib; {
+    description = "Apache Arrow is a cross-language development platform for in-memory data.";
+    homepage = "https://arrow.apache.org/";
+  };


dotlambda · 2018-02-25T23:15:29Z

pkgs/development/python-modules/pyarrow/default.nix

+  checkPhase = ''
+    # test requires gzip which hasn't been enabled yet:
+    substituteInPlace pyarrow/tests/test_io.py --replace 'test_compress_decompress' '_test_disabled'
+    substituteInPlace pyarrow/tests/test_parquet.py --replace 'test_pandas_parquet_configuration_options' '_test_disabled'


dotlambda · 2018-02-25T23:18:04Z

pkgs/development/python-modules/pyarrow/default.nix

+    (pkgs.thrift.override { withStatic = true; })
+  ];
+};
+in buildPythonPackage rec {


I think all the above code would be more readable if it was indented by 2 spaces and if a blank line was inserted between different packages.
On a side note, why are these not provided in all-packages.nix?

FRidh

I don't see any motivation for building static libraries.

mainly because its reliance on static libraries.

Where is that documented? Have you tried without?

teh · 2018-02-26T09:53:28Z

Thanks for review @dotlambda - will address and comment here when done.

@FRidh It's not documented explicitly - it's in their build files [1]. It's probably possible to patch it out but I haven't managed to do so.

[1]
https://github.com/apache/arrow/blob/master/cpp/CMakeLists.txt#L530

https://github.com/apache/arrow/blob/master/cpp/cmake_modules/FindLz4.cmake

(when setting e.g

"-DLZ4_HOME=${pkgs.lz4.dev}"
"-DLZ4_SHARED_LIB=${pkgs.lz4}/lib/liblz4.so"

you get errors like -- Could NOT find LZ4 (missing: LZ4_STATIC_LIB))

FRidh · 2018-02-26T10:05:54Z

@teh have you raised an issue about this?

veprbl · 2018-03-12T23:11:21Z

I've just learned that there is symlinkJoin. I think that it would be much help when building arrow-cpp.

veprbl · 2018-03-16T20:33:48Z

#37196

teh added 4 commits February 25, 2018 22:37

rapidjson: fix owner & build for gcc 7 or later.

9017468

flatbuffers: 1.4.0 -> 1.8.0

b59ee88

thrift: 0.10.0 -> 0.11.0

88c103f

pyarrow: init at 0.8.0

90c3c6d

teh requested a review from FRidh as a code owner February 25, 2018 22:51

GrahamcOfBorg added 6.topic: python 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ labels Feb 25, 2018

dotlambda requested changes Feb 25, 2018

View reviewed changes

FRidh requested changes Feb 26, 2018

View reviewed changes

FRidh self-assigned this Feb 26, 2018

veprbl mentioned this pull request Apr 4, 2018

Add arrow-cpp, parquet-cpp, pythonPackages.pyarrow #38440

Merged

8 tasks

matthewbauer closed this in #38440 Apr 17, 2018

teh deleted the pyarrow branch April 21, 2018 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pyarrow #35589

Pyarrow #35589

teh commented Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018

dotlambda Feb 25, 2018 •

edited

FRidh left a comment •

edited

teh commented Feb 26, 2018

FRidh commented Feb 26, 2018

veprbl commented Mar 12, 2018

veprbl commented Mar 16, 2018

		@@ -1,16 +1,21 @@
		{ stdenv, lib, fetchFromGitHub, pkgconfig, cmake }:
		{ stdenv, lib, fetchFromGitHub, pkgconfig, cmake, fetchpatch }:

Pyarrow #35589

Pyarrow #35589

Conversation

teh commented Feb 25, 2018

Motivation for this change

Things done

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

dotlambda Feb 25, 2018 • edited

Choose a reason for hiding this comment

FRidh left a comment • edited

Choose a reason for hiding this comment

teh commented Feb 26, 2018

FRidh commented Feb 26, 2018

veprbl commented Mar 12, 2018

veprbl commented Mar 16, 2018

dotlambda Feb 25, 2018 •

edited

FRidh left a comment •

edited