-
Notifications
You must be signed in to change notification settings - Fork 32
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. |
It seems to crash on the third or fourth of these lines: python-flint/src/flint/test/test_all.py Lines 1645 to 1648 in 242c48e
That is just constructing the fmpz_mod context. It fails for the larger modulus but the same functions (in terms of FLINT's C API) are called for all moduli. Looking at it though I see: python-flint/src/flint/types/fmpz_mod.pyx Lines 35 to 49 in 242c48e
I think that the call to fmpz_mod_discrete_log_pohlig_hellman_clear is wrong. It should be fmpz_mod_discrete_log_pohlig_hellman_init . That line was added in gh-95 which was apparently to prevent a Windows crash seen at the time. That code doesn't use the modulus though so it should not matter whether the modulus is large or small...
The modulus comes in here: python-flint/src/flint/types/fmpz_mod.pyx Lines 55 to 73 in 242c48e
I don't think that any of those functions would have a signature mismatch. I'm a bit confused about exactly what is happening but the only thing I can immediately see that seems wrong is that call to |
Depending on the modulus |
I'm not sure why but apparently this line causes the main problem: python-flint/src/flint/types/fmpz_mod.pyx Line 73 in 242c48e
At least if it is commented out then we don't see the crash... (although other tests fail) |
def use_fmpz_is_probabprime(): | ||
cdef fmpz p | ||
p = fmpz(2**127 - 1) | ||
return fmpz_is_probabprime(p.val) |
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.
Just calling this function is enough to cause the crash:
Pyodide has suffered a fatal error. Please report this to the Pyodide maintainers.
The cause of the fatal error was:
RuntimeError: null function or function signature mismatch
at wasm://wasm/02e45b4a:wasm-function[1550]:0x1de118
../.venv-pyodide/lib/python3.12/site-packages/flint/test/test_all.py::test_use_fmpz_is_probabprime Stack (most recent call first):
at wasm://wasm/02e45b4a:wasm-function[1547]:0x1de04f
File "/home/runner/work/python-flint/python-flint/.venv-pyodide/lib/python3.12/site-packages/flint/test/test_all.py", line 4682 in test_use_fmpz_is_probabprime
File "/home/runner/work/python-flint/python-flint/.venv-pyodide/lib/python3.12/site-packages/_pytest/python.py", line 159 in pytest_pyfunc_call
at wasm://wasm/02e45b4a:wasm-function[1531]:0x1dcd97
at wasm://wasm/02e45b4a:wasm-function[1530]:0x1dcba1
at wasm://wasm/02e45b4a:wasm-function[1521]:0x1dae4b
at wasm://wasm/02e45b4a:wasm-function[1522]:0x1db7dd
at wasm://wasm/02e45b4a:wasm-function[313]:0xa924e
at wasm://wasm/02e45b4a:wasm-function[220]:0x9dce6
at wasm://wasm/0268a37a:wasm-function[1160]:0x1c4cf8
at wasm://wasm/0268a37a:wasm-function[3428]:0x2a2a6d {
pyodide_fatal_error: true
}
It would be great if we could see what the functions in the stacktrace are but their names are all replaced with numbers in the minified wasm build. What I don't understand is if the mismatch occurs exactly when calling Note that the crash occurs when the python-flint/src/flint/flintlib/types/flint.pxd Lines 39 to 40 in 242c48e
This is specifying that an fmpz_t is an array of one slong which means an unsigned long that is 32 bits in a wasm32 bit build. The type of p.val is declared as fmpz_t :python-flint/src/flint/types/fmpz.pxd Lines 28 to 36 in 242c48e
Ultimately the C code generated for this struct says that val is of type slong[1] .
When we call Note that in the browser at sympy live you can use python-flint 0.6.0 along with FLINT 3.1 and this works fine there:
Also there the whole test suite passes at least if the doctests are skipped (if they are not skipped then it seems to hang and not run them):
I'm at a bit of a loss here. Maybe better debug output would help. |
This should go from FLINT main to rc2: flintlib/flint#2247 (comment) |
I'm going to try diving deeper into |
Closes #234