-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: test Pyodide with cibuildwheel and fix 32-bit issue #3270
Conversation
2f7cc25
to
a18048a
Compare
Since "Build C++ WASM" still fails, we'll keep this in It looks like it has a significant merge conflict, too. |
1f020d2
to
00a73c2
Compare
Okay, now getting a Pyodide fatal error:
|
is
which is core libawkward. |
This issue is solved by bumping CIBW to 2.22. However, the tests are skipped or fail because none of the dependencies are installed and Pyodide doesn't have |
I've just merged pyodide/pyodide#5247, so That said, I can't fully tell what the error was caused by and what resolved it, there weren't many changes between Pyodide 0.26.1 in cibuildwheel < 2.22 and cibuildwheel >=2.22's Pyodide 0.26.4, besides a few more JSPI-related bug fixes for stack switching and a bump from Python 3.12.1 to Python 3.12.7. |
I tried it on my fork via agriyakhetarpal#1 and, after temporarily skipping seven tests where ForthMachine does not seem to work with 32-bit environments and emits conversion errors, the rest of the tests all pass now: I'd like to reinstate the WASM job that was disabled as an effect of #3329 so that I can work on some further improvements around interactive docs for |
Fixing awkward-cpp's WASM issues is very welcome! (I had been under the impression that it was working. We do have a Windows 32-bit suite of tests to ensure that all integer/pointer handling correctly distinguishes between awkward-cpp has two main pieces: one is exposed through pybind11 in the ordinary way for a Python extension, and 3 subsystems are accessed this way:
The other main piece is accessed through ctypes, by compiling the C++ functions with an
I think all of these things are loaded on startup, when awkward/awkward-cpp/include/awkward/common.h Lines 6 to 7 in 0cc66e8
awkward/awkward-cpp/include/awkward/common.h Lines 24 to 25 in 0cc66e8
awkward/awkward-cpp/include/awkward/common.h Lines 36 to 41 in 0cc66e8
This is one possible culprit, but anything like this is suspect and can be investigated. We have enough tests for all the other builds that if you need to change something to make WASM work and all other tests still pass, it's a good fix. (To be super-cautious, we can also manually check the CUDA tests—which don't run in GitHub Actions—but I think they're very separate from the rest of the build procedure because they use CuPy for runtime compilation.) |
Thanks for the extended information!
that are failing at the moment. I am not an experienced C++ programmer in any fashion, but I can try to see where I can debug it while also getting the WASM wheels deployed with the docs again. |
The Avro reader is a "canary in a coal mine" in the sense that it's one of the few extended uses of AwkwardForth in the Awkward codebase. AwkwardForth is primarily needed for Uproot, but Awkward Array can't load Uproot in its tests without a circular dependence. Even though these two AwkwardForth-based tests fail, do the Uproot tests run (when using this compiled version of awkward-cpp)? That would give us an indication of how serious the failure is. For instance, what if it's failing in these two tests for a reason that's irrelevant to Uproot? If that's the case, then these two tests are using functionality we may not need and perhaps could remove. AwkwardForth is not a user-facing API (technically public so that a library like Uproot can use it, but not publicized); we can consider removing features. Of course, it would be even better to understand the failure and fix it. I've been assuming that the issue has something to do with compiling and linking—an area in which I'm not familiar with the details—but if it's a feature that's not used by Uproot, then it's possible that there's a segfault or other undefined behavior in the C++. So, checking the Uproot tests would be informative, either way. The Uproot tests that use AwkwardForth most intensively are:
Uproot does have a Pyodide test. I think it only tests local files (to avoid unimplemented features in Pyodide, many of which involve threading), but the above tests are based on local files. (scikit-hep-testdata downloads the file locally before using it.) |
00a73c2
to
64e8222
Compare
Signed-off-by: Henry Schreiner <[email protected]>
64e8222
to
014c9d5
Compare
I'm getting:
On lots (all?) Forth tests. Could this happen if it can't find the file? |
Sorry for not responding here – I did start looking into this some time ago, but I don't recall why I stopped (likely because |
I made a PR for Uproot (scikit-hep/uproot5#1365) that adds a test that uses AwkwardForth in wasm. The setup is pretty different (cibw is not used), so it is definitely that AwkwardForth is broken in wasm. |
The most basic functionality like vm = ForthMachine32("3 5 +")
vm.run() works, but as soon as there's anything more complex like ForthMachine32(": callme 1 2 3 4 ;") it crashes with the same error. As far as I can tell, the only usage of awkward/awkward-cpp/src/libawkward/forth/ForthMachine.cpp Lines 1427 to 1448 in 966533d
So maybe the try-catch block is not working properly? I'm not sure what's the best way to debug this in wasm, but I'll see if I can reproduce it with a minimal Emscripten code. |
I was able to reproduce this in a simple code compiled with Emscripten. Then I found that the reason for this is that exceptions are disabled in Emscripten by default (see here). Enabling them made the code work as expected. So @henryiii I think the fix is just to tell cibw to use |
pybind11 adds this for you. But maybe this part doesn't use pybind11 when compiling? |
Signed-off-by: Henry Schreiner <[email protected]>
Now I get:
|
It should work now. It was just missing an extra check for 32-bit systems (it was only checking for x86). |
Would this work more portably?
? |
Yeah, that would be better. I was also thinking whether it could be a problem later on if Emscripten starts supporting wasm64. Up to you if you want me to change it or you go ahead. |
Signed-off-by: Henry Schreiner <[email protected]>
Isn't |
@henryiii - shall I wait for this PR integration before the release? Thanks! |
It contains a fix for emscripten now, so I'd say yes. |
If it passes CI, it's ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@henryiii - Great! Thank you. CI passes, I'm merging it now.
Thanks! I can now revive my branch that will reinstate Pyodide-enabled interactive docs for |
Using cibuildwheel for pyodide. Adds testing.