ci: bump to Pyodide 3.12 #938
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
henryiii
force-pushed
the
henryiii/ci/emscripten4
branch
from
06a5dbb
to
f0c3f9b
Compare
|
Improvement: |
|
Oh well the problem I'm having is that I am building without |
|
It does inherit from |
|
This is a bit of a side track. But it decides that it does not inherit from std::exception because the following code sets void __get_exception_message(void* thrown_object, char** type, char** message) {
__cxa_exception* exception_header =
cxa_exception_from_thrown_object(thrown_object);
const __shim_type_info* thrown_type =
static_cast<const __shim_type_info*>(exception_header->exceptionType);
const char* type_name = thrown_type->name();
int status = 0;
char* demangled_buf = __cxa_demangle(type_name, 0, 0, &status);
if (status == 0 && demangled_buf) {
*type = demangled_buf;
} else {
if (demangled_buf) {
free(demangled_buf);
}
*type = (char*)malloc(strlen(type_name) + 1);
strcpy(*type, type_name);
}
*message = NULL;
const __shim_type_info* catch_type =
static_cast<const __shim_type_info*>(&typeid(std::exception));
int can_catch = catch_type->can_catch(thrown_type, thrown_object);
if (can_catch) {
const char* what =
static_cast<const std::exception*>(thrown_object)->what();
*message = (char*)malloc(strlen(what) + 1);
strcpy(*message, what);
}
} |
|
But it sure looks like const __shim_type_info* catch_type =
static_cast<const __shim_type_info*>(&typeid(std::exception));
int can_catch = catch_type->can_catch(thrown_type, thrown_object);returns |
|
Anyways now that I am building with |
|
Seems that |
|
So the problem is that |
|
If I fill in const func = new WebAssembly.Function({parameters: ["i32", "i32", "i32"], results:[]}, Module.___cxa_throw);
const slot = Module.wasmTable.grow(1);
Module.wasmTable.set(slot, func);
GOT.__cxa_throw.value = slot; |
|
Actually, just calling |
|
I think this Emscripten patch should fix the dynamic linking problems in the cli runner. Probably also solves the reason that --- a/src/library_dylink.js
+++ b/src/library_dylink.js
@@ -1143,7 +1143,9 @@ var LibraryDylink = {
}
try {
- return loadDynamicLibrary(filename, combinedFlags, localScope, handle)
+ const result = loadDynamicLibrary(filename, combinedFlags, localScope, handle);
+ reportUndefinedSymbols();
+ return result;
} catch (e) {
#if ASSERTIONS
err(`Error in loading dynamic library ${filename}: ${e}`); |
|
Great to hear there is a potential fix for this! I was wondering indeed whether this was not going to be an issue to use cibuildwheel for scikit-learn. I guess this is the issue you have in mind: pyodide/pyodide#3865. |
|
Hmm darn that sounds different. Here symbol resolution succeeded but the code to lazily initialize the GOT failed. There it looks like it failed at symbol resolution. So there's several bugs it seems. We're using this same mechanism for loading shared libs at cloudflare (it's necessary for performance) so there's a lot of reason to fix it... |
|
Minimal reproduction here is: from boost_histogram._core import axis as ca
ax1 = ca.regular_uflow(1, 2, 3)
ax2 = ca.regular_none(1, 2, 3)
ax1 == ax2Now I guess I'll have to start minimizing the C++ code =/ |
|
You can copy it locally and swap out the find_package for add_subdirectory. |
|
The repl is pretty broken on iOS in 0.26 otherwise I’d check it with the in-tree one. Can do later on a computer. |
|
I think I'm making pretty good progress on minimizing. So far have the C++ code in src down to: #include <bh_python/pybind11.hpp>
#include <bh_python/axis.hpp>
#include <bh_python/register_axis.hpp>
void register_axes(py::module& mod) {
auto ax = register_axis<axis::regular_none>(mod);
ax.def(py::init<unsigned, double, double>(), "bins"_a, "start"_a, "stop"_a);
ax = register_axis<axis::regular_uflow>(mod);
ax.def(py::init<unsigned, double, double>(), "bins"_a, "start"_a, "stop"_a);
}trying to reduce |
|
|
|
Currently I have #include <pybind11/pybind11.h>
#include <boost/histogram/axis/regular.hpp>
#include <boost/histogram/axis.hpp>
namespace py = pybind11;
namespace bh = boost::histogram;
using regular_none
= bh::axis::regular<double, bh::use_default, int, bh::axis::option::none_t>;
using regular_uflow
= bh::axis::regular<double, bh::use_default, double, bh::axis::option::none_t>;
PYBIND11_MODULE(_core, m) {
py::module mod = m.def_submodule("axis");
mod.def("do_the_thing", [] (const py::object& a) {
py::cast<regular_none>(a);
return 77;
});
py::class_<regular_uflow> ax(mod, "regular_uflow");
ax.def(py::init<unsigned, double, double>());
}and from boost_histogram._core import axis as ca
ca.do_the_thing(ca.regular_uflow(1, 2, 3))Without the fix, it is a fatal error. With the fix, it raises: RuntimeError: Unable to cast Python instance of type <class 'boost_histogram._core.axis.regular_uflow'> to C++ type 'boost::histogram::axis::regular<double, boost::use_default, int, boost::histogram::axis::option::bitset<0u>>' |
|
@henryiii looks like at this point I should be ready to remove |
|
Okay, no #include <pybind11/pybind11.h>
template <class Value>
class regular {};
namespace py = pybind11;
PYBIND11_MODULE(_core, m) {
py::module mod = m.def_submodule("axis");
mod.def("do_the_thing", [] (const py::object& a) {
py::cast<regular<double>>(a);
return 77;
});
py::class_<regular<int>> ax(mod, "Thing");
ax.def(py::init<>());
}and: from boost_histogram._core import axis as ca
print(ca.do_the_thing(ca.Thing())) |
|
If I can't reduce this any further, this already would be an acceptable test case. Still, would be nice to get rid of pybind11 and Python and get a C++ only reproducer... |
|
Actually, if you delete |
|
I've been using |
|
FYI, this works fine with the in-tree build of boost-histogram. That is based on the old setuptools build system instead of CMake, but assuming this shouldn't matter much? (the out-of-tree build system worked fine until the latest pyodide) |
|
Well it's just the out of tree CLI runner. It uses a lazier dynamic linking strategy. Normal Pyodide does pessimistic eager loads which is easier to get right but is slow. It would be nice to use the lazier approach always. But for now, I took a different tradeoff so that |
|
Anyways, the compile flags are: If we remove -okay.o: file format wasm 0x1
+crash.o: file format wasm 0x1
Section Details:
@@ -58,7 +58,7 @@ Data[2]:
Custom:
- name: "linking"
- symbol table [count=26]
- - 0: F <PyInit__core> func=18 [ binding=global vis=hidden ]
+ - 0: F <PyInit__core> func=18 [ binding=global vis=default ]
- 1: D <__THREW__> [ undefined binding=global vis=default ]
- 2: F <env.__cxa_allocate_exception> func=0 [ undefined binding=global vis=default ]
- 3: G <env.__memory_base> global=0 [ undefined binding=global vis=default ] |
|
Right now I have: build.sh: set -x
em++ \
-isystem \
.pyodide-xbuildenv-0.26.0/0.26.0/xbuildenv/pyodide-root/cpython/installs/python-3.12.1/include/python3.12 \
-c src/module.cpp \
-o module.cpp.o \
-O3 -fPIC -fexceptions -g2 \
#-fvisibility=hidden
# error happens with -fvisibility=hidden commented out but not if we pass it
em++ \
-o dist/boost_histogram-1.4.2.dev14+g54eb2dd.d20240603/boost_histogram/_core.cpython-312-wasm32-emscripten.so \
module.cpp.o \
-s WASM_BIGINT -s SIDE_MODULE=1 -fexceptions -g2 -O2 \
wheel pack dist/boost_histogram-1.4.2.dev14+g54eb2dd.d20240603 -d distand #include "Python.h"
#include <stdexcept>
extern "C"
PyObject *PyInit__core() {
try {
throw std::runtime_error("");
} catch (const std::exception &e) {
PyErr_SetString(PyExc_ImportError, "oops");
}
return nullptr;
} and print("=" * 20)
try:
from boost_histogram._core import do_the_thing
except Exception:
print("ok!")and rm -rf .venv-pyodide
pyodide venv .venv-pyodide
.venv-pyodide/bin/pip install dist/boost_histogram-1.4.2*.whl
.venv-pyodide/bin/python repro.py |
|
Okay I'm being silly, of course when we don't export ImportError('dynamic module does not define module export function (PyInit__core)')instead of the desired crash but this isn't an interesting observation. |
|
Emscripten bug: |
After many hours of debugging, I minimized the problem down to this: emscripten-core/emscripten#22052 Thanks to @henryiii for reporting. See thread here for my comments while I was debugging: scikit-hep/boost-histogram#938
After many hours of debugging, I minimized the problem down to this: emscripten-core/emscripten#22052 Thanks to @henryiii for reporting. See thread here for my comments while I was debugging: scikit-hep/boost-histogram#938 Resolves #2964.
After many hours of debugging, I minimized the problem down to this: emscripten-core/emscripten#22052 Thanks to @henryiii for reporting. See thread here for my comments while I was debugging: scikit-hep/boost-histogram#938 Resolves pyodide#2964.
|
Curious, I naively would have thought: - run: CFLAGS=-fexceptions LDFLAGS=-fexceptions pyodide build --exports whole_archive
+ run: pyodide build --exports whole_archiveAlong with [tool.pyodide.build]
cxxflags = "-fexceptions"
ldflags = "-fexceptions"Would have been the same, but now it's reporting I do see this warning when building, which is suspicious: |
Well that sounds like another thing to look into... |
I expect this to fail. (Okay, not that early!)