Missing package: boost-histogram

matthewfeickert commented 1 year ago

Package name

boost-histogram

Package version

all

PyPI URL

https://pypi.org/project/boost-histogram/

piwheels URL

https://www.piwheels.org/project/boost-histogram/

Python version

[X] Python 3.7
[X] Python 3.9

I am the maintainer

[ ] Yes

More information

I'm not a maintainer of boost-histogram but I am a member of the Scikit-HEP admin team (boost-histogram is a Scikit-HEP project). boost-histogram powers a lot of functionality in the Scikit-HEP ecosystem, so it would be quite nice if support could be added to compliment the nice support for awkward.

bennuttall commented 1 year ago

We had to skip this package as it was repeatedly hitting the 3 hour timeout and blocking anything else from being built. We can try again, perhaps just the latest version, if you think it should build ok.

Have you tried building it on a Pi? Is there anything special that's needed? We can install apt packages as required.

If it requires Rust, then it'll have to wait for the work on rustup to be completed https://github.com/piwheels/piwheels/pull/328

matthewfeickert commented 1 year ago

Have you tried building it on a Pi? Is there anything special that's needed? We can install apt packages as required.

I haven't yet, but I can try myself later.

If it requires Rust, then it'll have to wait for the work on rustup to be completed piwheels/piwheels#328

There's no Rust to my knowledge. Just C++ and pybind11.

matthewfeickert commented 1 year ago

I was able to pretty quickly build a sdist (no surprise there) and a wheel for boost-histogram v1.3.2 on my Raspberry Pi 4 Model B running Raspbian 11 bullseye

$ uname -a
Linux raspberrypi 5.15.84-v7l+ #1613 SMP Thu Jan 5 12:01:26 GMT 2023 armv7l GNU/Linux
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/arm-linux-gnueabihf/10/lto-wrapper
Target: arm-linux-gnueabihf
Configured with: ../src/configure -v --with-pkgversion='Raspbian 10.2.1-6+rpi1' --with-bugurl=file:///usr/share/doc/gcc-10/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-10 --program-prefix=arm-linux-gnueabihf- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --disable-libquadmath-support --enable-plugin --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-sjlj-exceptions --with-arch=armv6 --with-fpu=vfp --with-float=hard --disable-werror --enable-checking=release --build=arm-linux-gnueabihf --host=arm-linux-gnueabihf --target=arm-linux-gnueabihf
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.2.1 20210110 (Raspbian 10.2.1-6+rpi1)

with the following

$ python -m venv build && . build/bin/activate
$ python -m pip install --upgrade pip setuptools wheel
$ python -m pip install build
$ git clone --recursive --branch v1.3.2 https://github.com/scikit-hep/boost-histogram.git
$ cd boost-histogram/
$ python -m build .
...
Successfully built boost_histogram-1.3.2.tar.gz and boost_histogram-1.3.2-cp39-cp39-linux_armv7l.whl

I tried to test the wheel by installing and getting it to pass pytest, but it fails at import

$ python -m pip install ./dist/boost_histogram-1.3.2-cp39-cp39-linux_armv7l.whl
$ python -m pip install pytest pytest-benchmark
$ python -c 'import boost_histogram'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/__init__.py", line 1, in <module>
    from . import accumulators, axis, numpy, storage
  File "/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/accumulators.py", line 1, in <module>
    from ._core.accumulators import (  # pylint: disable=import-error
ImportError: /home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/_core.cpython-39-arm-linux-gnueabihf.so: undefined symbol: __atomic_load_8

From a quick glance at the Issues it seems that this undefined symbol: __atomic_load_8 is something that people have seen before, but I'm not experienced enough to know what this means without doing some looking (which I don't have time to do right right now).

If it is of interest, here's the wheel that I built (I've appended the .zip to the end so that GitHub will recognize it as a zip file and let me upload it): boost_histogram-1.3.2-cp39-cp39-linux_armv7l.whl.zip

(I don't think he'll have time to comment, but cc @henryiii — one of the maintainers.)

henryiii commented 1 year ago

3 hour timeout

It shouldn't take 3 hours to build! I can build in under a minute on my MacBook on 8 cores; a more normal computer still usually can do it in 5-10 mins. Maybe you are running out of memory and using the swap - I'd try forcing the build to run on a single core via setting CMAKE_BUILD_PARALLEL_LEVEL to 1 (or maybe 2 if that works). You don't need to run all the tests, just a couple of tests (maybe as simple as importing it) should be fine, maybe that would help?

matthewfeickert commented 1 year ago

It shouldn't take 3 hours to build! I can build in under a minute on my MacBook on 8 cores; a more normal computer still usually can do it in 5-10 mins.

On my Raspberry Pi

$ time python -m build .
...
Successfully built boost_histogram-1.3.2.tar.gz and boost_histogram-1.3.2-cp39-cp39-linux_armv7l.whl

real    6m51.681s
user    12m36.840s
sys 0m30.553s
$ export CMAKE_BUILD_PARALLEL_LEVEL=1
$ time python -m build .
...
Successfully built boost_histogram-1.3.2.tar.gz and boost_histogram-1.3.2-cp39-cp39-linux_armv7l.whl

real    10m43.196s
user    10m14.357s
sys 0m23.024s
$ unset CMAKE_BUILD_PARALLEL_LEVEL

but this is also the wheel that fails on import boost_histogram, so not sure if a working wheel takes longer.

henryiii commented 1 year ago

Yes, that's about what I'd expect, so I'm guessing maybe the memory on the build nodes is really low and multhreaded building is filling it, causing this to take much longer multithreaded.

I think we needs -latomic injected for ARM. Currently it's using setuptools (ignore the CMAKE in the variable above!), so we can manually add it if we detect an armv7 platform, perhaps? For now, I think setting LDFLAGS would be easiest.

matthewfeickert commented 1 year ago

I think we needs -latomic injected for ARM. Currently it's using setuptools (ignore the CMAKE in the variable above!), so we can manually add it if we detect an armv7 platform, perhaps? For now, I think setting LDFLAGS would be easiest.

I attempted this with the following patch

$ git diff v1.3.2
diff --git a/setup.py b/setup.py
index 308dbb1..b2a2768 100644
--- a/setup.py
+++ b/setup.py
@@ -1,6 +1,7 @@
 import os
 import sys
 from pathlib import Path
+import platform

 from setuptools import setup

@@ -38,6 +39,13 @@ INCLUDE_DIRS = [
     "extern/variant2/include",
 ]

+if sys.platform.startswith("win32"):
+    extra_compile_args=["/d2FH4-"]
+elif platform.machine() == "armv7l":
+    extra_compile_args=["-latomic"]
+else:
+    extra_compile_args=[]
+
 ext_modules = [
     Pybind11Extension(
         "boost_histogram._core",
@@ -45,7 +53,7 @@ ext_modules = [
         include_dirs=INCLUDE_DIRS,
         cxx_std=cxx_std,
         include_pybind11=False,
-        extra_compile_args=["/d2FH4-"] if sys.platform.startswith("win32") else [],
+        extra_compile_args=extra_compile_args,
     )
 ]

but unless I'm doing something wrong (very possible) this still fails with

$ python -c 'import boost_histogram'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/__init__.py", line 1, in <module>
    from . import accumulators, axis, numpy, storage
  File "/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/accumulators.py", line 1, in <module>
    from ._core.accumulators import (  # pylint: disable=import-error
ImportError: /home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/_core.cpython-39-arm-linux-gnueabihf.so: undefined symbol: __atomic_load_8

henryiii commented 1 year ago

That's a link arg, not a compile arg. Try:

LDFLAGS="-latomic" python -m build .

matthewfeickert commented 1 year ago

That's a link arg, not a compile arg.

Whoops! Sorry I seem to be asleep at the keyboard today — can't get much more obvious than that.

Try:

LDFLAGS="-latomic" python -m build .

Still failing though

$ LDFLAGS="-latomic" python -m build .
...
Successfully built boost_histogram-1.3.2.tar.gz and boost_histogram-1.3.2-cp39-cp39-linux_armv7l.whl
$ python -m pip uninstall -y boost-histogram
$ python -m pip install ./dist/boost_histogram*.whl
$ python -c 'import boost_histogram'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/__init__.py", line 1, in <module>
    from . import accumulators, axis, numpy, storage
  File "/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/accumulators.py", line 1, in <module>
    from ._core.accumulators import (  # pylint: disable=import-error
ImportError: /home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/_core.cpython-39-arm-linux-gnueabihf.so: undefined symbol: __atomic_load_8

matthewfeickert commented 1 year ago

The following patch works though

$ git diff v1.3.2
diff --git a/setup.py b/setup.py
index 308dbb1..caf5237 100644
--- a/setup.py
+++ b/setup.py
@@ -1,6 +1,7 @@
 import os
+import platform
 import sys
 from pathlib import Path

 from setuptools import setup

@@ -46,6 +47,7 @@ ext_modules = [
         cxx_std=cxx_std,
         include_pybind11=False,
         extra_compile_args=["/d2FH4-"] if sys.platform.startswith("win32") else [],
+        extra_link_args=["-latomic"] if platform.machine() == "armv7l" else [],
     )
 ]

$ python -m build .
...
Successfully built boost_histogram-1.3.3.dev0+g0c8659c.d20230110.tar.gz and boost_histogram-1.3.3.dev0+g0c8659c.d20230110-cp39-cp39-linux_armv7l.whl
$ python -m pip uninstall -y boost-histogram
$ python -m pip install ./dist/boost_histogram*.whl
$ python -c 'import boost_histogram; print(boost_histogram)'
<module 'boost_histogram' from '/home/pi/.local/venvs/build/lib/python3.9/site-packages/boost_histogram/__init__.py'
$ python -m pip install --upgrade "pytest>=6.0" "pytest-benchmark" "cloudpickle" "hypothesis>=6.0"
$ pytest
...
============================================================================== 830 passed in 117.87s (0:01:57) ===============================================================================

boost_histogram-1.3.3.dev0+g0c8659c.d20230110-cp39-cp39-linux_armv7l.whl.zip

where once the zip file is unpacked/wheel installed

$ objdump --all-headers _core.cpython-39-arm-linux-gnueabihf.so | grep atomic
  NEEDED               libatomic.so.1
  required from libatomic.so.1:
00000000       F *UND*  00000000              __atomic_fetch_add_8@LIBATOMIC_1.0
00000000       F *UND*  00000000              __atomic_store_8@LIBATOMIC_1.0
00000000       F *UND*  00000000              __atomic_load_8@LIBATOMIC_1.0

where

$ ldconfig --print-cache | grep atomic
    libatomic.so.1 (libc6,hard-float) => /lib/arm-linux-gnueabihf/libatomic.so.1

bennuttall commented 1 year ago

Is it possible to build from PyPI with just pip3 wheel? That's how our automated builds work. I'll try it when I get chance.

matthewfeickert commented 1 year ago

Is it possible to build from PyPI with just pip3 wheel? That's how our automated builds work. I'll try it when I get chance.

Not without the patch that I have above. I've opened https://github.com/scikit-hep/boost-histogram/issues/822 to discuss if this is worth supporting, or if the maintainers would prefer people to just move over to using a 64-bit OS on Raspberry Pi.

henryiii commented 1 year ago

pip3 wheel and python -m build --wheel are basically the same, with python -m build being preferred. Pip is slowly moving toward making build the backend for the wheel command (technically, I think they'd rather drop it, but too many people are using it because it predates build). It doesn't make any difference above, it's just that armv7 requires an extra non-standard link argument to use C++ atomics.

I don't know why LDFLAGS didn't work, it seems like it should here: https://setuptools.pypa.io/en/latest/userguide/ext_modules.html#compiler-and-linker-options

matthewfeickert commented 1 year ago

I don't know why LDFLAGS didn't work, it seems like it should here: https://setuptools.pypa.io/en/latest/userguide/ext_modules.html#compiler-and-linker-options

I agree that this seems strange for the exact reasons that you mentioned

The linker options appear in the command line in the following order:

first, the options provided by environment variables and sysconfig variables,

then, a -L option for each element of Extension.library_dirs,

then, a linker-specific option like -Wl,-rpath for each element of Extension.runtime_library_dirs,

finally, the options provided by Extension.extra_link_args.

The fact that you can set enviornmental variables that get picked up by python -m build was the whole point of https://github.com/scikit-hep/pyhf/pull/1887, so this is confusing.

matthewfeickert commented 1 year ago

I rebuilt the wheels and the only difference that I can see when looking at their logs is where -latomic gets placed

v1.3.2

$ LDFLAGS="-latomic" python -m build . &> LDFLAGS_build_out.txt
$ grep latomic LDFLAGS_build_out.txt 
arm-linux-gnueabihf-g++ -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -latomic build/temp.linux-armv7l-cpython-39/src/module.o build/temp.linux-armv7l-cpython-39/src/register_accumulators.o build/temp.linux-armv7l-cpython-39/src/register_algorithm.o build/temp.linux-armv7l-cpython-39/src/register_axis.o build/temp.linux-armv7l-cpython-39/src/register_histograms.o build/temp.linux-armv7l-cpython-39/src/register_storage.o build/temp.linux-armv7l-cpython-39/src/register_transforms.o -L/usr/lib -o build/lib.linux-armv7l-cpython-39/boost_histogram/_core.cpython-39-arm-linux-gnueabihf.so

vs.

with patch

$ python -m build . &> patch_build_out.txt
$ grep latomic patch_build_out.txt 
arm-linux-gnueabihf-g++ -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 build/temp.linux-armv7l-cpython-39/src/module.o build/temp.linux-armv7l-cpython-39/src/register_accumulators.o build/temp.linux-armv7l-cpython-39/src/register_algorithm.o build/temp.linux-armv7l-cpython-39/src/register_axis.o build/temp.linux-armv7l-cpython-39/src/register_histograms.o build/temp.linux-armv7l-cpython-39/src/register_storage.o build/temp.linux-armv7l-cpython-39/src/register_transforms.o -L/usr/lib -o build/lib.linux-armv7l-cpython-39/boost_histogram/_core.cpython-39-arm-linux-gnueabihf.so -latomic

The only thing that I can think of is this comment in this Stack Overflow question:

Recent versions of GCC require that you put the object files and libraries in the order that they depend on each other - as a consequential rule of thumb, you have to put the library flags as the last switch for the linker.

matthewfeickert commented 1 year ago

@bennuttall Now that boost-histogram v1.4.0 is out could you attempt this build again?

bennuttall commented 1 year ago

That built ok for me.

We're currently working through the backlog of Python 3.11 builds so it'll try those in the next few days and then it'll try 1.4.0 on Buster and Bullseye once we re-introduce those builders.

matthewfeickert commented 1 year ago

Thanks very much @bennuttall. SGTM!

piwheels / packages