-
Notifications
You must be signed in to change notification settings - Fork 29
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
WASM build #262
base: main
Are you sure you want to change the base?
WASM build #262
Conversation
I've been testing it a bit through a PR in my fork, and it all builds well, up to one point in the Cython sources where it fails in step 107/114: https://github.com/agriyakhetarpal/python-flint/actions/runs/13576857716/job/37954995494?pr=1 with the following error trace:
|
FWIW; I hardly have any Cython development experience, so I will have to wrap my head around this a bit, unless someone has an idea for a fix. The basis for the failure here is that WASM is stricter about type conversions than compilation for other platforms, which is why this mismatch is causing errors. |
Okay – the cause of the error, more specifically, is that python-flint/src/flint/types/_gr.pxd Lines 1189 to 1202 in 34dbb5d
but and this has apparently been fixed in version v3.1.0. I'll try updating |
Successful build: https://github.com/agriyakhetarpal/python-flint/actions/runs/13577635441?pr=1 ✨ The next task is to run tests, which I can take a look at later in the day. |
I haven't looked through this yet but thanks! |
Best to keep with latest FLINT version for the pyodide build. There is no need to support a range of versions in pyodide. Generally python-flint tracks the latest version but has some |
Sounds good to me! I think the tests will have a bunch of function signature mismatches, though 😬 based on what I noticed through my fork's run. Also, building itself takes ~20 minutes in total – we could get to half of that if we have prebuilt WASM binaries. I guess they could be hosted somewhere in a GitHub release when things are ready. |
d4d9652
to
5dc6770
Compare
This can take a bit of time. I'm disabling tests and running it in on my fork to see how widespread the signature mismatches are, and I managed to get |
I don't quite understand what the problem with the tests is. Is it just caused by the Cython/C signatures not matching? There is now FLINT 3.2.0-rc1. We could perhaps update all the Cython declarations to that version and use that for the WASM build. I'm not sure when 3.2.0 final will be released but we would want to update to that straight away when it is. |
Yes, majorly. If there's even a slight mismatch between the upstream signatures for Flint and the ones we have here, WASM's type safety guarantees that it will terminate the program, which makes Pyodide raise a fatal error. These can happen at the time of linking (like when it happened with version 3.0.1 above), or at runtime, like we are facing now. I have to drop these completely instead of xfailing them as the running Pyodide interpreter is no longer possible to be used after it has encountered fatal errors. There are so many of these with SciPy, especially from its wrapping of Fortran libraries that we can't compile to WASM directly without having to
Yes, this sounds good to me, and I hope they've fixed a few of these upstream. I could start another PR for this when I have enough time to do so; I assume it will help me learn Cython a bit. :) |
I've opened gh-264 to bump the FLINT version to 3.2.0-rc1 and update all of the Cython declarations. The Cython declarations are set from the FLINT docs by running the |
I've just merged this to main. If you rebase/merge with main then this PR will get the updated declarations. There might still be some wrong declarations that we can fix manually for now but then upstream to FLINT afterwards. |
Thank you! Let's see how this goes; I've improved the caching for the CI job, though I've kept the failing tests skipped. I have been facing trouble building GMP locally with my installed Emscripten toolchain, let alone the rest of the libraries, so CI is my testbed for now. |
It is expected that the ARM Linux jobs will fail after gh-264. I think that the "Test coverage meson build" job is failing here because of this line that should be deleted: python-flint/coverage_plugin.py Line 131 in 242c48e
Somehow the changes here to test_all.py are causing that print to happen at a different time.
|
The WASM job says
coming from
|
cd flint-3.2.0-rc1 | ||
|
||
emconfigure ./configure \ | ||
--disable-dependency-tracking \ | ||
--disable-shared \ | ||
--prefix=${{ env.WASM_LIBRARY_DIR }} \ | ||
--with-gmp=${{ env.WASM_LIBRARY_DIR }} \ | ||
--with-mpfr=${{ env.WASM_LIBRARY_DIR }} |
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.
I think you need to set something like --disable-assembly
or --host=...
here.
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.
I don't know what the host triple is for WASM
Co-Authored-By: Oscar Benjamin <[email protected]>
Thanks, the host triple should be |
Co-Authored-By: Oscar Benjamin <[email protected]>
The CMake build is not recommended except for Windows. I don't know if it would work. |
The label suffix is used by the macro preprocessor that runs on the assembly code. I don't think that there is any assembly code for WASM. This is why I suggested |
Ah, wait, missed that comment |
I think it wouldn't help, as we have a warning as follows:
so |
The most rudimentary reason for this is because the compiler is misconfigured – or it's probably coming from flintlib/flint@76c90d5. |
@albinahlback do you know what options need to be passed to configure to build for emscripten/WASM? Currently configure fails with:
This was working before with FLINT 3.1 but now fails with 3.2.0-rc1. As I understand it determining the label suffix is not needed when building for WASM since assembly will be disabled anyway. Maybe that has changed since I last looked at it though... |
It is needed for |
Disable assembly does not mean disabling assembly completely, it just means disable compiling assembly files. Perhaps this could be made more clear, but that's the meaning behind it. |
From what I see it looks like this is only defined for What should we do in the WASM case? |
I'll fix this before releasing 3.2.0. Please open up an issue at FLINT. |
I've opened flintlib/flint#2247 for this. |
Closes #234