Add boost-histogram

[henryiii]

Description

This adds boost-histogram, one of three key compiled libraries in the
Scikit-HEP ecosystem (and like all three, useful beyond HEP). It was featured
at SciPy 2020. The other two packages (iMinuit and Awkward) use setuptools CMake in the build
process, so might be more challenging to add.

I wasn't able to run the build again after updating to 1.3.0, since on updating
pyodide I now get:

Error building pyparsing. Printing build logs.
/src/.venv/bin/python: No module named wheel

when trying to build. Pretty sure I didn't do that. Actually, I did, as I made a venv to put pyodide-build in.

See #2167.

Checklists

• Add a CHANGELOG entry
• Add / update tests
• Add new / update outdated documentation

[henryiii]

TypeError: Cannot create property 'pyodide_fatal_error' on number '28811048'

There's a reproducer. ;)

[hoodmane]

There's a reproducer. ;)

Thanks!

[henryiii]

Ouch, during the rollout, we just discovered a bad regression in 1.3.0. Looks like there will be a 1.3.1 very soon; if this patch works, I can include it too, so we won't need a patch here.

If you are curious as to what happens in pyodide when you get a segfault, I can tell you how to segfault 1.3.0. 🤦

[hoodmane]

If you are curious as to what happens in pyodide when you get a segfault, I can tell you how to segfault 1.3.0. 🤦

I am interested if Pyodide doesn't give a fatal error message. wasm32_emscripten has no memory protection so if you try to dereference a bad pointer it won't instantly crash unless the pointer is larger than the total size of the memory. Likewise, dereferencing null works just fine. So we have to manually detect segfaults and report them otherwise the runtime will try to keep going after corrupting its state.

There is some luck required, it is possible the bad memory access will never propagate into anything that causes a trap and will just lead to weird stuff. But in most cases an actual wasm trap occurs a bit later and we can notice it and kill the runtime. Any case like #2178 where we fail to do this is very interesting.

[henryiii]

For the CPython segfault, Pyodide just locks up and hangs.

FYI, the segfault is from calling CPython API after releasing the GIL. It works if there's not a problem doing the conversion (mostly), but if it can't covert (-1 to uint), then it segfaults. Fixed in boost-histogram 1.3.1, which will be out very soon.

[henryiii]

Correction, I forgot to check the console, the segfaulting behavior acts like throwing a py::key_error, which also "hangs", but there's this in the console (either one):

An easy way to get the key error:

import boost_histogram as bh
mean = bh.accumulators.Mean()
mean["invalid"]

[hoodmane]

Hmm okay that looks interesting, I'll have to reproduce it locally and see if I can improve our handling of these errors. Thanks so much for all your help!

[henryiii]

If it could turn into proper KeyError like in CPython (even for just the latest browsers), that would be ideal/amazing. :)

[hoodmane]

Yeah, that'd be great. There's a similar issue with panic unwinds on #2081 which would also be nice to fix, though they may or may not be related. I will try to investigate this soon.

[hoodmane]

Okay I think #2178 fixes all of the examples you've given of bad fatal errors. On that branch your segfault shows up as:

Pyodide has suffered a fatal error. Please report this to the Pyodide maintainers.
The cause of the fatal error was:
CppException: Unable to cast Python instance to C++ type (compile in debug mode for details)

[hoodmane]

I figured out how to get the exception handling working. @henryiii should I push it here?

It increases the size of the main Pyodide module by about 150kb or about 1%, which is probably acceptable. I am not sure about impact on benchmarks, will have to see if it causes a noticeable performance regression.

[henryiii]

That sounds fantastic for pybind11 libraries (and probably others). Can be here or another PR, I can combine both and try it out locally if needed.

[hoodmane]

I think we can avoid any overhead in the main module other than the 150kb size increase.

[henryiii]

I've triggered the release of 1.3.1, will update here when the release happens (2-3 hours). The mean["invalid"] example will still trigger a Python exception from C++, but we are avoiding using exceptions for control flow (the patch here) now, as well as the GIL issue has been fixed.

[henryiii]

Does your work on exception handling help with the Rust panic unwinds, do you know? Also, does it fall back to the old behavior on older browsers?

[hoodmane]

It will work correctly on all browsers -- I'm using the Javascript exceptions not the wasm exceptions because there isn't enough browser support for wasm exceptions still. I have not investigated how it relates to Rust.

[henryiii]

Okay, great. 1.3.1 is just out, so after this builds, I'll bump to 1.3.1 and add a test expecting mean["invalid"] to raise an exception instead.

[hoodmane]

I compared the benchmarks to the ones on the main branch and there is no apparent difference, so I think they support my claim that this change doesn't impose any cost on the main module.

[henryiii]

Is there something I need to rebuild? I'm trying this locally and I get:

From what I understand (and from passing tests above), this should actually produce the Python error now (KeyError: 'invalid not one of count, value, _sum_of_deltas_squared'). I'm guessing I need to rebuild something other than PYODIDE_PACKAGES="boost-histogram" make? Do I need to remove --pre-built from ./run_docker? Would hope to keep at least some caching.

[hoodmane]

I think touch src/core/main.c and then PYODIDE_PACKAGES="boost-histogram" make will be sufficient.

[henryiii]

Didn't work, so used make clean on the main directory, plus in emsdk just to be sure.

Binary size grew a little, the wheel is now 939K. Very nice!!!

Is there a way I can access my test suite and/or a python file from the terminal emulator? Curious to try benchmarking.

[henryiii]

(I also just realized I don't need to be in the docker container to run python3 -m http.server 🤦)

[henryiii]

From a simple copy-paste performance test, boost-histogram in the browser is 3.0x slower than native (python 3.10). For a 1D regularly binned histogram, boost-histogram in a browser still beats native NumPy by a few percent. 🤣 Native NumPy 1D histograming is 3.2x slower in the browser. 2D histograms are 1.8x slower in boost-histogram and 1.3x slower in NumPy (but there boost-histogram has a 10x starting lead). The numbers from time are oddly rounded, maybe there's a clock accuracy limitation? Looks like it stops at milliseconds, perhaps.

[hoodmane]

The numbers from time are oddly rounded, maybe there's a clock accuracy limitation? Looks like it stops at milliseconds, perhaps.

See https://developer.mozilla.org/en-US/docs/Web/API/Performance/now:

It's important to keep in mind that to mitigate potential security threats such as Spectre, browsers typically round the returned value by some amount in order to be less predictable. This inherently introduces a degree of inaccuracy by limiting the resolution or precision of the timer. For example, Firefox rounds the returned time to 1 millisecond increments.

[henryiii]

No such file or directory: 'build-logs/pyrsistent.log'

Don't think that one's on me. But I've been wrong before. 🤣

[hoodmane]

No, that's a sporadic failure, I restarted CI.

[hoodmane]

Looks good to me, but ideally either @rth or @ryanking13 should review the changes I made to enable exceptions.

Thanks @henryiii it was fun working with you on this.

Note that my emscripten patch has been accepted: emscripten-core/emscripten#16309
In the corresponding issue the emscripten folks confirmed that this approach should not degrade performance in the main models.

[hoodmane]

Okay, I think what we should do is move the exception-related changes to the Makefiles over to #2178 and merge that first, then merge this.

[ryanking13]

Looks good to me too. Thanks for your effort on this @henryiii and @hoodmane.

Okay, I think what we should do is move the exception-related changes to the Makefiles over to #2178 and merge that first, then merge this.

This would be nice.

[rth]

Thanks, merging.

[rth]

Can we backport this to 0.19.1 in #2192 without including #2178 ?

[hoodmane]

Yes, ideally such a backport would include also the patch in f3dcdf6. But I think it may be fine to backport #2178 too.

[hoodmane]

Actually I think the best option is to add EXCEPTION_CATCHING_ALLOWED=['we only want to allow exception handling in side modules'] to ldflags and flags and add the emsdk patch in addition to this commit. That allows this package to do its exception handling without fatally falling. The rest of #2178 is more to make it easier to port C++ packages.